14 ms·
OpenSSL Security Advisory (14 December 2021)
- adamdecaf 5y agoFor those curious CVE-2021-4044 looks to be fixed by a couple lines changed. https://github.com/openssl/openssl/commit/c1c1bb7c5e2baa109baec62d2af09d24caae5557 https://github.com/openssl/openssl/commit/c1c1bb7c5e2baa109b...
- rurban 5y agoDoesn'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; So this turns an error into a verify. Maybe they should hire more experienced devs, or give up. This is just insanity.
- CameronNemo 5y agoThey are using 0 to indicate verification failure, according to the comment above the function.
- mjg59 5y agoIt'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?
- rurban 5y agoNow look at their insane API. The ssl verify returns Verify a certificate chain * Return codes: * 1: Verify success * 0: Verify failure or error * -1: Retry required */ But the x509 cert verify returns -1 for verification error.
- mjg59 5y agoRight. 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.
- dc-programmer 5y agoI’m pretty sure this has caused CVEs before. Folks will check !ret and call it a day.
- GoblinSlayer 5y agoYou just never used OpenSSL, their API is legendary.
- camgunz 5y agoSiblings 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: int vr = X509_verify_cert(ctx); if vr <= 0 { i = 0; } else { i = 1; } But also, like another commenter suggested, using enums or #define would be helpful documentation too. And finally, i isn't a wonderful name for this variable. rv or ret or status are all better options.
- deleted 5y ago[deleted]
- dc-programmer 5y agoFor 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.
- usefulcat 5y agoPretty sure Polish notation has nothing to do with limited RAM
- dc-programmer 5y agoHungarian notation is what I was thinking of
- camgunz 5y ago> 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/1c0eede9827b0962f1d752fa4ab5d436fa039da4/crypto/pkcs7/pk7_smime.c#L289 https://github.com/openssl/openssl/blob/1c0eede9827b0962f1d7...
- throitallaway 5y agoFTA: > 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.
- CameronNemo 5y agoStill does not inspire confidence. This is the second CVE this year, after they pushed out a vulnerable TLS 1.3 version.
- hannob 5y agoIt's actually the 8th: https://www.openssl.org/news/vulnerabilities.html 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.
- CameronNemo 5y agoHmm also the TLS 1.3 bug was in April 2020... Uh oh I am on covid time.
- staticassertion 5y agoC return code error handling strikes again.
- 0xbadcafebee 5y agoYou 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.
- ketralnis 5y agoSince 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
- tadbit 5y agoGiven the poor track record of the OpenSSL project, the grand parent's complaint for crappy programming isn't unfounded.
- dylan604 5y agoIsn't the compiler supposed to protect us? /s
- 0xbadcafebee 5y agoI'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.
- mistat 5y agoAnd this is why projects like https://boringssl.googlesource.com/boringssl/ https://boringssl.googlesource.com/boringssl/ exist