View Issue Details

IDProjectCategoryView StatusLast Update
0006183GNUnetutil librarypublic2020-07-09 09:17
Reporterfefe Assigned ToChristian Grothoff  
PriorityhighSeveritytweakReproducibilityN/A
Status closedResolutionfixed 
Product Version0.12.1 
Target Version0.13.0Fixed in Version0.13.0 
Summary0006183: buffer overflow in GNUNET_CRYPTO_hkdf_v
DescriptionThe 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.
TagsNo tags attached.

Activities

Christian Grothoff

2020-04-15 13:58

manager   ~0015655

The code also contains a bunch of not nice memset(a,x,1) calls that would be nicer as plain assignments...

Christian Grothoff

2020-04-15 20:41

manager   ~0015658

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.)

Christian Grothoff

2020-04-15 20:53

manager   ~0015659

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!

schanzen

2020-07-09 09:17

manager   ~0016433

0.13.0 released

Issue History

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 Adminknox Issue cloned: 0006315
2020-06-01 00:52 Adminknox Issue cloned: 0006347
2020-07-09 09:17 schanzen Note Added: 0016433
2020-07-09 09:17 schanzen Status resolved => closed