Mir

Merge lp://qastaging/~mir-team/mir/160bit-hash-mac into lp://qastaging/mir

160bit-hash-mac
Merge into development-branch

Proposed by Brandon Schaefer on 2015-11-16

Status:	Rejected
Rejected by:	Brandon Schaefer on 2016-01-14
Proposed branch:	lp://qastaging/~mir-team/mir/160bit-hash-mac
Merge into:	lp://qastaging/mir
Diff against target:	1535 lines (+244/-129) 44 files modified 3rd_party/android-input/android/frameworks/base/include/androidfw/Input.h (+8/-6) 3rd_party/android-input/android/frameworks/base/include/androidfw/InputTransport.h (+4/-4) 3rd_party/android-input/android/frameworks/base/services/input/Input.cpp (+2/-2) 3rd_party/android-input/android/frameworks/base/services/input/InputListener.cpp (+2/-2) 3rd_party/android-input/android/frameworks/base/services/input/InputListener.h (+5/-4) 3rd_party/android-input/android/frameworks/base/services/input/InputReader.cpp (+17/-17) 3rd_party/android-input/android/frameworks/base/services/input/InputTransport.cpp (+6/-6) 3rd_party/android-input/android/frameworks/base/services/input/KeyCharacterMap.cpp (+1/-1) include/client/mir/events/event_builders.h (+21/-3) include/cookie/mir/cookie_factory.h (+2/-0) include/cookie/mir_toolkit/cookie.h (+2/-1) src/client/events/event_builders.cpp (+37/-9) src/client/input/input_event.cpp (+3/-3) src/client/mir_surface.cpp (+1/-1) src/cookie/cookie_factory.cpp (+15/-5) src/include/common/mir/events/event_private.h (+4/-2) src/protobuf/mir_protobuf.proto (+1/-1) src/server/frontend/session_mediator.cpp (+6/-1) src/server/input/android/input_sender.cpp (+2/-0) src/server/input/android/input_translator.cpp (+13/-5) src/server/input/default_event_builder.cpp (+11/-5) src/server/input/key_repeat_dispatcher.cpp (+2/-1) src/server/input/surface_input_dispatcher.cpp (+3/-1) src/server/input/validator.cpp (+1/-1) tests/acceptance-tests/test_client_input.cpp (+1/-1) tests/acceptance-tests/test_surface_modifications.cpp (+4/-2) tests/acceptance-tests/test_surface_placement.cpp (+1/-1) tests/acceptance-tests/test_surface_raise.cpp (+3/-2) tests/acceptance-tests/test_surface_specification.cpp (+4/-2) tests/unit-tests/client/input/test_android_input_receiver.cpp (+2/-2) tests/unit-tests/client/input/test_xkb_mapper.cpp (+1/-1) tests/unit-tests/frontend/test_event_sender.cpp (+1/-1) tests/unit-tests/frontend/test_session_mediator.cpp (+6/-0) tests/unit-tests/input/android/test_android_input_lexicon.cpp (+12/-6) tests/unit-tests/input/android/test_android_input_sender.cpp (+3/-3) tests/unit-tests/input/android/test_input_translator.cpp (+3/-3) tests/unit-tests/input/test_event_builders.cpp (+16/-6) tests/unit-tests/input/test_event_filter_chain_dispatcher.cpp (+1/-1) tests/unit-tests/input/test_key_repeat_dispatcher.cpp (+2/-2) tests/unit-tests/input/test_surface_input_dispatcher.cpp (+10/-10) tests/unit-tests/input/test_validator.cpp (+1/-1) tests/unit-tests/scene/test_abstract_shell.cpp (+1/-1) tests/unit-tests/scene/test_surface.cpp (+2/-2) tests/unit-tests/test_mir_cookie.cpp (+1/-1)
To merge this branch:	bzr merge lp://qastaging/~mir-team/mir/160bit-hash-mac
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Alexandros Frantzis (community)			Needs Fixing on 2015-11-30
Alan Griffiths			Needs Fixing on 2015-11-30
PS Jenkins bot (community)	continuous-integration		Needs Fixing on 2015-11-28
Chris Halse Rogers		2015-11-16	Needs Fixing on 2015-11-17
Review via email: mp+277636@code.qastaging.launchpad.net

Commit message

When using the sha1 hash we were only saving 64bits of the hash and comparing that. Now we save the entire 160bit hash so its much harder to fake.

Description of the change

When using the sha1 hash we were only saving 64bits of the hash and comparing that. Now we save the entire 160bit hash so its much harder to fake.

Addressing the security review here:
https://code.launchpad.net/~mir-team/mir/attestable-timestamps-server/+merge/270457

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5559/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-11-17:

Multiple places:
+ inline std::array<uint8_t, 20> getMac() const { return mMac; }

There's no particular reason to require a copy here; return a const reference instead?

Multiple places:
- mMac = from.mMac;
+ memcpy(&mMac[0], from.mMac.data(), sizeof(from.mMac));

I believe these changes to be unnecessary; std::array::operator= will do the right thing. Also, &mMac[0] is an odd construct :).

316 - NotifyMotionArgs args(when, 0,getDeviceId(), mSource, policyFlags,
317 + NotifyMotionArgs args(when, {},getDeviceId(), mSource, policyFlags,

While you're here you might as well fix the missing ' ' before getDeviceId() :)

It might be a good idea to stick a “using Mac = std::array<uint8_t, 20>;” in cookie_factory.h, and then using that rather than explicitly std::array<>? Maybe? Explicit std::array<> has the advantage of making the fixed protocol obviously fixed.

Otherwise fine.

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-17:

=== modified file 'include/client/mir/events/event_builders.h'

This breaks ABI, we need to keep compatibility overloads. (Or, if we chose to "get away with it" at least confirm that qtmir and unity8 builds are unaffected.)

~~~~

- mev.mac = mac;
+ memcpy(&mev.mac, mac.data(), sizeof(mev.mac));

(Many times) there's no need, the assignment operator does the right thing. (And, while it likely works, have you checked that the standardese supports memcpy of std::array<>?)

~~~~

+ int count = 0;
+ for (size_t i = 0; i < sizeof(calculated_mac); i++)
+ {
+ if (calculated_mac[i] == cookie.mac[i])
+ {
+ count++;
+ }
+ }
+
+ return count == sizeof(cookie.mac);

"return !memcmp(calculated_mac, cookie.mac, sizeof(calculated_mac));" is more readable (and likely more efficient).

~~~~

I'm not sure of the "right fix" here, but...

There's a lot of repetition of "std::array<uint8_t, 20>". It isn't an accident that they are the same, so give it a name (using Mac = std::array<uint8_t, 20>). Or, as it would be convenient to be compatible with C, it might be better with:

// MAC is a 160bit sha1 hash
typedef struct MirMac { uint8_t data[20]; } MirMac;

in a mircookie header.

review: Needs Fixing

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-11-17:

> === modified file 'include/client/mir/events/event_builders.h'
>
> This breaks ABI, we need to keep compatibility overloads. (Or, if we chose to
> "get away with it" at least confirm that qtmir and unity8 builds are
> unaffected.)
>
> ~~~~
Dammit you're right those bits landed in 0.16... Hmmm Ill need to make a compatibility overload... :(

>
> - mev.mac = mac;
> + memcpy(&mev.mac, mac.data(), sizeof(mev.mac));
>
> (Many times) there's no need, the assignment operator does the right thing.
> (And, while it likely works, have you checked that the standardese supports
> memcpy of std::array<>?)
>
> ~~~~

That should be needed? Since mev.mac is a uint64_t and mav.mac_2 is a uint8_t[12]. So i need to copy 64 bits into mev.mac then 12 * 8 bits in to mac_2? As well as:
error: cannot convert ‘const std::array<unsigned char, 20ul>’ to ‘uint64_t {aka long unsigned int}’ in assignment

When attempted.

http://en.cppreference.com/w/cpp/container/array/data

data() + size() is always valid. So as long as I do data() < sizeof(mev.mac) < size() then its valid.

>
> + int count = 0;
> + for (size_t i = 0; i < sizeof(calculated_mac); i++)
> + {
> + if (calculated_mac[i] == cookie.mac[i])
> + {
> + count++;
> + }
> + }
> +
> + return count == sizeof(cookie.mac);
>
> "return !memcmp(calculated_mac, cookie.mac, sizeof(calculated_mac));" is more
> readable (and likely more efficient).
>
> ~~~~

Reason I did it this way was to avoid short circuiting to avoid a timed attack. Checking the man pages of memcmp:

Do not use memcmp() to compare security critical data, such as cryptographic secrets, because the required CPU time depends on the number of equal bytes.

Which sounds like short circuiting to me. Though I could look at making a better one...

>
> I'm not sure of the "right fix" here, but...
>
> There's a lot of repetition of "std::array<uint8_t, 20>". It isn't an accident
> that they are the same, so give it a name (using Mac = std::array<uint8_t,
> 20>). Or, as it would be convenient to be compatible with C, it might be
> better with:
>
> // MAC is a 160bit sha1 hash
> typedef struct MirMac { uint8_t data[20]; } MirMac;
>
> in a mircookie header.

True, the reason I did std::array<> in the c++ bits was because it was a bit safer then doing a straight uint8_t const* in the functions. Guarantee I have 20 bytes.

> === modified file 'include/client/mir/events/event_builders.h'
> 
> This breaks ABI, we need to keep compatibility overloads. (Or, if we chose to
> "get away with it" at least confirm that qtmir and unity8 builds are
> unaffected.)
> 
> ~~~~
Dammit you're right those bits landed in 0.16... Hmmm Ill need to make a compatibility overload... :(

> 
> -    mev.mac = mac;
> + memcpy(&mev.mac, mac.data(), sizeof(mev.mac));
> 
> (Many times) there's no need, the assignment operator does the right thing.
> (And, while it likely works, have you checked that the standardese supports
> memcpy of std::array<>?)
> 
> ~~~~

When attempted.

http://en.cppreference.com/w/cpp/container/array/data

data() + size() is always valid. So as long as I do data() < sizeof(mev.mac) < size() then its valid.

> 
> +        int count = 0;
> +        for (size_t i = 0; i < sizeof(calculated_mac); i++)
> +        {
> +            if (calculated_mac[i] == cookie.mac[i])
> +            {
> +                count++;
> +            }
> +        }
> +
> + return count == sizeof(cookie.mac);
> 
> "return !memcmp(calculated_mac, cookie.mac, sizeof(calculated_mac));" is more
> readable (and likely more efficient).
> 
> ~~~~

Reason I did it this way was to avoid short circuiting to avoid a timed attack. Checking the man pages of memcmp:

Do  not  use  memcmp()  to compare security critical data, such as cryptographic secrets, because the required CPU time depends on the number of equal bytes.

Which sounds like short circuiting to me. Though I could look at making a better one...
 
> 
> I'm not sure of the "right fix" here, but...
> 
> There's a lot of repetition of "std::array<uint8_t, 20>". It isn't an accident
> that they are the same, so give it a name (using Mac = std::array<uint8_t,
> 20>). Or, as it would be convenient to be compatible with C, it might be
> better with:
> 
>     // MAC is a 160bit sha1 hash
>     typedef struct MirMac { uint8_t data[20]; } MirMac;
> 
> in a mircookie header.

True, the reason I did std::array<> in the c++ bits was because it was a bit safer then doing a straight uint8_t const* in the functions. Guarantee I have 20 bytes.

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-17

3107. By Brandon Schaefer on 2015-11-17: * Use = when moving between std::array vs std::array
* Fix some other small things

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5568/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-11-17:

> Multiple places:
> + inline std::array<uint8_t, 20> getMac() const { return mMac; }
>
> There's no particular reason to require a copy here; return a const reference
> instead?
>
> Multiple places:
> - mMac = from.mMac;
> + memcpy(&mMac[0], from.mMac.data(), sizeof(from.mMac));
>
> I believe these changes to be unnecessary; std::array::operator= will do the
> right thing. Also, &mMac[0] is an odd construct :).
>
>
> 316 - NotifyMotionArgs args(when, 0,getDeviceId(), mSource,
> policyFlags,
> 317 + NotifyMotionArgs args(when, {},getDeviceId(), mSource,
> policyFlags,
>
> While you're here you might as well fix the missing ' ' before getDeviceId()
> :)
>

Done.

> It might be a good idea to stick a “using Mac = std::array<uint8_t, 20>;” in
> cookie_factory.h, and then using that rather than explicitly std::array<>?
> Maybe? Explicit std::array<> has the advantage of making the fixed protocol
> obviously fixed.
>
> Otherwise fine.

I would have to manually do the using Mac for all the headers (maybe a few?). If thats wanted I can do that.

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-11-17:

But yeah... if we keep the std::array<..., 20> we are kind of stuck at 20 anyway?

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-18

3108. By Brandon Schaefer on 2015-11-18: * Need to maintain API (need to double check if we can remove those uint64_t functions)
* using Mac = std::array<uint64_t, 20>

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-18:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5569/rebuild

review: Needs Fixing (continuous-integration)

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-18

3109. By Brandon Schaefer on 2015-11-18: * Move to use Mac vs std::array<uint64_t, 20>

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-18:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5570/rebuild

review: Needs Fixing (continuous-integration)

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-18

3110. By Brandon Schaefer on 2015-11-18: * Use using Mac = ... in android bits
3111. By Brandon Schaefer on 2015-11-18: * Merge trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-18:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5583/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-19:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5584/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-19:

+ // Cant break protocol so the extra bits we need for mac are stored here
+ uint8_t mac_2[12];

1. MirMotionEvent is a private struct, not part of the protocol.
2. why can't we break protocol? We guarantee the client ABI, not the protocol.

OTOH

- required uint64 mac = 2;
+ required bytes mac = 2;

this is a protocol break. I don't think we need it, but this is the place to put an "extra bits" field (which should be optional for backwards compatibility).

~~~~

+ std::array<uint8_t, 20> mac{};

should be:

mir::cookie::Mac mac;

~~~~

+ Mac mMac; // 160bits for sha1 hash

this comment is redundant. It risks getting out of step with the code as anyone changing the definition of Mac won't look here.

~~~~

There are two definitions of

using Mac = std::array<uint8_t, 20>;

it would be better to use a single one.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-19:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5596/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-11-19:

> + // Cant break protocol so the extra bits we need for mac are stored here
> + uint8_t mac_2[12];
>
> 1. MirMotionEvent is a private struct, not part of the protocol.
> 2. why can't we break protocol? We guarantee the client ABI, not the protocol.
>
> OTOH

Hmm the issue being that i added that mac field in the middle of the struct? Doing that broke the protocol? (I was also confused to why a private struct was causing these issues). Just wanted to avoid breaking things ... again.

>
> - required uint64 mac = 2;
> + required bytes mac = 2;
>
> this is a protocol break. I don't think we need it, but this is the place to
> put an "extra bits" field (which should be optional for backwards
> compatibility).
>

This shouldnt be an issue since this is part of the client cookie part (which hasnt been released yet and is staged to be release in 0.18).

> ~~~~
>
> + std::array<uint8_t, 20> mac{};
>
> should be:
>
> mir::cookie::Mac mac;
>
> ~~~~
>

Opps. Fixed.

> + Mac mMac; // 160bits for sha1 hash
>
> this comment is redundant. It risks getting out of step with the code as
> anyone changing the definition of Mac won't look here.
>
> ~~~~

Agree! Fixed.

>
> There are two definitions of
>
> using Mac = std::array<uint8_t, 20>;
>
> it would be better to use a single one.

I did it this way... because one was for the 3rd_party android stuff and the other for the public cookie factory API. If we want a single one then the android input would need to include the cookie_factory.h.

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-19

3112. By Brandon Schaefer on 2015-11-19: * Turn everything into a mir::cookie::Mac expect things that depend on the proctol in InputTransport

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-20:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5602/rebuild

review: Needs Fixing (continuous-integration)

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-20

3113. By Brandon Schaefer on 2015-11-20: * Vivid has an older gcc which doesnt support the {} for an init array, soo use {{}} for now

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-20:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5604/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-20:

+ mir::cookie::Mac mac;
+ memcpy(&mac[0], cookie.mac, mac.size());

Don't memcpy std::array<>

~~~~

+// C++ std lib
+#include <array>

Not needed now.

~~~~

+// MAC is a 160bit sha1 hash
typedef struct MirCookie
{

Comment surely belongs with the definition of Mac (and should be spelt accordingly)

~~~~

- uint64_t mac;
+ // TODO remove when its ok to break protocol (or when we do break protocol) FIXME
+ uint64_t unused;

I still don't understand why you're doing this.

review: Needs Fixing

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-20

3114. By Brandon Schaefer on 2015-11-20: * Move to std::copy from memcpy
* More fixes for vivid gcc older version
3115. By Brandon Schaefer on 2015-11-20: * Missed one (Hopefully thats right)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-20:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5618/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-21:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5619/rebuild

review: Needs Fixing (continuous-integration)

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-21

3116. By Brandon Schaefer on 2015-11-21: * Missed two more
3117. By Brandon Schaefer on 2015-11-21: * Init the array

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-21:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5621/rebuild

review: Needs Fixing (continuous-integration)

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-21

3118. By Brandon Schaefer on 2015-11-21: * Move vivid fixes

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-21:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5622/rebuild

review: Needs Fixing (continuous-integration)

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-23

3119. By Brandon Schaefer on 2015-11-23: * This doesnt break protocol, InputTrasport does (when changed)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-23:

FAILED: Continuous integration, rev:3119
http://jenkins.qa.ubuntu.com/job/mir-ci/5629/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4990
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3896
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4936
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/892/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1782/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/890
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/890/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4936
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4936/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7496
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25389
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/893/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5629/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-23:

Mostly style nits:

+ event_cookie->set_mac(reinterpret_cast<char const*>(cookie.mac), sizeof(cookie.mac));

reinterpret_cast<> should be the penultimate resort to do something non-portable. That isn't the case here as *no cast is required*.

~~~~

+ std::array<uint8_t, 20> mac;
             int32_t deviceId;
             int32_t source;
             int32_t action;
@@ -81,7 +81,7 @@
         struct Motion {
             uint32_t seq;
             int64_t eventTime;
- uint64_t mac;
+ std::array<uint8_t, 20> mac;

mir::cookie::Mac

~~~~

std::begin(cookie.mac) + sizeof(cookie.mac)

can be written

std::end(cookie.mac)

~~~~

+ mir::cookie::Mac mac;
+ auto cookie = cookie_factory->timestamp_to_cookie(timestamp.count());
+ std::copy(std::begin(cookie.mac), std::begin(cookie.mac) + sizeof(cookie.mac), mac.begin());

Why stick the "auto cookie..." line between creating and initializing mac?

~~~~

=== modified file 'src/client/input/input_event.cpp'
...
+#include <string.h>
+

Not needed.

~~~~

=== modified file 'src/server/input/default_event_builder.cpp'
...
+#include <string.h>

Not needed.

~~~~

Finally, just a (non-blocking) comment:

+ std::copy(old_kev.mac.begin(), old_kev.mac.end(), std::begin(cookie.mac));

could be written as

std::copy(begin(old_kev.mac), end(old_kev.mac), std::begin(cookie.mac));

review: Needs Fixing

lp://qastaging/~mir-team/mir/160bit-hash-mac updated on 2015-11-23

3120. By Brandon Schaefer on 2015-11-23: * Fix reviewers comments
3121. By Brandon Schaefer on 2015-11-23: * More fixes
3122. By Brandon Schaefer on 2015-11-23: * A couple more :)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-23:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5640/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-23:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5644/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-28:

FAILED: Continuous integration, rev:3122
http://jenkins.qa.ubuntu.com/job/mir-ci/5710/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5088/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3994/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5036/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/39/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/39/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5036/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5710/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-30:

+ event_cookie->set_mac(reinterpret_cast<char const*>(cookie.mac), sizeof(cookie.mac));

reinterpret_cast<> should be the penultimate resort to do something non-portable. That isn't the case here as *no cast is required*.

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-11-30:

A possible security concern (not sure if we should worry about this in practice with all the IPC timing noise, but still...):

+ // The check needs to be in constant time. Cannot use memcmp.
+ ...
+ if (calculated_mac[i] == cookie.mac[i])
+ {
+ count++;
+ }

Although the proposed method is certainly much better than a premature return, using an if statement will still leak timing information to an attacker, since success and failure for a byte will use different code paths [1]. For improved security from such attacks I would recommend a fixed code path (assuming proper compilation):

count += (calculated_mac[i] == cookie.mac[i]);

[1] A possible attack: start with all zero mac, for each byte in mac: for each of the 256 values for that byte: measure time for mac verification, keep value for that byte that results in highest verification time, complexity = O(sizeof(mac))

review: Needs Fixing

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-12-01:

Thanks for the reviews! This is going to be put on hold now that i have some extra time. The main things I want before this goes in:

Refactor InputTrasnport/Message to take in a std::vector. Needs some refactoring in there.
Move to an opaque structure in cookie_factory as well as the client cookie stuff.
Move to the 160 bit size for the mac field when dealing with c++. The main (hidden structure at this point) will be a unit8_t[20] but since it'll be opaque then turned into a std::vector we can freely change the size of the hash!

Unmerged revisions

3122. By Brandon Schaefer on 2015-11-23: * A couple more :)
3121. By Brandon Schaefer on 2015-11-23: * More fixes
3120. By Brandon Schaefer on 2015-11-23: * Fix reviewers comments
3119. By Brandon Schaefer on 2015-11-23: * This doesnt break protocol, InputTrasport does (when changed)
3118. By Brandon Schaefer on 2015-11-21: * Move vivid fixes
3117. By Brandon Schaefer on 2015-11-21: * Init the array
3116. By Brandon Schaefer on 2015-11-21: * Missed two more
3115. By Brandon Schaefer on 2015-11-20: * Missed one (Hopefully thats right)
3114. By Brandon Schaefer on 2015-11-20: * Move to std::copy from memcpy
* More fixes for vivid gcc older version
3113. By Brandon Schaefer on 2015-11-20: * Vivid has an older gcc which doesnt support the {} for an init array, soo use {{}} for now