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.
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?
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.
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:
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.
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)
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
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.
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.
The only issue is that Rust programmers do not do anything usefull. The only place i heard Rust is used is in Firefox.
So, if Rust is so good, will be nice to see some real world uses.
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.
> 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.