5 ms·

> There is a function, ossl_punycode_decode that decodes Punycode. Punycode is a way to encode Unicode as ASCII, used to represent Unicode strings in the ASCII-

> There is a function, ossl_punycode_decode that decodes Punycode. Punycode is a way to encode Unicode as ASCII, used to represent Unicode strings in the ASCII-only world of DNS. ossl_punycode_decode takes an output buffer, and if the buffer runs out it keeps parsing and verifying the Punycode but discards the rest of the output. > I did not have the heart to figure out why it works like this. Maybe there's a good reason to do progressive parsing. Maybe it's an artifact of how C makes you do memory management or of OpenSSL's C style. Anyway. Not involved in OpenSSL, but this is a fairly common pattern in a lot of C APIs. You want to decode some data, but you're not sure how big the output is going to be ahead of time. You could write a separate function to calculate the length, but that function has to do most of the work of actual decoding to figure out that length. A lot of times the output is small enough it can fit in some conservatively sized buffer so you can save a fair bit of work by having a (potentially stack-allocated) buffer of some fixed size and then allocating a precisely sized buffer on the heap if it turns out to not be big enough. Further, having a separate length function means you typically end up with two similar but separate decode implementations which has its own problems. In most other languages, you have some sort of growable container in the standard library so you just avoid the problem entirely at the expense of having less control over memory allocations.

j-bos 4y ago
I am by no means a rust developer and asking in ignorance, would this have been a problem had it been written in rust?
- vore 4y ago
  Rust would take the growable container approach here, so it wouldn't be vulnerable in the same way. There's nothing in C that prevents you from doing this either, but as the parent post mentions it's common style to do this (see also things like snprintf).
  - Elinvynia 4y ago
    > There's nothing in C that prevents you from doing this either Yet these errors consistently happen in C projects of various sizes.
    - greysphere 4y ago
      Might be some survivorship bias.
    - vore 4y ago
      I am a Rust evangelist as much as the next person, but this is really a case of C developers preferring caller reallocation over callee reallocation, which as the root comment points out is fraught with danger. If the C implementation here used callee reallocation, while you do still have to be careful, the risk of this kind of error is greatly reduced (but at the cost of having to use dynamic memory, which might not be appropriate in all cases). Yes, Rust would eliminate this error, but you can still do it "safer" in C (but you have to give up certain things to do it that way).
    - bombolo 4y ago
      Because all the important and used stuff that gets looked at is written in C. At the moment rust is used for some very marginal and unimportant leaf projects.
- mike_d 4y ago
  It is an exercise in navel gazing until an organization steps up to fund: 1) a linkable clib harness that exports Rust functions to replace OpenSSL functions. and 2) pays to get the resulting harness and underlying code FIPS certified so people can use it.
  - account42 4y ago
    Most people have no problem using non-FIPS-certified code so that is not a precondition for the effort being useful.
    - mike_d 4y ago
      But the people who make OpenSSL the predominant implementation do care.
- Ygg2 4y ago
  Not in this case as far as I can tell.
- newpavlov 4y ago
  Rust code would probably return an error on output buffer overflow instead of siltently discarding data. Something like this: https://docs.rs/base16ct/latest/base16ct/mixed/fn.decode.html https://docs.rs/base16ct/latest/base16ct/mixed/fn.decode.htm...
- simcop2387 4y ago
  Probably not. It's definitely possible to write this function exactly as it is in openssl with this issue, nothing about Rust's safety claims prevent it. However, Rust doesn't have the same issues that lead to this kind of pattern being common like they are in C. Result<> types being so common, better handling of dynamic allocations, and at least for now a generally (it can't last forever) more security conscious community mean that other patterns will be chosen unstead. I'm not sure any language can actually prevent it from being possible outright, but you possibly pair it with some kind of formal proof system like using Coq and TLA+ might let you prove thag an issue like this doesn't exist in a specific implementation. However it's incredibly hard to do this.
- tptacek 4y ago
  In no other mainstream language other than C (and maybe non-idiomatic C++) would this have ever been a problem.
  - ollien 4y ago
    Yeah, most folks in C++ would just use an std::vector, right? I guess if you wanted to use the modern abstraction of std::array, it could be a problem, since using `operator[]` on a non-existent element produces UB.
  - tjoff 4y ago
    That is fair, though, I'd guess that no other mainstream language except for modern C++ and Rust would fit the bill of OpenSSL. Both of which are embarrassingly recent. It is not a failure of C that we couldn't improve on it for so many decades.
- cesarb 4y ago
  > would this have been a problem had it been written in rust? The answer would be "it depends on whether you consider denial-of-service a problem". The key detail which makes all the difference is that, unless you're playing with raw pointers (which can only be dereferenced in "unsafe" blocks), the pointer to a buffer slice is always kept together with its length (in a "fat pointer"). Attempting to write through it past the buffer's bounds will result in a Rust panic, which usually aborts the whole process (there are ways to abort just one thread, or even to treat it similarly to a C++ exception, but a library cannot depend on them since the program might be compiled in the panic=abort mode). While that's obviously better than allowing for remote code execution, it still could be considered an issue. Of course, that's assuming you want a similar API and are writing the code in a similar style, just replacing the direct pointer manipulation with slice manipulation. I don't know whether, in this particular case, more idiomatic Rust code would have avoided the issue. And, of course, the "growable container" approach would completely avoid it, but Rust is also used in places where memory allocation is not allowed, so having a non-allocating API still makes sense.
  - zaarn 4y ago
    It should be noted that you can simply use the fallible API's for slices in Rust, then you don't have a crash but can cleanly abort the operation and return an error.
    - cesarb 4y ago
      True, but it doesn't make much sense to use the fallible slice APIs when you know they will never fail (the only way they could fail would be if the code has a bug). It would just complicate not only the code within the function, but also the API to the function, which also becomes a fallible API (and this propagates outward, until it meets a caller which already was fallible for other reasons). And complicating the code unnecessarily increases the chance of logic bugs.
      - zaphar 4y ago
        It's actually preferrable in many of those "infallible" cases to panic rather than handling the error since it usually indicates that your application is in unknown territory and there is no "safe" way to handle it.
tptacek 4y ago
Even in C, you can write an abstraction for a growable buffer. The problem is, you have to make all the rest of the code work with it, rather than a char* or whatnot.
- wahern 4y ago
  BoringSSL and libressl added CBS (bytestring) and CBB (bytebuilder) interfaces in 2015. Converting everything over is a chore, but that's exactly what those projects have been methodically doing over the years. Unfortunately, I don't see those interfaces in OpenSSL 3.0, though maybe (hopefully) they're working toward something similar.
  - pjmlp 4y ago
    How many projects have to keep re-inventing those kind of data structures until WG14 takes security seriously?
- kevin_thibedeau 4y ago
  Then you have the new problem of restricting its growth and that adds a new failure mode to deal with. The C idiom works fine if you don't have implementation bugs.
  - tptacek 4y ago
    Everything has that failure mode, in every language.
    - kevin_thibedeau 4y ago
      Writing into a zero length output buffer does not fail and consumes no memory resources.
      - tedunangst 4y ago
        Who cares? You still have to write the code that handles failure when the buffer length isn't zero.
magicalhippo 4y ago
> Not involved in OpenSSL, but this is a fairly common pattern in a lot of C APIs. A better approach IMHO, typically found in the Win32 API for example, is to take an extra argument which receives the buffer size needed to hold the whole output, in addition to the size of the buffer you pass. This allows the caller to detect the condition and decide if it's an error or not if the required buffer size returned is greater than the size of the buffer, and potentially call it again with a sufficiently large buffer.
- fbdab103 4y ago
  Isn't that roughly how heartbleed works? Trusting the provided length?
  - MereInterest 4y ago
    If the caller is an external actor, yes, their provided length should not be trusted. However, this is not always the case. The caller may be another part of the same program, trusted not to perform malicious actions to the same extent that the rest of the program is trusted.
  - magicalhippo 4y ago
    Hearbleed was due to failure to sanitize external data[1], not sure how that's directly relevant to what I wrote. I mean sure if the caller fails to sanitize and as a result passes a size that's bigger than the actual buffer, the callee doesn't have much in the way of detecting that. But that's a general C issue, nothing specific to what I wrote. [1]: https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=96db9023b881d7cd9f379b0c154650d6c108e9a3 https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h...
- mikepavone 4y ago
  > A better approach IMHO, typically found in the Win32 API for example, is to take an extra argument which receives the buffer size needed to hold the whole output, in addition to the size of the buffer you pass. This is exactly what the function in question does and is what lead to the buffer overflow. Basically, this sort of API requires you to continue decoding (so you can calculate the length), but stop writing to the output buffer once the buffer is full. The function had a bug that lead it to keep writing to the buffer in some cases even after it was full.

j-bos 4y ago

I am by no means a rust developer and asking in ignorance, would this have been a problem had it been written in rust?

vore 4y ago
Rust would take the growable container approach here, so it wouldn't be vulnerable in the same way. There's nothing in C that prevents you from doing this either, but as the parent post mentions it's common style to do this (see also things like snprintf).
- Elinvynia 4y ago
  > There's nothing in C that prevents you from doing this either Yet these errors consistently happen in C projects of various sizes.
  - greysphere 4y ago
    Might be some survivorship bias.
  - vore 4y ago
    I am a Rust evangelist as much as the next person, but this is really a case of C developers preferring caller reallocation over callee reallocation, which as the root comment points out is fraught with danger. If the C implementation here used callee reallocation, while you do still have to be careful, the risk of this kind of error is greatly reduced (but at the cost of having to use dynamic memory, which might not be appropriate in all cases). Yes, Rust would eliminate this error, but you can still do it "safer" in C (but you have to give up certain things to do it that way).
  - bombolo 4y ago
    Because all the important and used stuff that gets looked at is written in C. At the moment rust is used for some very marginal and unimportant leaf projects.
mike_d 4y ago
It is an exercise in navel gazing until an organization steps up to fund: 1) a linkable clib harness that exports Rust functions to replace OpenSSL functions. and 2) pays to get the resulting harness and underlying code FIPS certified so people can use it.
- account42 4y ago
  Most people have no problem using non-FIPS-certified code so that is not a precondition for the effort being useful.
  - mike_d 4y ago
    But the people who make OpenSSL the predominant implementation do care.
Ygg2 4y ago
Not in this case as far as I can tell.
newpavlov 4y ago
Rust code would probably return an error on output buffer overflow instead of siltently discarding data. Something like this: https://docs.rs/base16ct/latest/base16ct/mixed/fn.decode.html https://docs.rs/base16ct/latest/base16ct/mixed/fn.decode.htm...
simcop2387 4y ago
Probably not. It's definitely possible to write this function exactly as it is in openssl with this issue, nothing about Rust's safety claims prevent it. However, Rust doesn't have the same issues that lead to this kind of pattern being common like they are in C. Result<> types being so common, better handling of dynamic allocations, and at least for now a generally (it can't last forever) more security conscious community mean that other patterns will be chosen unstead. I'm not sure any language can actually prevent it from being possible outright, but you possibly pair it with some kind of formal proof system like using Coq and TLA+ might let you prove thag an issue like this doesn't exist in a specific implementation. However it's incredibly hard to do this.
tptacek 4y ago
In no other mainstream language other than C (and maybe non-idiomatic C++) would this have ever been a problem.
- ollien 4y ago
  Yeah, most folks in C++ would just use an std::vector, right? I guess if you wanted to use the modern abstraction of std::array, it could be a problem, since using `operator[]` on a non-existent element produces UB.
- tjoff 4y ago
  That is fair, though, I'd guess that no other mainstream language except for modern C++ and Rust would fit the bill of OpenSSL. Both of which are embarrassingly recent. It is not a failure of C that we couldn't improve on it for so many decades.
cesarb 4y ago
> would this have been a problem had it been written in rust? The answer would be "it depends on whether you consider denial-of-service a problem". The key detail which makes all the difference is that, unless you're playing with raw pointers (which can only be dereferenced in "unsafe" blocks), the pointer to a buffer slice is always kept together with its length (in a "fat pointer"). Attempting to write through it past the buffer's bounds will result in a Rust panic, which usually aborts the whole process (there are ways to abort just one thread, or even to treat it similarly to a C++ exception, but a library cannot depend on them since the program might be compiled in the panic=abort mode). While that's obviously better than allowing for remote code execution, it still could be considered an issue. Of course, that's assuming you want a similar API and are writing the code in a similar style, just replacing the direct pointer manipulation with slice manipulation. I don't know whether, in this particular case, more idiomatic Rust code would have avoided the issue. And, of course, the "growable container" approach would completely avoid it, but Rust is also used in places where memory allocation is not allowed, so having a non-allocating API still makes sense.
- zaarn 4y ago
  It should be noted that you can simply use the fallible API's for slices in Rust, then you don't have a crash but can cleanly abort the operation and return an error.
  - cesarb 4y ago
    True, but it doesn't make much sense to use the fallible slice APIs when you know they will never fail (the only way they could fail would be if the code has a bug). It would just complicate not only the code within the function, but also the API to the function, which also becomes a fallible API (and this propagates outward, until it meets a caller which already was fallible for other reasons). And complicating the code unnecessarily increases the chance of logic bugs.
    - zaphar 4y ago
      It's actually preferrable in many of those "infallible" cases to panic rather than handling the error since it usually indicates that your application is in unknown territory and there is no "safe" way to handle it.

tptacek 4y ago

Even in C, you can write an abstraction for a growable buffer. The problem is, you have to make all the rest of the code work with it, rather than a char* or whatnot.

wahern 4y ago
BoringSSL and libressl added CBS (bytestring) and CBB (bytebuilder) interfaces in 2015. Converting everything over is a chore, but that's exactly what those projects have been methodically doing over the years. Unfortunately, I don't see those interfaces in OpenSSL 3.0, though maybe (hopefully) they're working toward something similar.
- pjmlp 4y ago
  How many projects have to keep re-inventing those kind of data structures until WG14 takes security seriously?
kevin_thibedeau 4y ago
Then you have the new problem of restricting its growth and that adds a new failure mode to deal with. The C idiom works fine if you don't have implementation bugs.
- tptacek 4y ago
  Everything has that failure mode, in every language.
  - kevin_thibedeau 4y ago
    Writing into a zero length output buffer does not fail and consumes no memory resources.
    - tedunangst 4y ago
      Who cares? You still have to write the code that handles failure when the buffer length isn't zero.

magicalhippo 4y ago

> Not involved in OpenSSL, but this is a fairly common pattern in a lot of C APIs. A better approach IMHO, typically found in the Win32 API for example, is to take an extra argument which receives the buffer size needed to hold the whole output, in addition to the size of the buffer you pass. This allows the caller to detect the condition and decide if it's an error or not if the required buffer size returned is greater than the size of the buffer, and potentially call it again with a sufficiently large buffer.

fbdab103 4y ago
Isn't that roughly how heartbleed works? Trusting the provided length?
- MereInterest 4y ago
  If the caller is an external actor, yes, their provided length should not be trusted. However, this is not always the case. The caller may be another part of the same program, trusted not to perform malicious actions to the same extent that the rest of the program is trusted.
- magicalhippo 4y ago
  Hearbleed was due to failure to sanitize external data[1], not sure how that's directly relevant to what I wrote. I mean sure if the caller fails to sanitize and as a result passes a size that's bigger than the actual buffer, the callee doesn't have much in the way of detecting that. But that's a general C issue, nothing specific to what I wrote. [1]: https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=96db9023b881d7cd9f379b0c154650d6c108e9a3 https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h...
mikepavone 4y ago
> A better approach IMHO, typically found in the Win32 API for example, is to take an extra argument which receives the buffer size needed to hold the whole output, in addition to the size of the buffer you pass. This is exactly what the function in question does and is what lead to the buffer overflow. Basically, this sort of API requires you to continue decoding (so you can calculate the length), but stop writing to the output buffer once the buffer is full. The function had a bug that lead it to keep writing to the buffer in some cases even after it was full.