View Issue Details

IDProjectCategoryView StatusLast Update
0006152GNUnetutil librarypublic2020-04-23 10:47
ReporterfefeAssigned ToChristian Grothoff 
PrioritynormalSeveritytrivialReproducibilityN/A
Status resolvedResolutionfixed 
Product VersionSVN HEAD 
Target Version0.13.0Fixed in Version0.13.0 
Summary0006152: Using GNUNET_memcmp for comparing public keys
DescriptionThe exchange code uses GNUNET_memcmp, which is a dangerous API because the size is implicit.
GNUNET_memcmp looks like this:

1093 #define GNUNET_memcmp(a, b) \
1094 ({ \
1095 const typeof (*b) * _a = (a); \
1096 const typeof (*a) * _b = (b); \
1097 memcmp (_a, _b, sizeof(*a)); \
1098 })

This carries several risks.
1. if a is a pointer to an array, only the first element is compared.
2. the first two lines are apparently meant to assert that a and b are pointers to the same type, but in C pointers to different types are assignable. In some cases you'd get a compiler warning, but not in all.
3. If we are comparing crypto material, we might be introducing a timing side channel because memcmp will abort after the first unmatched byte.
Steps To Reproduce406 static void
407 test_master_present (void *cls,
408 const struct TALER_MasterPublicKeyP *mpub,
409 const char *exchange_url)
410 {
411 int *found = cls;
412
413 (void) exchange_url;
414 if (0 == GNUNET_memcmp (mpub,
415 &TALER_ARL_master_pub))
416 *found = GNUNET_YES;
417 }

This concrete invocation looks OK because both arguments are pointers to the same struct type, and it looks like the timing side channel would not be an issue.

However, I would recommend having a memcmp variant without timing side channel and always using it instead of memcmp if the compared data even faintly smells like crypto.
TagsNo tags attached.

Activities

Christian Grothoff

2020-04-03 13:22

manager   ~0015498

GNUNET_memcmp() is our replacement for memcmp(). I think it's safer, because it'll (at least) warn you if the types are not the same. We *did* have problems with code doing things like memcmp(a,b,sizeof(*a)) before where a and b are not of the same type, which IMO is much worse. The GNUnet-wrappers around libc are expected to _improve_ upon libc in terms of removing bug-potential -- but of course using GNUNET_memcmp() isn't expected to discharge the programmer of all responsibility! It is just to generate warnings on "obvious" bugs. So calling the API 'dangerous' (like gets()) feels wrong. What API would you propose instead?

I agree about the *existence* of the timing side-channel. However, I have never considered that timing attacks on comparing public keys for equality could be an issue. My _assumption_ has been that timing side-channels matter ONLY inside of code that deals with *private* key material. Could you please point me to any attack exploiting a side channel on comparing public keys or hash codes?

Christian Grothoff

2020-04-03 13:24

manager   ~0015499

Oh, and of course your point (1) is very well taken (array cmp). We may want to more explicitly document that limitation.

Christian Grothoff

2020-04-03 15:24

manager   ~0015506

4e259dbbb..0541fd194 adds GNUNET_memcmp_priv() which is suitable for comparing 'private' inputs (aka constant-time). I dare to close this bug, please DO file bugs for specific instances where you believe we actually are comparing private/sensitive inputs and should thus change GNUNET_memcmp() to GNUNET_memcmp_priv().

Issue History

Date Modified Username Field Change
2020-04-03 12:01 fefe New Issue
2020-04-03 12:01 fefe Status new => assigned
2020-04-03 12:01 fefe Assigned To => Christian Grothoff
2020-04-03 13:22 Christian Grothoff Note Added: 0015498
2020-04-03 13:24 Christian Grothoff Note Added: 0015499
2020-04-03 14:25 Christian Grothoff Project Taler => GNUnet
2020-04-03 14:25 Christian Grothoff Category exchange => General
2020-04-03 14:28 Christian Grothoff Category General => util library
2020-04-03 14:28 Christian Grothoff Product Version 0.7.0 => SVN HEAD
2020-04-03 15:24 Christian Grothoff Note Added: 0015506
2020-04-03 15:25 Christian Grothoff Fixed in Version => 0.12.2
2020-04-03 15:25 Christian Grothoff Target Version => 0.12.2
2020-04-03 15:25 Christian Grothoff Status assigned => resolved
2020-04-03 15:25 Christian Grothoff Resolution open => fixed
2020-04-23 10:45 schanzen Fixed in Version 0.12.2 => 0.13.0
2020-04-23 10:47 schanzen Target Version 0.12.2 => 0.13.0