add and use constant time 32 byte equality function #3999

moneromooo-monero · 2018-06-13T17:26:39Z

Oddly, it looks like while it's way slower than memcmp, the timings for both different and equal are pretty similar for both.

coneiric · 2018-06-13T18:11:13Z

src/device/device_ledger.cpp

@@ -127,7 +127,7 @@ namespace hw {
    }

    bool operator==(const crypto::key_derivation &d0, const crypto::key_derivation &d1) {
-      return !memcmp(&d0, &d1, sizeof(d0));
+      return !crypto_verify_32(&d0, &d1, sizeof(d0));


@moneromooo-monero I can't find an overload of crypto_verify_32 that takes three parameters. Am I missing/misunderstanding something?

Probably that this file isn't a dep of unit tests nor performance tests so didn't get rebuilt. Fixing.

Ah, no, its because this is only built if some lib dep is installed.

stoffu · 2018-06-14T01:38:17Z

src/device/device_ledger.cpp

@@ -127,7 +127,8 @@ namespace hw {
    }

    bool operator==(const crypto::key_derivation &d0, const crypto::key_derivation &d1) {
-      return !memcmp(&d0, &d1, sizeof(d0));
+      static_assert(sizeof(crypto::key_derivation) == 32, "key_derivation must be 32 bytes");
+      return !crypto_verify_32(&d0, &d1);


From buildbot:

/home/vagrant/slave/monero-static-ubuntu-amd64/build/src/device/device_ledger.cpp:131:40: error: cannot convert 'const crypto::key_derivation*' to 'const unsigned char*' for argument '1' to 'int crypto_verify_32(const unsigned char*, const unsigned char*)' return !crypto_verify_32(&d0, &d1);

vtnerd · 2018-06-14T18:59:24Z

src/crypto/generic-ops.h

@@ -34,13 +34,18 @@
 #include <cstring>
 #include <functional>

+extern "C"


I think it makes more sense to do this in the verify.h file since this is a primarily a C++ project. #iifdef __cplusplus is the usual way of detecting this at the top and bottom of the file.

moneromooo-monero · 2018-07-19T13:07:01Z

~~Do not merge yet, it seems to kill performance in a full sync due to the large number of hash compares, which may not need to be constant time.~~ Fixed in the caller of operator==, will PR later.

stoffu · 2018-07-20T05:03:50Z

Hmm, looks like the function name crypto_verify_32 is already used by libsodium (https://github.com/jedisct1/libsodium/blob/91d9051bce2660dc3f7a6fd890e0ddb602848c22/src/libsodium/crypto_verify/sodium/verify.c#L89) which causes link error on Win64:

C:/msys64/mingw64/lib/libsodium.a(libsodium_la-verify.o):(.text+0x50): multiple definition of `crypto_verify_32'
../crypto/libcncrypto.a(verify.c.obj):verify.c:(.text+0x0): first defined here

https://build.getmonero.org/builders/monero-static-win64/builds/4832/steps/compile/logs/stdio

and OSX:

duplicate symbol _crypto_verify_32 in:
    ../crypto/libcncrypto.a(verify.c.o)
    /usr/local/lib/libzmq.a(src_libzmq_la-tweetnacl.o)

https://build.getmonero.org/builders/monero-static-osx-10.13/builds/789/steps/compile/logs/stdio

Would it make sense to use libsodium's version?

moneromooo-monero · 2018-07-20T08:51:07Z

Weird, I didn't know we were using that...
If we are, then it makes sense to use it.

stoffu · 2018-07-20T12:56:44Z

Hmm, OSX build is getting this error:

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
SODIUM_LIBRARY
    linked by target "cncrypto" in directory /Users/buildbot/monero-builder/monero-static-osx-10_13/build/src/crypto

Not sure how to fix it.

Win64 build is failing now due to the trivial 'sleep' was not declared error that's fixed in the current master. Can you rebase this PR so as to see if win64 build succeeds on buildbot?

moneromooo-monero · 2018-07-20T13:05:52Z

By installing libsodium-devel I think. It's probably not finding headers, which it did not need when it was used via libzmq only.

iamsmooth · 2018-08-08T10:39:56Z

src/crypto/generic-ops.h

+      return crypto_verify_32((const unsigned char*)&_v1, (const unsigned char*)&_v2) == 0; \
+    if (sizeof(_v1) == 8) \
+      return (const uint64_t&)_v1 == (const uint64_t&)_v2; \
+    return !memcmp(&_v1, &_v2, sizeof(_v1)); \


This is still not constant time for the case of size other than 32 or 8 bytes, right?

While this may not matter for current usage it is leaving a booby trap for some future usage.

This is correct about 32/8 vs others. The operator doesn't make any claim to be constant time in the first place, so I don't see any booby trap here. If you want constant time, you wouldn't use an operator which does not claim to do so.

I don't understand the purpose of this commit except to implement the operation in constant time (otherwise the original and simpler memcmp would have been just fine). You seem to be suggesting either that the operator now implements (presumably because this is relied upon by its use cases) constant time but this is not 'claimed' or that the operator claims to be constant time but only for 8 and 32 bytes but not other sizes?

Either of these seems quite odd and brittle to me, although I guess the latter would be okay if clearly documented.

The operator== defined by that macro does not claim to do constant time. It just happens to do so for 32 bytes because we want it for secret keys and secret keys are 32 bytes. The 8 byte case is just drive by really, and on second thoughts maybe the memcmp is better since we might get unaligned 8 bytes if they're not uint64_t.

That's a bit more complex, but duplicates the macros so there's one specifically for constant time. Still building here so might need updating is tests break.

It just happens to do so for 32 bytes because we want it for secret keys and secret keys are 32 bytes

This is exactly what I am saying about relying on brittle behavior.

If we want constant time for private keys then there should a specific defined mechanism for getting that.

stoffu · 2018-08-09T23:03:37Z

OSX builds are failing due to libsodium missing, apparently.

@Pigeons Could you install it?

Edit: I’ve misquoted someone else, sorry:p

danrmiller · 2018-08-10T14:28:27Z

I've installed libsodium on the OSX machines and kicked off the rebuilds.

d2e26c2 add and use constant time 32 byte equality function (moneromooo-monero)

coneiric reviewed Jun 13, 2018

View reviewed changes

moneromooo-monero force-pushed the ver32 branch from 869335a to c6e2e3c Compare June 13, 2018 20:02

stoffu reviewed Jun 14, 2018

View reviewed changes

moneromooo-monero force-pushed the ver32 branch from c6e2e3c to f867294 Compare June 14, 2018 07:55

stoffu approved these changes Jun 14, 2018

View reviewed changes

vtnerd reviewed Jun 14, 2018

View reviewed changes

moneromooo-monero force-pushed the ver32 branch from f867294 to 28ca53d Compare June 14, 2018 19:28

anonimal approved these changes Jun 23, 2018

View reviewed changes

moneromooo-monero force-pushed the ver32 branch 2 times, most recently from 6bc05ee to e95cfcf Compare July 4, 2018 21:23

moneromooo-monero changed the title ~~add and use constant time 32 byte equality function~~ [DO NOT MERGE] add and use constant time 32 byte equality function Jul 19, 2018

moneromooo-monero changed the title ~~[DO NOT MERGE] add and use constant time 32 byte equality function~~ add and use constant time 32 byte equality function Jul 19, 2018

moneromooo-monero force-pushed the ver32 branch from e95cfcf to 21632ed Compare July 20, 2018 11:08

moneromooo-monero force-pushed the ver32 branch from 21632ed to 2440b5c Compare July 20, 2018 13:07

stoffu approved these changes Jul 20, 2018

View reviewed changes

stoffu mentioned this pull request Aug 8, 2018

Upstream merge 1 aeonix/aeon#30

Merged

iamsmooth reviewed Aug 8, 2018

View reviewed changes

moneromooo-monero force-pushed the ver32 branch from 2440b5c to 9786c9d Compare August 9, 2018 18:35

stoffu approved these changes Aug 9, 2018

View reviewed changes

add and use constant time 32 byte equality function

d2e26c2

moneromooo-monero force-pushed the ver32 branch from 9786c9d to d2e26c2 Compare August 23, 2018 07:57

luigi1111 merged commit d2e26c2 into monero-project:master Aug 23, 2018

luigi1111 added a commit that referenced this pull request Aug 23, 2018

Merge pull request #3999

fa7cdd6

d2e26c2 add and use constant time 32 byte equality function (moneromooo-monero)

stoffu mentioned this pull request Sep 4, 2018

[DO NOT MERGE] pro: link against libsodium on Linux monero-project/monero-gui#1540

Closed

sanderfoobar mentioned this pull request Oct 10, 2018

Linking libsodium monero-project/monero-gui#1625

Merged

This was referenced Feb 19, 2019

Upstream merge 4 aeonix/aeon#99

Closed

Upstream merge 4 [fixed] aeonix/aeon#100

Closed

stoffu mentioned this pull request Mar 12, 2019

Upstream merge 4 aeonix/aeon#106

Merged

stoffu mentioned this pull request Dec 23, 2019

Upstream merge 5 aeonix/aeon#167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add and use constant time 32 byte equality function #3999

add and use constant time 32 byte equality function #3999

moneromooo-monero commented Jun 13, 2018

coneiric Jun 13, 2018

moneromooo-monero Jun 13, 2018

moneromooo-monero Jun 13, 2018

stoffu Jun 14, 2018

vtnerd Jun 14, 2018

moneromooo-monero commented Jul 19, 2018 •

edited

Loading

stoffu commented Jul 20, 2018 •

edited

Loading

moneromooo-monero commented Jul 20, 2018

stoffu commented Jul 20, 2018

moneromooo-monero commented Jul 20, 2018

iamsmooth Aug 8, 2018

moneromooo-monero Aug 8, 2018

iamsmooth Aug 9, 2018 •

edited

Loading

moneromooo-monero Aug 9, 2018

moneromooo-monero Aug 9, 2018 •

edited

Loading

iamsmooth Aug 10, 2018

stoffu commented Aug 9, 2018 •

edited

Loading

danrmiller commented Aug 10, 2018

add and use constant time 32 byte equality function #3999

add and use constant time 32 byte equality function #3999

Conversation

moneromooo-monero commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moneromooo-monero commented Jul 19, 2018 • edited Loading

stoffu commented Jul 20, 2018 • edited Loading

moneromooo-monero commented Jul 20, 2018

stoffu commented Jul 20, 2018

moneromooo-monero commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamsmooth Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moneromooo-monero Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu commented Aug 9, 2018 • edited Loading

danrmiller commented Aug 10, 2018

moneromooo-monero commented Jul 19, 2018 •

edited

Loading

stoffu commented Jul 20, 2018 •

edited

Loading

iamsmooth Aug 9, 2018 •

edited

Loading

moneromooo-monero Aug 9, 2018 •

edited

Loading

stoffu commented Aug 9, 2018 •

edited

Loading