View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0006152 | GNUnet | util library | public | 2020-04-03 10:01 | 2020-07-09 07:17 |
| Reporter | fefe | Assigned To | Christian Grothoff | Due Date | |
| Priority | normal | Severity | trivial | Reproducibility | N/A |
| Status | closed | Resolution | fixed | ||
| Product Version | Git master | ||||
| Target Version | 0.13.0 | Fixed in Version | 0.13.0 | ||
| Summary | 0006152: Using GNUNET_memcmp for comparing public keys | ||||
| Description | The 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 Reproduce | 406 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. | ||||
| Tags | No tags attached. | ||||
| 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? | |
| Oh, and of course your point (1) is very well taken (array cmp). We may want to more explicitly document that limitation. | |
| 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(). | |
| 0.13.0 released | |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2020-04-03 10:01 | fefe | New Issue | |
| 2020-04-03 10:01 | fefe | Status | new => assigned |
| 2020-04-03 10:01 | fefe | Assigned To | => Christian Grothoff |
| 2020-04-03 11:22 | Christian Grothoff | Note Added: 0015498 | |
| 2020-04-03 11:24 | Christian Grothoff | Note Added: 0015499 | |
| 2020-04-03 12:25 | Christian Grothoff | Project | Taler => GNUnet |
| 2020-04-03 12:25 | Christian Grothoff | Category | exchange => General |
| 2020-04-03 12:28 | Christian Grothoff | Category | General => util library |
| 2020-04-03 12:28 | Christian Grothoff | Product Version | 0.7.0 => Git master |
| 2020-04-03 13:24 | Christian Grothoff | Note Added: 0015506 | |
| 2020-04-03 13:25 | Christian Grothoff | Fixed in Version | => 0.12.2 |
| 2020-04-03 13:25 | Christian Grothoff | Target Version | => 0.12.2 |
| 2020-04-03 13:25 | Christian Grothoff | Status | assigned => resolved |
| 2020-04-03 13:25 | Christian Grothoff | Resolution | open => fixed |
| 2020-04-23 08:45 | schanzen | Fixed in Version | 0.12.2 => 0.13.0 |
| 2020-04-23 08:47 | schanzen | Target Version | 0.12.2 => 0.13.0 |
| 2020-05-31 22:49 | Issue cloned: 0006321 | ||
| 2020-05-31 22:52 | Issue cloned: 0006353 | ||
| 2020-07-09 07:17 | schanzen | Note Added: 0016435 | |
| 2020-07-09 07:17 | schanzen | Status | resolved => closed |