4 ms·

There is quite some irony here - I was always taught to code by using standard libraries because they've been tested to destruction and back in ways I'd never b

There is quite some irony here - I was always taught to code by using standard libraries because they've been tested to destruction and back in ways I'd never be able to work out, and that rolling my own was destined for failure. As far as good practise goes, OpenSSH seems to tick all the right boxes. This article seems to criticise this, but the deeper meaning is that the standard C library is not designed with security in mind. There's no inherent problem with the approach taken (not like OpenSSL reinventing several wheels and causing things like Heartbleed). In fact, the article's conclusion that the OpenSSH team should have rolled their own memory management code rather than use tried, tested and approved libraries doesn't sound like a good recommendation to me.

HerrMonnezza 11y ago
> In fact, the article's conclusion that the OpenSSH team should have > rolled their own memory management code rather than use tried, > tested and approved libraries That's not the conclusion I draw from the article. I rather read it as warning that even innocent and quite common operations like realloc() or using buffered I/O can have security implications, so one need to be extra extra extra careful when writing code that handles sensitive data. In other words, security can be even harder than security professionals think.
- maxlybbert 11y ago
  I agree with you. And while I don't think there's any secure way to use realloc(), I would like to remind people that stdio does give you control over the internal buffer (setvbuf()), which you can potentially set to a buffer that can't get swapped to disk and which you can zero out at the appropriate time. You can also call fflush() to be sure that the data in stdio's buffer can be zeroed even before the file is closed.
- cnvogel 11y ago
  One alternative would be to sidestep this memory-reallocation-and-buffering problem altogether and just use the code that already exists to connect to an ssh-agent, even when only private keys from the local filesystem are used for authentication: Let ssh spawn an ephemeral ssh-agent just for the duration of the key-exchange and just for this single ssh instance. Kill it once authentication is complete.
DSMan195276 11y ago
> In fact, the article's conclusion that the OpenSSH team should have rolled their own memory management code rather than use tried, tested and approved libraries doesn't sound like a good recommendation to me. I agree with your general statements but disagree with your ending conclusion. Notice that they never mentioned `malloc` in that post, nor `read` and `write`, etc. If you're writing code that needs to be secure, then it's important to be able to recognize what features you can use, which you can't, and thus what pieces of the standard-library are usable. `malloc` is perfectly fine, because it will never touch the memory you put inside it - If you clear the memory with a `memset` before `free` (And make sure the `memset` isn't removed), then `malloc` and `free` will never be able to copy your memory or leave it laying around. `realloc` breaks this guarantee because it explicitly will make a copy if it has to find a new place to put the buffer, and thus it's not usable if you need to keep the above guarantees about copies of the data. `fread` and `fwrite` are obviously out for the same reason - There's no guarantee they'll clear their buffers. For such a situation you definitely want unbuffered IO, and the best way to do that is to just directly do `read` and `write`. You could write your own buffer layer on top of it (that is securely cleared), but it's not necessary to use buffers at all - Especially if the file is small enough that you could just read the entire thing into a single buffer anyway. (Note that it may be possible to get those guarantees through some `setvbuf` and similar calls - I'm not completely sure since I've never tried - It's not the defaults regardless). The big problem with the approach taken is that it doesn't take enough precautions when handling secure data, and uses functions that explicitly do things they don't want.

HerrMonnezza 11y ago

> In fact, the article's conclusion that the OpenSSH team should have > rolled their own memory management code rather than use tried, > tested and approved libraries That's not the conclusion I draw from the article. I rather read it as warning that even innocent and quite common operations like realloc() or using buffered I/O can have security implications, so one need to be extra extra extra careful when writing code that handles sensitive data. In other words, security can be even harder than security professionals think.

maxlybbert 11y ago
I agree with you. And while I don't think there's any secure way to use realloc(), I would like to remind people that stdio does give you control over the internal buffer (setvbuf()), which you can potentially set to a buffer that can't get swapped to disk and which you can zero out at the appropriate time. You can also call fflush() to be sure that the data in stdio's buffer can be zeroed even before the file is closed.
cnvogel 11y ago
One alternative would be to sidestep this memory-reallocation-and-buffering problem altogether and just use the code that already exists to connect to an ssh-agent, even when only private keys from the local filesystem are used for authentication: Let ssh spawn an ephemeral ssh-agent just for the duration of the key-exchange and just for this single ssh instance. Kill it once authentication is complete.

DSMan195276 11y ago

> In fact, the article's conclusion that the OpenSSH team should have rolled their own memory management code rather than use tried, tested and approved libraries doesn't sound like a good recommendation to me. I agree with your general statements but disagree with your ending conclusion. Notice that they never mentioned `malloc` in that post, nor `read` and `write`, etc. If you're writing code that needs to be secure, then it's important to be able to recognize what features you can use, which you can't, and thus what pieces of the standard-library are usable. `malloc` is perfectly fine, because it will never touch the memory you put inside it - If you clear the memory with a `memset` before `free` (And make sure the `memset` isn't removed), then `malloc` and `free` will never be able to copy your memory or leave it laying around. `realloc` breaks this guarantee because it explicitly will make a copy if it has to find a new place to put the buffer, and thus it's not usable if you need to keep the above guarantees about copies of the data. `fread` and `fwrite` are obviously out for the same reason - There's no guarantee they'll clear their buffers. For such a situation you definitely want unbuffered IO, and the best way to do that is to just directly do `read` and `write`. You could write your own buffer layer on top of it (that is securely cleared), but it's not necessary to use buffers at all - Especially if the file is small enough that you could just read the entire thing into a single buffer anyway. (Note that it may be possible to get those guarantees through some `setvbuf` and similar calls - I'm not completely sure since I've never tried - It's not the defaults regardless). The big problem with the approach taken is that it doesn't take enough precautions when handling secure data, and uses functions that explicitly do things they don't want.