17 ms·

From the diff: - newkey = xmalloc(sizeof(newkey)); + newkey = xcalloc(1, sizeof(newkey)); The only change is that the allocated memory is zeroed, right?

From the diff: - newkey = xmalloc(sizeof(*newkey)); + newkey = xcalloc(1, sizeof(*newkey)); The only change is that the allocated memory is zeroed, right? (Just wondering if I'm missing something.)

djmdjm 13y ago
Nope. That's all. The newkey struct in turn contains a bunch of OpenSSL context structures, and one of these includes a cleanup callback pointer for the MAC (message authentication code) in use. For most ciphers this was being initialised later but AES-GCM provides message integrity without the need for an external MAC and in this case the MAC context was being left uninitialised. When the newkeys struct was later being cleaned up as part of a rekeying operation, the cleanup callback was called. It's address was whatever happened to be on the heap when this allocation occurred. It certainly appears exploitable using standard techniques, though it may be complicated somewhat by OpenSSH's privilege separation architecture.
- mnarayan01 13y ago
  Thanks. Is calloc(1, size) considered idiomatic for zeroing memory? While I haven't used C much lately, back when I did I don't think I realized this, and could see myself at least considering reverting the change during "code cleanup".
  - coldpie 13y ago
    Yeah, it's fairly common. calloc zeroes, malloc doesn't, and you shouldn't make a significant change like that during code cleanup. I think it's somewhat more common to see malloc followed by memset, as the intent is way more clear to a reader than just calloc by itself.
    - tedunangst 13y ago
      calloc can be faster because it has internal knowledge of whether the memory is already zero.
- hrjet 13y ago
  Hmm.. would it make sense to replace all calls of xmalloc with xcalloc as a preventive measure?
  - jzwinck 13y ago
    I don't think it is advisable to blindly replace like that in a widely used, audited and tested system. You need to think about more than just the bits in memory, for example timing attacks. And who knows, your "fix" could even expose some other latent bug. It's not clear cut.
    - ScottBurson 13y ago
      I don't get this. How could leaving memory uninitialized be better than zeroing it? Using calloc could be redundant in places, in that the allocated object might get initialized some other way right after allocation. But I don't see how it could ever be wrong. (There might have been zeros in that memory already anyway.) There are languages that have no way to allocate uninitialized memory -- are you going to say they aren't appropriate for crypto algorithms? And besides, exposing a latent bug is a good thing, no?
      - geofft 13y ago
        There are a couple of concrete worries I'd have: * If you malloc something very large, on Linux at least (I don't know about the BSDs), it doesn't necessarily get allocated, and only requests memory from the system when you actually access it. It's a valid pattern to, say, malloc a large amount of memory, pass it to your own allocator as a pool, and use what you need from it. If you go and zero all that memory, then you've accessed it all, resulting in much increased memory usage and maybe OOM kills. (You can argue this is an issue with the Linux overcommit system, not with malloc, but that's the system we have today and we'd need to account for it.) * Slow code is a deterrent for people deploying crypto. If it takes 5 milliseconds for an HTTP response and 50 for an HTTPS handshake, people are going to be hesitant about deploying HTTPS, or do something silly like use it to protect only the initial login. So you do need to worry about the time that your crypto takes as an operational concern for deployment, and balance that off against hardening and paranoia. Favoring either extreme too much results in less overall security. * There are very few languages that are truly "appropriate" for crypto algorithms. Even C only kind of counts, and you have to be very careful about how you write it. You ideally need promises from the language that certain operations are constant-time and that the optimizer is going to do what you want and neither more nor less. Most of the languages I can think of that only allocate you zeroed memory also include risks on the order of garbage collection being triggered in the middle of your algorithm depending on what your secret data is, so that makes them definitely unsuitable. And most of the non-toy crypto libraries in higher-level languages end up being bindings of carefully-tuned C libraries, and in that sense _do_ quasi-unintentionally expose a way to allocate uninitialized memory, by going through the C allocation routines.
zokier 13y ago
Yes. I believe the bug rises from some code like: if (newkey->callback != NULL) { newkey->callback() } If newkey structure was not zeroed, then the callback would have had some contents from previous use and the NULL check would have passed and some unintended memory position would be called.
- revelation 13y ago
  Why didn't that crash? I understand the cleanup routine is always called and certainly someone tested the AES-GCM provider by just running it once.
  - alexkus 13y ago
    Probably because the memory allocated just happened to contain only NUL bytes where that specific cleanup function pointer is stored whenever it was run in testing. The point is that a targeted malicious attack would try and ensure that those bytes were not NUL and that it ended up calling malicious injected code on the target server.
    - makomk 13y ago
      Yeah, I've found a few bugs like this myself where the memory in question happened to be NULL, until some unrelated change elsewhere in the toolchain meant that it wasn't anymore.
- kzrdude 13y ago
  The C standard doesn't require such code to be correct, even with calloc, because the NULL pointer is not necessarily represented by the zero bitpattern. While it will work "everywhere", calloc + NULL check is not semantically correct C.
  - deleted 13y ago
    [deleted]
  - deleted 13y ago
    [deleted]
marc_omorain 13y ago
It seems odd to see a commit in this day and age with no unit test. When you make a new newkey struct the clean-up callback should be NULL.
- noselasd 13y ago
  That's the fun with C, it would be quite unlikely the malloc call in a unit test for this didn't happen to zero initialize the struct.
  - meowface 13y ago
    Yep. You may get different results based on the code path taken, the compiler, and (maybe?) the OS. Most of the time the value at that memory location will probably be a zero byte (or a zero word/dword), but sometimes it may not.
- djmdjm 13y ago
  We committed a regression test fix separately.

From the diff: - newkey = xmalloc(sizeof(*newkey)); + newkey = xcalloc(1, sizeof(*newkey)); The only change is that the allocated memory is zeroed, right?

From the diff: - newkey = xmalloc(sizeof(*newkey)); + newkey = xcalloc(1, sizeof(*newkey)); The only change is that the allocated memory is zeroed, right? (Just wondering if I'm missing something.)

djmdjm 13y ago

Nope. That's all. The newkey struct in turn contains a bunch of OpenSSL context structures, and one of these includes a cleanup callback pointer for the MAC (message authentication code) in use. For most ciphers this was being initialised later but AES-GCM provides message integrity without the need for an external MAC and in this case the MAC context was being left uninitialised. When the newkeys struct was later being cleaned up as part of a rekeying operation, the cleanup callback was called. It's address was whatever happened to be on the heap when this allocation occurred. It certainly appears exploitable using standard techniques, though it may be complicated somewhat by OpenSSH's privilege separation architecture.

mnarayan01 13y ago
Thanks. Is calloc(1, size) considered idiomatic for zeroing memory? While I haven't used C much lately, back when I did I don't think I realized this, and could see myself at least considering reverting the change during "code cleanup".
- coldpie 13y ago
  Yeah, it's fairly common. calloc zeroes, malloc doesn't, and you shouldn't make a significant change like that during code cleanup. I think it's somewhat more common to see malloc followed by memset, as the intent is way more clear to a reader than just calloc by itself.
  - tedunangst 13y ago
    calloc can be faster because it has internal knowledge of whether the memory is already zero.
hrjet 13y ago
Hmm.. would it make sense to replace all calls of xmalloc with xcalloc as a preventive measure?
- jzwinck 13y ago
  I don't think it is advisable to blindly replace like that in a widely used, audited and tested system. You need to think about more than just the bits in memory, for example timing attacks. And who knows, your "fix" could even expose some other latent bug. It's not clear cut.
  - ScottBurson 13y ago
    I don't get this. How could leaving memory uninitialized be better than zeroing it? Using calloc could be redundant in places, in that the allocated object might get initialized some other way right after allocation. But I don't see how it could ever be wrong. (There might have been zeros in that memory already anyway.) There are languages that have no way to allocate uninitialized memory -- are you going to say they aren't appropriate for crypto algorithms? And besides, exposing a latent bug is a good thing, no?
    - geofft 13y ago
      There are a couple of concrete worries I'd have: * If you malloc something very large, on Linux at least (I don't know about the BSDs), it doesn't necessarily get allocated, and only requests memory from the system when you actually access it. It's a valid pattern to, say, malloc a large amount of memory, pass it to your own allocator as a pool, and use what you need from it. If you go and zero all that memory, then you've accessed it all, resulting in much increased memory usage and maybe OOM kills. (You can argue this is an issue with the Linux overcommit system, not with malloc, but that's the system we have today and we'd need to account for it.) * Slow code is a deterrent for people deploying crypto. If it takes 5 milliseconds for an HTTP response and 50 for an HTTPS handshake, people are going to be hesitant about deploying HTTPS, or do something silly like use it to protect only the initial login. So you do need to worry about the time that your crypto takes as an operational concern for deployment, and balance that off against hardening and paranoia. Favoring either extreme too much results in less overall security. * There are very few languages that are truly "appropriate" for crypto algorithms. Even C only kind of counts, and you have to be very careful about how you write it. You ideally need promises from the language that certain operations are constant-time and that the optimizer is going to do what you want and neither more nor less. Most of the languages I can think of that only allocate you zeroed memory also include risks on the order of garbage collection being triggered in the middle of your algorithm depending on what your secret data is, so that makes them definitely unsuitable. And most of the non-toy crypto libraries in higher-level languages end up being bindings of carefully-tuned C libraries, and in that sense _do_ quasi-unintentionally expose a way to allocate uninitialized memory, by going through the C allocation routines.

zokier 13y ago

Yes. I believe the bug rises from some code like: if (newkey->callback != NULL) { newkey->callback() } If newkey structure was not zeroed, then the callback would have had some contents from previous use and the NULL check would have passed and some unintended memory position would be called.

revelation 13y ago
Why didn't that crash? I understand the cleanup routine is always called and certainly someone tested the AES-GCM provider by just running it once.
- alexkus 13y ago
  Probably because the memory allocated just happened to contain only NUL bytes where that specific cleanup function pointer is stored whenever it was run in testing. The point is that a targeted malicious attack would try and ensure that those bytes were not NUL and that it ended up calling malicious injected code on the target server.
  - makomk 13y ago
    Yeah, I've found a few bugs like this myself where the memory in question happened to be NULL, until some unrelated change elsewhere in the toolchain meant that it wasn't anymore.
kzrdude 13y ago
The C standard doesn't require such code to be correct, even with calloc, because the NULL pointer is not necessarily represented by the zero bitpattern. While it will work "everywhere", calloc + NULL check is not semantically correct C.
- deleted 13y ago
  [deleted]
- deleted 13y ago
  [deleted]

marc_omorain 13y ago

It seems odd to see a commit in this day and age with no unit test. When you make a new newkey struct the clean-up callback should be NULL.

noselasd 13y ago
That's the fun with C, it would be quite unlikely the malloc call in a unit test for this didn't happen to zero initialize the struct.
- meowface 13y ago
  Yep. You may get different results based on the code path taken, the compiler, and (maybe?) the OS. Most of the time the value at that memory location will probably be a zero byte (or a zero word/dword), but sometimes it may not.
djmdjm 13y ago
We committed a regression test fix separately.