*sshole, or when are we finally going to get rid of C?
32 points by novedevo
32 points by novedevo
There is nothing inherently C-caused in this vulnerability.
There is nothing inherently C-caused in this vulnerability.
Nothing is inherently caused by anything, in the sense that you can achieve any desired outcome with any given tool, if you put enough effort into it. Yet, we still say that there are good and bad tools, and similarly, there are languages that make it easier or harder to follow best practices.
In-band signaling with sentinel values is an astonishingly bad practice which is made astonishingly pervasive by C's bad type system.
In-band signalling in this context is also known as the semipredicate problem and like you say, it is pervasive in C
I wrote a summary of the things that led to this bug and one of the crucial issues that this article misses is standards conformance, and how standards can or don’t evolve to deal with new circumstances.
In this case the problem is that the specification for crypt() doesn’t allow for error cases except for the complete absence of crypt() itself. In practice most code is written as if crypt(&str, &str) -> String (or perhaps more accurately with the moral equivalent of .unwrap()) which in the past was reasonable when the code only works on implementations that have crypt().
So the question is then, how should crypt() evolve to support error cases that were not previously considered, such as malformed salt, salt that specifies an unsupported algorithm, unreasonably long passwords, etc. And there are two levels of answer to this question: at a small scale, what can or should the crypt() API look like, and at a larger scale, what are the standards and compatibility considerations for the various options.
At the small scale, musl decided to return an in-band value that is poorly chosen because it conflicts with existing assumptions about what strings crypt() can return. (A better choice would have been “:” since that cannot appear inside a field in the password file, so it can never match an account that doesn’t allow password logins.) Other implemenentations have chosen to expand the vestigial NULL return so that it can be used for dynamic errors that did not previously exist.
At the larger scale, the better choice depends on how the libc maintainers view their relationship with the programs that use libc and with POSIX. If you view the ecosystem as essentially fixed, too difficult to change, then musl’s in-band error is reasonable (except for the extremely dangerous choice of error value). On the other hand, if you think that the amount of effort needed to fix callers is small enough then it’s reasonable to improve the API.
Then there’s the question of how to fix POSIX so that it doesn’t lead to poor choices like musl’s. And that depends on whether changes to POSIX should occur first in the spec or first in implementations (or perhaps in parallel). I tend to think that specs should not be treated as if they were handed down by god, and when (as in this case) there’s an obvious way to fix a bad API, implementations shouldn’t be scared to lead the effort to fix it by writing code that deviates from the spec, especially if that is likely to encourage the spec to be changed to match.
In this case the problem is that the specification for crypt() doesn’t allow for error cases except for the complete absence of crypt() itself.
This is a fundamental misunderstanding of how POSIX works. Functions specified by POSIX can define errnos that either must or may be set in certain circumstances, but unless the function's specification explicitly states otherwise, then implementations may return any other appropriate errno as well.
Implementations shall not generate a different error number from one required by this volume of POSIX.1-2024 for an error condition described in this volume of POSIX.1-2024, but may generate additional errors unless explicitly disallowed for a particular function.
— https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html#tag_16_03
That’s a good point, but you also need to consider the preceding sentence:
Implementations may generate error numbers listed here under circumstances other than those described, if and only if all those error conditions can always be treated identically to the error conditions as described in this volume of POSIX.1-2024.
In the case of crypt() the difficulty is introducing errors that depend on the password and/or salt, because those kinds of errors need to be handled differently from the only error defined in POSIX (which will always fail or always succeed). It’s a practical difficulty too, because (as I wrote) there is old software that doesn’t check for errors from crypt() because historically it had no errors.
In the case of crypt() the difficulty is introducing errors that depend on the password and/or salt, because those kinds of errors need to be handled differently from the only error defined in POSIX (which will always fail or always succeed).
Not at all, musl could just as well choose to return a single errno value for all cases in which crypt was called with invalid arguments, which would be infinitely more expressive than what it currently does.
It’s a practical difficulty too, because (as I wrote) there is old software that doesn’t check for errors from crypt() because historically it had no errors.
That's not really true, it always had errors across various implementations, people failing to care doesn't mean that it could never happen.
Musl used to report errors from crypt but decided to stop doing so because C programmers don't know to check for NULL pointers. In the process they made buggy programs not crash but also made correct programs misbehave. That seems like a clear bug to me.
And by that rationale we should stop signaling errors everywhere that returns NULL because people skip NULL checks everywhere, including when calling malloc.
musl could just as well choose to return a single errno
Are you sure? POSIX doesn't normally allow routines to just invent new errors to be returned, so if you have surveyed a large number of POSIX-compatible applications that use crypt() and can report to them that programs will tolerate it, then you can just write to them and then yes, musl could use another errno.
I suspect that many applications don't check crypt() since it presently only returns ENOSYS and so fall-back on the assumption that the database is plain-text on every error which might be just as bad.
I think it would have been better to simply to put crypt() in the same bin we put gets()
The same norms the article points to would apply just as well here, IMO.
Evolving an unsound API would come with a lot of careful consideration and review, and in-band signaling wouldn't even enter into the discussion. The impact would be measurable with Crater, and automated tooling might even be developed if feasible to fix callers.
I think the point of the article is that Rust-the-engineering-culture is what matters. Rust-the-language is one expression of that culture.
musl decided to return an in-band value that is poorly chosen because it conflicts with existing assumptions about what strings crypt() can return. (A better choice would have been “:” since that cannot appear inside a field in the password file, so it can never match an account that doesn’t allow password logins.)
To be clear, musl doesn't return "" for accounts that don't allow password logins, it returns "x" in those scenarios (source). The reason why "" was return in OpenSSH was because as you noted in your summary, OpenSSH after determining that the doesn't support password authentication and the request should fails, decides to call crypt with a different users salt to burn time. But then decides to act on the result on the call even though it already had determined that the login attempt should fail.
I can see how might one argue that musl's choice of leaving a boobytrap for people that are misusing the crypt API is anti-social behaviour. But the lions share of the vulnerability lies in OpenSSH.
Then there’s the question of how to fix POSIX so that it doesn’t lead to poor choices like musl’s. And that depends on whether changes to POSIX should occur first in the spec or first in implementations
OpenSSH's upstream, OpenBSD stopped using crypt in 2007 (auth-password.c). Replacing crypt with auth-usercheck. That's plenty of time for the spec to catch up to implementations.
In-band signaling with sentinel values is an astonishingly bad practice which is made astonishingly pervasive by C's bad type system.
Those are a lot of opinions!
I don't agree that "in-band signalling with sentinel values" are "inherent" to C, but perhaps poor choices young programmers make, and my opinions is that software risk, whether that is a "security vulnerability" or just annoyingly buggy, largely comes from the dogmatic teaching/learning process most young programmers experience with software.
I know a lot of people have wasted a lot of time looking for tools to make them better, but that's just not how tools work.
Sometimes. Sometimes it's a great way to organize memory. For instance, a pool of interned null-terminated strings can be referenced using only 4 bytes each, as opposed to 16 bytes each for a pointer and length. If you have a lot of references to short strings, the savings are quite meaningful.
This has nothing to do with C or any programming language; this is a fundamental property of information theory.
Furthermore, it is possible to model sentinel-terminated pointers in the type system.
That's like saying wearing reflective safety vests is useless because the photons that are hitting the retina of the driver do not deccelerate the car that moves towards you. Which is technically true but also a total non-sequitur.
Technically the photons do decelerate the car. Just not by much.
I know, and I knew this would come up, but I didn't get around to correcting the phrasing. The fact that we are having this discussion shows how suitable the metaphor is ;-)
C failed to prevent such preventable problem.
It doesn't have a type system able to express "this is a failure that the caller must handle". Error handling in C is enforced by shaming programmers, which demonstrably is less effective than compile-time errors.
It doesn't have a type system able to express "this is a failure that the caller must handle".
C23 has [[nodiscard]] functions, and there has been an equivalent feature for decades in GNU C.
But in the case of crypt() the caller needs to use the return value to check the password, and if errors are represented by returning NULL in the idiomatic manner, then a caller will crash if it fails to handle the error.
So this vulnerability would not have occurred if crypt() was written in idiomatic C.
The vulnerability is caused in part because musl doesn’t return an error in the idiomatic way, but instead signals an error with a value that can match a disabled account (which should fail) in the same way that a successful return matches an account with the right password (which should succeed). (And in part because OpenSSH uses a dummy salt for checking disabled accounts.)
C failed to prevent such preventable problem.
Your opinion. I think it was the programmer, but I tend to not blame tools.
It doesn't have a type system able to express "this is a failure that the caller must handle"
That's your lack of imagination.
In my own APIs, I use negative pointers for special values I need to handle specially- not just errors, but also ciphertext. That means if someone gets it wrong they get SIGSEGV and will fix it quick. Such a thing is impossible in most languages but trivial in C.
Yes there is, in any language with proper union types you would be fired for even suggesting such a thing as representing NULL by "*". This is truely just a case of C having an insufficiently powerfull typesystem to express things correctly.
The post specifies a reason why the author think that C programming leads to this behavior.
You’re free to argue this reason is wrong. But you cannot responsibly just ignore the reason and state that this is not an issue with C. You might as well just say “No.”
Having recently dug into this topic a bit, I think the better question is
when are we finally getting rid of
crypt(3)?
So many subtly different input formats for the algorithm and salt...
I love C, I pretty much did C for the past twenty years of my life.
Yet I acknowledge that is a C problem (but Python, JavaScript, ... also have the same problem). We must move to an non legacy language when possible (I'm not saying Python or JavaScript are legacy).
We have enough computer power and modern languages to have good type systems and rule out most of the bugs. Yet we don't have enough "human power" to move all the legacy code to modern alternative.
It's not a flame war. Just saying that doing any new modern technology in C is absurd nowadays.
JS and Python don't have this problem - such APIs can (and do) throw exceptions instead of relying on callers checking for a special sentinel value. They can also just return undefined/None without worrying about callers causing UB.
This is a musl bug not a C issue. If you had the same logic written in Haskell or whatever your language of choice was you'd get the same issue.
The fact that AWS offers official images that use a libc reimplementation that's an endless source of bugs is a disservice to their customers.
Yes, but if you were to do this (store NULL as "*") rather than use union types in Haskell you would be doing something wrong.
What if the API was initially designed such that there was initially no concept of a user without a password, and you needed to compatibly shoehorn something in after the fact without breaking millions of lines of existing, ISO-standardized APIs?
Do you design every Haskell API with the idea that some day someone may add an error case, even though none currently exist?
That would be an issue, but in this case crypt was defined to be fallible from the start, and that's one of the key players here. In the C version, crypt returned a string or null, but the type system doesn't differentiate between those two, so it was easier for callers to incorrectly handle the return value. But in Haskell, crypt would have returned Either (I think?) with the string or an explicit error. And at that point, the caller cannot implicitly ignore anything, they have to deal with both cases explicitly, and the problem would never have shown up in the first place.
It wasn't initially failible -- failability was added quite a long time ago, though, when migrating to encrypted passwords in /etc/shadow.
From the post:
What if there isn’t an error variant today? Then a breaking change would add an error variant, and a RustSec advisory would probably be published as well.
I suspect the same would apply to Haskell. (Or maybe not, but I didn't bring up Haskell.)
but you wouldn't get the same logic in Haskell or whatever, because languages with better constructs for exceptional results use them.
C APIs don't even agree whether returning 0 means false or not-an-error-code. This is absolutely a C problem: primitivism of the language lead to a widespread problem of inconsistent ad-hoc error handling. Inability to enforce proper error handling by the compiler leads to the vicious cycle of sloppy library users summoning nasal demons, and library authors mitigating that with the crappy sentinels.
musl is libc... If you're not in C you don't need it at all
The powerful thing about community norms in the LLM age is that it's what the LLM will replicate.
If you make secure and memory-safe code the average, then the LLM will output secureish and memory safeish code.
The engineering norms of C developers allow for
"*"to be an acceptable sentinel value indicating an error; the engineering norms of Rust developers simply do not.
I call BS. This is empty Us vs. Them rhetoric masquerading as language advocacy.
The icing on the turd is writing "*sshole" instead of, say, "ss*hole". Who does that?
The icing on the turd is writing "sshole" instead of, say, "sshole". Who does that?
The people who published the vulnerability: https://sshhole.com/
The original website does that.
The name is sshhole, but the page title reads as *sshole, the 'h' is only shadowed, and the first character is an asterisk. This is confusing, and it's all just for a juvenile pun.
Ref: https://sshhole.com/
Rust has Option, Result and panic!() for such cases, and there's a decade of real-world experience and hundreds of thousands of libraries demonstrating that using them is indeed the norm.
OTOH C has been using sentinel values forever. EOF is in stdlib. NULL is normal and the standard C doesn't even have non-nullable pointers. In-band signalling without compiler enforcement is the norm in C.
Is there any technical constraint preventing C from acquiring a result type and/or multi-value return? I get there are social constraints and ecosystem issues that could arise but are those the only reasons?
Pretty sure you can already sort of pull off multi-value returns just by returning a struct by value. Result types can be easy or hard depending on how strict you want compile-time checks to be (basic discriminated unions are easy; statically ruling out misuse is harder, especially given C's relative paucity of information hiding options).
Also I feel that once you drop social and ecosystem constraints there probably isn't much left to prevent any language from changing in arbitrary ways?
Technically it seems doable to extend enum and switch to have at least some minimal tagged unions and basic pattern matching.
I think the problem is political and social:
std::variant at home. Microsoft's veto has chilling effect on C standardization and adoption of features. Projects worrying about portability and back compat would end up wrapping it in macros, creating ugly syntax nobody likes.Even without these obstacles it will take few years for the C working group to consider it, then a few more years before it lands in a C spec, then few more years before compilers ship it, then a few more years before distros switch to those compilers, then a few more years before notable C projects consider the syntax old enough to be acceptable to migrate to it. And even then, stdlib and POSIX probably won't ever change.
The article implies that Rust is magically more secure. uutils audit discovered 44 CVEs for things that are well known by experienced C programmers (many TOC/TOU) and not covered by Rust type system. These CVEs are the reason moving away from C for existing things is not as simple.