OpenSSL Security Advisory (14 December 2021)(openssl.org)
openssl.org
OpenSSL Security Advisory (14 December 2021)
https://www.openssl.org/news/secadv/20211214.txt
43 comments
Still does not inspire confidence. This is the second CVE this year, after they pushed out a vulnerable TLS 1.3 version.
It's actually the 8th:
https://www.openssl.org/news/vulnerabilities.html
But this really isn't that unusual for a library of that complexity and size. All of these vulns are pretty boring, none is likely to cause any bigger harm.
But this really isn't that unusual for a library of that complexity and size. All of these vulns are pretty boring, none is likely to cause any bigger harm.
Hmm also the TLS 1.3 bug was in April 2020... Uh oh I am on covid time.
For those curious CVE-2021-4044 looks to be fixed by a couple lines changed.
https://github.com/openssl/openssl/commit/c1c1bb7c5e2baa109b...
https://github.com/openssl/openssl/commit/c1c1bb7c5e2baa109b...
Doesn't look like a fix to me, rather amplifying the problem.
i = X509_verify_cert(ctx);
/* We treat an error in the same way as a failure to verify */
if (i < 0)
i = 0;
It's documented as returning 1 when verification succeeds and 0 when it fails, so this appears to turn an error into a failure to verify, which seems to be the intent?
Now look at their insane API.
The ssl verify returns
The ssl verify returns
Verify a certificate chain
* Return codes:
* 1: Verify success
* 0: Verify failure or error
* -1: Retry required
*/
Right. Previously the return code from x509_verify_cert was returned directly to the caller, which was incorrect - ssl_verify_chain_cert was supposed to return -1 if the caller should retry, but would end up doing so on any error that was returned from x509_verify_cert. So the patch is correct.
If your argument is that the return codes should be consistent across different layers of OpenSSL then I'm not going to disagree, but this is a large old codebase and this isn't some unique case.
If your argument is that the return codes should be consistent across different layers of OpenSSL then I'm not going to disagree, but this is a large old codebase and this isn't some unique case.
I’m pretty sure this has caused CVEs before. Folks will check !ret and call it a day.
You just never used OpenSSL, their API is legendary.
They are using 0 to indicate verification failure, according to the comment above the function.
Siblings have said this is fine and I agree, but I agree w/ you that the intent is obscured. It looks like a clamp or a hack. I think it'd be clearer to assign to a different variable? Ex:
And finally, i isn't a wonderful name for this variable. rv or ret or status are all better options.
int vr = X509_verify_cert(ctx);
if vr <= 0 {
i = 0;
} else {
i = 1;
}
And finally, i isn't a wonderful name for this variable. rv or ret or status are all better options.
For some reason, bad naming conventions are baked into the C programming culture. Like you aren’t a real C programmer if you don’t use two letter variables. I’m assuming short names and schemes like Polish notation once had a place when RAM was more expensive, but I can’t comprehend why new C code can’t use modern practices.
> Like you aren’t a real C programmer if you don’t use two letter variables.
Reasonable people disagree about this. John Ousterhout is in the "use meaningful variable names" camp, and Rob Pike is in the "use short variable names when the meaning is obvious" camp.
The reason I think "i" is bad here is that "i" usually (almost always?) means "index". You could return "i" if you were searching for the index of something, but that's not what this function is doing.
A lot of bugs have this pattern of layers of incongruity. "i" is usually an index, but here it's the return status value. These two sections of the library use different return status value semantics. You can kind of think about it like steering a ship through icebergs of inconsistency--eventually you're gonna hit one.
There are things you can do, and they all exist on the continuum of "easy/ineffective - hard/super effective". This function could use enum/#define. As a sibling comment said, if OpenSSL used typedef'd enums for its status codes the compiler would complain, and then you can use switch/case (maybe even wrapped in a conversion function) to convert between different universes of status codes. You could completely separate these sections of the library, which are currently very intermingled, and communicate between them using specific interfaces.
As an example for that last one, this code doesn't care about various error conditions when it comes to cert verification, it just wants to know if the cert checks out or not. So when X509_verify_cert returns more than 2 possible values, it's leaking irrelevant information. The X509 section could have a different function that returned a boolean, specifically for uses that don't care about the details of certificate verification failure.
And actually, it looks like every single use of X509_verify_cert only checks for > 0. Nothing cares about 0 vs. < 0. Was it ever important for something external to know if there were internal errors vs. the certificate failed? Is that even a good idea at all?
These are the kinds of things you look into and think, "what is going on here". Evidence starts mounting that like, this thing isn't built very well (or this gem [1], the product of a code formatter, making one worry about potential other goto fail problems)
[1]: https://github.com/openssl/openssl/blob/1c0eede9827b0962f1d7...
Reasonable people disagree about this. John Ousterhout is in the "use meaningful variable names" camp, and Rob Pike is in the "use short variable names when the meaning is obvious" camp.
The reason I think "i" is bad here is that "i" usually (almost always?) means "index". You could return "i" if you were searching for the index of something, but that's not what this function is doing.
A lot of bugs have this pattern of layers of incongruity. "i" is usually an index, but here it's the return status value. These two sections of the library use different return status value semantics. You can kind of think about it like steering a ship through icebergs of inconsistency--eventually you're gonna hit one.
There are things you can do, and they all exist on the continuum of "easy/ineffective - hard/super effective". This function could use enum/#define. As a sibling comment said, if OpenSSL used typedef'd enums for its status codes the compiler would complain, and then you can use switch/case (maybe even wrapped in a conversion function) to convert between different universes of status codes. You could completely separate these sections of the library, which are currently very intermingled, and communicate between them using specific interfaces.
As an example for that last one, this code doesn't care about various error conditions when it comes to cert verification, it just wants to know if the cert checks out or not. So when X509_verify_cert returns more than 2 possible values, it's leaking irrelevant information. The X509 section could have a different function that returned a boolean, specifically for uses that don't care about the details of certificate verification failure.
And actually, it looks like every single use of X509_verify_cert only checks for > 0. Nothing cares about 0 vs. < 0. Was it ever important for something external to know if there were internal errors vs. the certificate failed? Is that even a good idea at all?
These are the kinds of things you look into and think, "what is going on here". Evidence starts mounting that like, this thing isn't built very well (or this gem [1], the product of a code formatter, making one worry about potential other goto fail problems)
[1]: https://github.com/openssl/openssl/blob/1c0eede9827b0962f1d7...
Pretty sure Polish notation has nothing to do with limited RAM
Hungarian notation is what I was thinking of
[deleted]
C return code error handling strikes again.
You mean crappy programmers strike again? This bug would not happen by properly using structs, typedefs, enums, etc to qualify data types and explicitly declare (in a human-readable way anyway) what values to return when. Just using integers as return values with no intermediate abstraction is crazy.
Since we aren't all as smart as you, there's genuine value in languages & principles that guide us dummies to naturally avoid these kinds of issues as part of the happy path
Given the poor track record of the OpenSSL project, the grand parent's complaint for crappy programming isn't unfounded.
I'm not that smart. I don't have a high IQ or anything. I'm not even a professional software engineer. But if I need to use a tool, I learn the right way to use it. I don't expect it to be easy to use.
The biggest lie in software is that new software is better than old software. Anyone who's used software for long enough knows this is ridiculous. You introduce some new software which has some new features indented to fix an old flaw. But then it turns out there's new flaws in the new software that become more of an issue than the old flaws.
Most tools in the world don't change significantly over time. Instead of changing the tool, people learn how to use the tool to avoid problems. But that doesn't work in a world where nobody learns how to use the tool right, or is constantly picking up new tools and never learning lessons.
The biggest lie in software is that new software is better than old software. Anyone who's used software for long enough knows this is ridiculous. You introduce some new software which has some new features indented to fix an old flaw. But then it turns out there's new flaws in the new software that become more of an issue than the old flaws.
Most tools in the world don't change significantly over time. Instead of changing the tool, people learn how to use the tool to avoid problems. But that doesn't work in a world where nobody learns how to use the tool right, or is constantly picking up new tools and never learning lessons.
Isn't the compiler supposed to protect us? /s
Feels harsh to call C programmers "crappy" but ok
Isn’t Linus a pretty hard-core C programmer?
Not sure I’d call him “crappy.” Not to his face, anyway …
Not sure I’d call him “crappy.” Not to his face, anyway …
I'm fairly sure he'd have no such reticence to calling you (or me) crappy to our (mailing list) faces...
This is true, but I have to admit, he does C (and programming, in general) quite well.
[deleted]
> This bug would not happen by not using C
FTFY
FTFY
Come on. As 0xbadcafebee hinted at, you can practically write OCaml in C if you wish to. And good luck dealing with timing attacks, secure data deletion and overflow behavior in other languages.
All those are pretty easy in Rust.
Rust software is bug free by design /s.
Rust software is free of entire classes of bugs by default, by design, and the language lends itself to patterns that eliminate even more.
Nobody is making that argument. It's about what the language/libraries guide you to naturally.
Really not a value-add contribution. A lot of software that has existed forever is written in C by unpaid volunteers; are you planning to rewrite it in rust?
Further, snark could be made about how this will have near zero security impact yet there’s a far more serious bug (log4j) which has already had significant cost and follows endless patterns of Java developers embedding logic flow in untrusted data. “Just don’t use Java lol” etc
Edit: Hahaha, hey David. Had no idea that was you.
Further, snark could be made about how this will have near zero security impact yet there’s a far more serious bug (log4j) which has already had significant cost and follows endless patterns of Java developers embedding logic flow in untrusted data. “Just don’t use Java lol” etc
Edit: Hahaha, hey David. Had no idea that was you.
Who is this :D mason?
And this is why projects like https://boringssl.googlesource.com/boringssl/ exist
> OpenSSL 3.0.0 SSL/TLS clients are affected by this issue. > OpenSSL 1.1.1 and 1.0.2 are not affected by this issue.
I haven't encountered any stable distro running a higher version than 1.1.1. Arch, Debian Unstable, Alpine latest, and Ubuntu 20.10 are all on 1.1.1.