4 ms·

Here's the referenced commit for the interested: https://github.com/libressl-portable/openbsd/commit/ddd98f8ea741a122952185a36c1396c14c2fda74#diff-027facc0b7c35

Here's the referenced commit for the interested: https://github.com/libressl-portable/openbsd/commit/ddd98f8ea741a122952185a36c1396c14c2fda74#diff-027facc0b7c35aa46b0e8fa7b467f1c4 https://github.com/libressl-portable/openbsd/commit/ddd98f8e... To be honest I'm kinda surprised that even after the 'goto fail' story people still write code in this questionable style(I know this particular issue is not stemming from the lack of curly braces, but still).

UnoriginalGuy 9y ago
This is C. There's nothing questionable about this "style." It is just how C is written. How else are you going to free resources without a code section to do it? Copy/paste it dozens of times? The specific code isn't "goto fail" anyway, it is more akin to a finally block.
- erdeszt 9y ago
  I was referring to the lack of curly braces around the body of the then clause. This code has nothing to do with resource management and gotos per se so I don't see where your frustration and attack comes from.
  - avar 9y ago
    If you compile your code with -Werror -Wmisleading-indentation it's no more dangerous than writing Python.
- gpvos 9y ago
  I think erdeszt is just referring to the lack of curly braces around the statements governed by the if statements. This can be done around any single-line statement block, and is generally safer in the face of merges from version control.
cryptarch 9y ago
I feel like having "safety net" sections is a smell, too. Maybe it's due to limitations of the language? I'm not a C expert. Safety nets make it seem like you're handling edge cases in a vaguely specified, ad-hoc way, which is prone to forgetting to add the safety net in at least some places, while the nets themselves are easy to mess up as well. Could this code benefit from more typing perhaps? Automated checking of pre- and postconditions? Are there C (macro) libraries implement that in a usable way?
- kroeckx 9y ago
  I think there is a misunderstand of what the existing safety net is about. There are 2 error states: did the verification fail and should the connection be aborted. The safety net makes sure that if a function (the callback) says the connection must be aborted but didn't set the verification error, that it sets an unknown verification error. Note that the callback is external code. The new "safety net" that libressl added said that if the connection doesn't need to be aborted there was no verification error.
- jeltz 9y ago
  Yeah, the real smell here is the "safety net" which is also what seems to have caused the bug. Clean quality code should avoid this kind of safety nets as much as possible and instead make it hard in the first place to get the program into an invalid state and if we do get into an invalid state not just silently try to guess what the user really wanted to do. As it turns out people actually wanted to return success with an error code and relied on this being possible in their applications. A better type system would help out a lot here, but it is also possible to write clean C code without this class of bugs. And OpenSSL has some of the worst code I have seen in an open source project (I have seen much worse in commercial projects), so while C has a lot of flaws do not judge it after OpenSSL.
- dom0 9y ago
  "safety net" cases are pretty much covered by design-by-contract 101.
PhantomGremlin 9y ago
surprised that even after the 'goto fail' story people still write code in this questionable style LibreSSL didn't spring into existence out of whole cloth. It started as a fork of OpenSSL, which goes back to 1998. The "questionable style" is from legacy code. It would be a massive effort to revise the entire codebase. And if LibreSSL did that, it would make it harder to import changes from OpenSSL and from other forks such as BoringSSL.
- technion 9y ago
  The code style in question is the current recommendation in the systemd style guide - a much more current project, and a rule that was put in place in 2014. https://github.com/systemd/systemd/commit/61f33134fc9231e07e1b9519b140d68139e9fad0#diff-cae545c4578eed1a167d69fb6d3b806bR82 https://github.com/systemd/systemd/commit/61f33134fc9231e07e... I'm all for blaming legacy but this is unfortunately still a trend.
- kbenson 9y ago
  > it would make it harder to import changes from OpenSSL When it's a choice between making it easier to import changes or harder to import bugs, I know which one I think is more important when dealing with a security library.
  - askmike 9y ago
    I don't think you see the amount of work that would go into refactoring a project of this scale whilst keeping up with the latest patches from upstream.
    - kbenson 9y ago
      I do understand the the amount of work, but that wasn't the only argument presented. It was also presented as making it harder to accept patches. Since one of the goals of LibreSSL is to start applying security best practices, and one of the reasons for its existence is the poor quality and recurrent problems with OpenSSL, keeping compatibility with OpenSSL to make it easier to accept patches should be very low on the list of priorities. Put another way, if you forked because upstream was crap, not changing because it makes it easier to accept upstream patches is a poor reason not to change something that might benefit from it.

UnoriginalGuy 9y ago

This is C. There's nothing questionable about this "style." It is just how C is written. How else are you going to free resources without a code section to do it? Copy/paste it dozens of times? The specific code isn't "goto fail" anyway, it is more akin to a finally block.

erdeszt 9y ago
I was referring to the lack of curly braces around the body of the then clause. This code has nothing to do with resource management and gotos per se so I don't see where your frustration and attack comes from.
- avar 9y ago
  If you compile your code with -Werror -Wmisleading-indentation it's no more dangerous than writing Python.
gpvos 9y ago
I think erdeszt is just referring to the lack of curly braces around the statements governed by the if statements. This can be done around any single-line statement block, and is generally safer in the face of merges from version control.

cryptarch 9y ago

I feel like having "safety net" sections is a smell, too. Maybe it's due to limitations of the language? I'm not a C expert. Safety nets make it seem like you're handling edge cases in a vaguely specified, ad-hoc way, which is prone to forgetting to add the safety net in at least some places, while the nets themselves are easy to mess up as well. Could this code benefit from more typing perhaps? Automated checking of pre- and postconditions? Are there C (macro) libraries implement that in a usable way?

kroeckx 9y ago
I think there is a misunderstand of what the existing safety net is about. There are 2 error states: did the verification fail and should the connection be aborted. The safety net makes sure that if a function (the callback) says the connection must be aborted but didn't set the verification error, that it sets an unknown verification error. Note that the callback is external code. The new "safety net" that libressl added said that if the connection doesn't need to be aborted there was no verification error.
jeltz 9y ago
Yeah, the real smell here is the "safety net" which is also what seems to have caused the bug. Clean quality code should avoid this kind of safety nets as much as possible and instead make it hard in the first place to get the program into an invalid state and if we do get into an invalid state not just silently try to guess what the user really wanted to do. As it turns out people actually wanted to return success with an error code and relied on this being possible in their applications. A better type system would help out a lot here, but it is also possible to write clean C code without this class of bugs. And OpenSSL has some of the worst code I have seen in an open source project (I have seen much worse in commercial projects), so while C has a lot of flaws do not judge it after OpenSSL.
dom0 9y ago
"safety net" cases are pretty much covered by design-by-contract 101.

PhantomGremlin 9y ago

surprised that even after the 'goto fail' story people still write code in this questionable style LibreSSL didn't spring into existence out of whole cloth. It started as a fork of OpenSSL, which goes back to 1998. The "questionable style" is from legacy code. It would be a massive effort to revise the entire codebase. And if LibreSSL did that, it would make it harder to import changes from OpenSSL and from other forks such as BoringSSL.

technion 9y ago
The code style in question is the current recommendation in the systemd style guide - a much more current project, and a rule that was put in place in 2014. https://github.com/systemd/systemd/commit/61f33134fc9231e07e1b9519b140d68139e9fad0#diff-cae545c4578eed1a167d69fb6d3b806bR82 https://github.com/systemd/systemd/commit/61f33134fc9231e07e... I'm all for blaming legacy but this is unfortunately still a trend.
kbenson 9y ago
> it would make it harder to import changes from OpenSSL When it's a choice between making it easier to import changes or harder to import bugs, I know which one I think is more important when dealing with a security library.
- askmike 9y ago
  I don't think you see the amount of work that would go into refactoring a project of this scale whilst keeping up with the latest patches from upstream.
  - kbenson 9y ago
    I do understand the the amount of work, but that wasn't the only argument presented. It was also presented as making it harder to accept patches. Since one of the goals of LibreSSL is to start applying security best practices, and one of the reasons for its existence is the poor quality and recurrent problems with OpenSSL, keeping compatibility with OpenSSL to make it easier to accept patches should be very low on the list of priorities. Put another way, if you forked because upstream was crap, not changing because it makes it easier to accept upstream patches is a poor reason not to change something that might benefit from it.