View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0006183 | GNUnet | util library | public | 2020-04-15 13:34 | 2020-07-09 09:17 |
| Reporter | fefe | Assigned To | Christian Grothoff | ||
| Priority | high | Severity | tweak | Reproducibility | N/A |
| Status | closed | Resolution | fixed | ||
| Product Version | 0.12.1 | ||||
| Target Version | 0.13.0 | Fixed in Version | 0.13.0 | ||
| Summary | 0006183: buffer overflow in GNUNET_CRYPTO_hkdf_v | ||||
| Description | The function takes as first two arguments a destination buffer (result) and its length (out_len). It also takes prf_algo and then sets 154 unsigned int k = gcry_md_get_algo_dlen (prf_algo); The arithmetic in this function has several potential numeric overflows that could wreak havoc, for example we are just adding up ctx_len from size_t varargs. Then we create this variable length array on the stack: 195 char plain[plain_len]; That is generally speaking a bad idea. gcc will basically move the stack pointer here. It will not check for overflow, and it will not touch all the memory pages on the way. So if plain_len is large enough, gcc might skip the guard page at the end of the stack and the next stack write will overwrite arbitrary memory. The general advice is: do not use VLAs in gcc unless you are very sure the value is bounded and small. Here is however the actual problem this bug report is about: 220 GNUNET_memcpy (result, hc, k); We have no reason to believe that k <= out_len at this point. This is a classical buffer overflow bug. To trigger any of these bugs, the caller must be passing in either an unreasonably small output buffer or unreasonably large input buffers. I wonder why we have to make a plaintext copy of all the data to begin with. | ||||
| Tags | No tags attached. | ||||
|
|
The code also contains a bunch of not nice memset(a,x,1) calls that would be nicer as plain assignments... |
|
|
b7be5b9f5..839badf7c removes the wtf-memset()s 839badf7c..c894cf82d moves the unbounded plain[] onto the heap and adds integer overflow guards on the arguments. (You are right that one could/should probably avoid memcpy()ing everything into the 'plain' array in the first place, but that's a larger change, not for tonight.) |
|
|
As for the buffer overflow, I do not think it is real. The code DOES check: Note that *both* GNUNET_memcpy(result, hc, k) invocations depend on "if (t > 0)" and "i < t" (aka also t > 0). Now what is t? Well, we find that just above: t = out_len / k. So t is how many times we must 'expand' the hash function to get to out_len (and d = out_len % k is the remainder). Thus, the memcpy() is strictly guarded by out_len being big enough! |
|
|
0.13.0 released |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2020-04-15 13:34 | fefe | New Issue | |
| 2020-04-15 13:58 | Christian Grothoff | Note Added: 0015655 | |
| 2020-04-15 14:03 | Christian Grothoff | Priority | normal => high |
| 2020-04-15 14:03 | Christian Grothoff | Severity | minor => major |
| 2020-04-15 14:03 | Christian Grothoff | Reproducibility | have not tried => N/A |
| 2020-04-15 14:03 | Christian Grothoff | Target Version | => 0.12.2 |
| 2020-04-15 20:41 | Christian Grothoff | Note Added: 0015658 | |
| 2020-04-15 20:53 | Christian Grothoff | Note Added: 0015659 | |
| 2020-04-15 20:53 | Christian Grothoff | Severity | major => tweak |
| 2020-04-15 20:53 | Christian Grothoff | Assigned To | => Christian Grothoff |
| 2020-04-15 20:53 | Christian Grothoff | Status | new => resolved |
| 2020-04-15 20:53 | Christian Grothoff | Resolution | open => fixed |
| 2020-04-15 20:53 | Christian Grothoff | Fixed in Version | => 0.12.2 |
| 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 |
| 2020-06-01 00:49 |
|
Issue cloned: 0006315 | |
| 2020-06-01 00:52 |
|
Issue cloned: 0006347 | |
| 2020-07-09 09:17 | schanzen | Note Added: 0016433 | |
| 2020-07-09 09:17 | schanzen | Status | resolved => closed |
