Comment by BoppreH
2 days ago
In-band signaling[1] strikes again. * should not have been used as special value, neither for "no password" in /etc/passwd nor "error while hashing the password" in crypt. I'm afraid we'll find many more of these abuses, specially in string manipulations. The null was ok, an exception (or equivalent) better, and Result<T> (or equivalent) best.
[1] https://en.wikipedia.org/wiki/In-band_signaling#Computer_dat...
Exceptions are almost never the right choice. They create extremely complicated control flow extremely quickly, and then you have to test all the different control flows. Java software is an example of how this goes sideways fast. There's no way to create a class that prevents callers from reading the object before the constructor runs (in multi-threaded software, new first hands a reference to the caller, which can put it on the heap, then it calls the constructor), and objects are default initialized, so all fields (even static ones that are assigned in the constructor) can be null.
So, any field de-reference in Java can produce a null pointer exception. Try testing all paths in something like:
try { foo.sideEffect1().sideEffect2()...sideEffectN(); } catch { bar.sideEffect1().sideEffect2()...sideEffectM(); } finally { quuz.sideEffect1().sideEffect2()...sideEffectP(); }
There are M * N * P paths. The only way to write testable code with exceptions is to panic on uncaught exception, and to never catch unexpected exceptions. Even then, you're relying on nothing unexpectedly throwing the exception type you caught.
I'd argue Result<T> >> NULL >> sentries >> exceptions for this reason. Sentries are terrible, but at least they're not computationally hard to use correctly.
I'd argue that's the fault of having default nullability, not the exceptions. You would have the same problem if every attribute access returned a Result<T> too.
And I'm not a fan of breaking control flow, but I still find it preferable to nulls. A null may travel far from the place where it was generated before blowing up, making troubleshooting harder, or it might come from a function you called for its side effect but forgot to check the return value. I gotta say I'm also a careful enthusiast of checked exceptions, it's great knowing which ways a function can fail.
But I think the consensus is growing that Result<T> (plus warnings for unused return values) is simply the superior choice.
C has what you are asking for, mostly:
As long as an error condition on the return value causes a crash on use, then it's mostly the same as result. (The main remaining difference being that you do an implicit `.unwrap()` in C, and in things like rust that's an explicit code smell.)
> There's no way to create a class that prevents callers from reading the object before the constructor runs
That can only happen if you leak the reference (given we are talking about java language level semantics and not byte code), which I give you is unfortunately not that hard to do.
As for exceptions, I think they would pair beautifully with software transactional memory, which would work well with effects. In that case (and no side effects) it would not matter what code path we took, we could just go back to a known safe point.
Because ergonomy-wise I still think exceptions win (even though I do like Maybe types, and Rust's ? macro is plenty good): - they let one focus on the actual happy path code - no error is ever swallowed, unless explicitly written so (looking at you go with useless if errs that fail to properly handle the error), they are always handled somewhere - they automatically "unwrap" the Ok result, and automatically bubble up the Err side. - bubbling up is especially useful because most errors can't be handled at their source (e.g. IO error is meaningless, how you handle it depends on what you want to do in the first place, which is done several layers higher) - they automatically attach a stackframe/source, while with result types you may get a list of Ok Ok Ok Err and have no idea how that Err got there.
As the author of the class, I cannot prevent my caller from leaking the reference, so my public API allows for pre-constructor invocations of all my methods.
Software transactional memory doesn't really work with exceptions, since it precludes the use of catch + finally. It looks more like a top-level panic handler (where the throwing code cannot permute the heap, which is cool). To get exceptions, you'd need nested software transactional memory, which gets into obscure transaction processing theory. There's also the problem that your transaction could side-effect the outside world, then throw and be aborted, so you need to rework how I/O works if you walk down that path.
Exceptions are basically write-only, so I agree that their ergonomics for that use case are pretty good. Panic is better though, since it's really no error handling. Panic is easier to test, and easier to read / understand.
> There's no way to create a class that prevents callers from reading the object before the constructor runs (in multi-threaded software, new first hands a reference to the caller, which can put it on the heap, then it calls the constructor)
Which is why the pattern of double-checked locking doesn't work.
https://en.wikipedia.org/wiki/Double-checked_locking
It's wrong. The caller doesn't get the reference until the constructor runs. Although it looks like two operations in bytecode, the only thing you can legally do with an uninitialised object reference is call its constructor.
2 replies →
> never catch unexpected exceptions
Yes, broad exception catch clauses are simply incorrect code.
Panic is maybe an overreaction, "scratch everything and orderly abort" is sufficient.
I'm inclined to say a Panic is absolutely preferable to root auth.
You should never ignore exceptions! The least you can do is to crash the program. Then you fix the bugs via battle hardening until it no longer crashes. Making the program not crash by ignoring the errors will just mess with the state of the program until you have no idea what caused it. It's better to let it crash, load the dump into your debugger tool to see what went wrong, then fix the bug, and repeat until you have a stable program. Rather then just live by the motto "ignorance is a bliss".
Exceptions work fine and enable a clean separation of business logic and error handling. Visibility into partially-initialized objects is a language design issue independent of exceptions. In a non-exceptional language, the corresponding access to uninitialized fields would just be bugs, not errors you can see.
Nothing in your post actually supports the idea that exceptions are "computationally hard". The CFG error edges exist however the errors are spelled
It is difficult to get to M * N * P control flow edges with O(1) result unwrapping code.
1 reply →
> There's no way to create a class that prevents callers from reading the object before the constructor runs
Doesn't this address the issue? https://openjdk.org/jeps/539
Unfortunately we’re moving to llm based systems which rely entirely on in-band signaling.
PSA: you can typically declare different types of string so that your type system objects to mixing data of different types even though deep down we know they're both strings.
If you mean type aliases, that's a good idea within a single codebase, but the distinction between different types of strings doesn't survive serialization so it wouldn't have prevented this error. What they need is a way to signal errors out-of-band, like Result<T> or exceptions.
In python, aliases are separate from typing.NewType, but you're right: it's only enforced within a codebase and then only if the dev cares to run the typechecker.
So yeah, I guess it either needs to happen at runtime, or we just do signaling and user data on separate file descriptors.
This is presuming that the type system is nomitive rather than structural and there is not serialisation/deserialisation or type refinement occurring. A structural difference is much more preferable.
This is a side-effect of Unix-y "it's all plain text" design that leans extremely heavily upon in-band signalling (among other design decisions).
If /etc/shadow was a JSON (or XML, even, although that would make me feel gross), this bug would have been much easier to see, because a field that reads '"hash": ""' sticks out a lot* more than a custom microformat that you don't understand.
With data structures, there's never "plain text" - it's all structured. The question is whether you want to use existing formats and tools, or create a new one for every tool and literally make footguns like this.
A few years back I un-bashed my life as much as possible in favor of nushell (though it's not the only shell to encourage structure). I feel like it has really helped me notice a lot of this-needs-more-structure type problems outside of my shell that I otherwise wouldn't have.
The elegance of the unix-y approach is actually that it's all bits. You can just join things together and shove the bits through, that's a wild simplification compared with having vendor-specific kinds of plugs. But we could have that kind of compatibility while also benefiting from structure.
So I encourage everybody to give a shell with structured output a try, let it rewire your brain, and then ask yourself: how is the OS holding this back from being everything it could be? Because if we build that OS, it won't encourage these problems.
When I imagine it, it would probe either side of a pipe to see if they support negotiation. If not, send the bits through in the same old unixy way. If so, use the newest schema for structured data that is supported by both sides.
At Amazon we had RecordStream which implemented most of the standard Unix tools on structured streams. It was pretty easy to write filters, aggregators, and formatted output in something not much more complicated than sed/awk/cut/join/sort pipelines. There was a convenient structured log format for services that was often used as input.
https://github.com/benbernard/RecordStream
3 replies →
Returning “*” instead of NULL to signal failure is the real footgun. Don’t blame the format.
For historical reasons, we could’ve ended up JSON as “hash”: “*” just the same. The /etc/shadow format lets you skip values completely, like “::”, so your argument doesn’t hold.
Terminating an app because of a NULL dereference is the other footgun. Sadly, we still have “panic” in modern languages like Go and Rust.
"panic" is a good thing. There's always some error condition that your code cannot handle. At some point, the least bad thing to do is crash.
For instance, what if your error handling code throws?
One spectacular example of panic saving the day was the Apollo 11 1202 error. It was basically a watchdog timer that fired if the landing computer was starved of CPU time. A misconfiguration caused the first moon landing to hit CPU overload. When that happened, the computer displayed 1202 and rebooted, then ran things in priority order. Apollo 11 landed in the middle of a tight 'panic, reboot, adjust thrusters' loop. Thanks to core memory, most of the heap survived the reboot, so the reboot step was fast enough for it to mostly land without human interaction. The pilot had to make ~1 adjustment to get it to actually hit the landing site, but the panic-ridden computer handled the rest of the descent.
https://dpma.de/english/our_office/publications/ingeniouswom...
4 replies →
> is the real footgun
This contains a false snuck premise - that one and only one thing was the footgun. This is categorically not true. An obtuse, poorly-understood, purpose-built text-based DSL is absolutely a footgun and if it was replaced with a JSON and inactive accounts had been indicated by `"active": false` instead of a freaking asterisk then this would absolutely would not have happened.
> For historical reasons, we could’ve ended up JSON as “hash”: “” just the same.
That's dishonest, because it intentionally conceals the fact that that's a vanishingly* unlikely scenario.
People have brains that, factually, are far more likely to see a single context-less character "" and not realize the implications of it than to see `"hash": ""` and think "oh, that's normal".
This is just straight-up psychologically true.
> The /etc/shadow format lets you skip values completely, like “::”
Totally irrelevant.
> so your argument doesn’t hold.
False `::` is both (1) still harder to understand than `"active": false` and (2) even though the syntax existed it wasn't used here because everyone thought they understood the system.
Don't make excuses for this insane text syntax. It is, factually, harder to understand than a decently-written JSON format.
The issue isn't the config, but the fact that crypt() doesn't have any way to signal failure. In better designed languages you would have exceptions, result types, or something. Even in C you have "return integer indicating success or error" which would've prevented the confusion.
Now the public API is hard to change but you could still avoid the issue by returning a longer token like "==DANGEROUS_CRYPT_ERROR_length==" or "==DANGEROUS_CRYPT_ERROR_unsupported_alg==" which would never match the password field unless someone was really trying to induce the issue.
>but the fact that crypt() doesn't have any way to signal failure
https://pubs.opengroup.org/onlinepubs/009604599/functions/cr...
>Upon successful completion, crypt() shall return a pointer to the encoded string. The first two characters of the returned value shall be those of the salt argument. Otherwise, it shall return a null pointer and set errno to indicate the error.
returning a sentinel value of NULL and setting errno is pretty standard C, and shouldn't cause an error.
OP's sshhole link specifically notes that it _should_ return NULL, but some MUSL devs decided they wanted it to return trash instead, and the trash they returned instead of NULL caused this.
All because some devs were too lazy to check the result and another dev decided to be clever and "fix" NULL errors from crypt by sending back 'unmatchable' trash banking on devs not checking NULL.
edit: apparently crypt implementations have been trash ignoring the NULL return for a long time. this isn't only on MUSL, but it's still because people ignored and tried to work around the existing out-of-band error mechanism.
19 replies →
Personally I think there was nothing wrong with returning NULL. "But if people don't check for it it crashes their programs!" Good! Let it crash loudly! Then they notice and fix the issue.
Also I think it could have returned ":". Given the file format of /etc/shadow and /etc/passwd I don't think it's possible to have a : as a value for anything.
2 replies →
> The issue isn't the config
That's a meaningless statement. There isn't a singular "the" issue like you said. Any one of several different technical decisions would have completely prevented the issue. Including, to my point, /etc/shadow being a JSON file where an account being locked is an `"active": false` field instead of an asterisk with no intrinsic meaning.
No, that's just more in-band signaling!
Some badly designed password hashing algorithms (cough bcrypt cough) have maximum password lengths, ignoring everything after X characters. So it could happen that I set my password to a 100-character string, it gets saved as "==DANGEROUS_CRYPT_ERROR_length==", and then anybody can log in to my account by entering an unrelated long string that returns the same "error".
If a function promises a string result, it should only provide a string if the function succeeded. The null was much better than magic strings.
2 replies →
Yes, absolutely. Take for example this cURL command:
Random parts of it will be interpreted by the shell, others by cURL, or the filesystem, or the remote server. They each have their own encoding rules, nested. I count ten distinct separators and nine ways of escaping/tagging. It's one command. No, just no.
We have seriously overused strings and we're now paying the price.
Also: free text is the input format of LLMs...
At this point I’m resigning myself to think that we’ve just gotta wait for some people to die. It feels very dramatic and glib (no pun intended lol) and whatever to say, but at this point it feels like there’s a certain hostility in some parts to changing anything from the platonic ideals of however computing happened to work in 2004. “Oh we don’t need to mess with it!”
How did you arrive at that conclusion? If anything, this vulnerability happened because someone decided to change the behaviour of crypt() without fully understanding the consequences. This specific vulnerability did not exist in 2004.
1 reply →
I think this is a bit unfair. The way I see it, a lot of people are saying that changing breaks lots of old stuff, and risks complete fragmentation. Those are real costs to change. It might still be worth it, but let's not pretend that everyone is resisting change just because it's change.
4 replies →
> "it's all plain text" design that leans extremely heavily upon in-band signalling
Honestly I've been shaking my fist at these since 0 becomes True/Success in Bash and the opposite everywhere else
Error code 0 means success in a lot more places than just bash.
That's not specifically bash though, that's process exit codes
4 replies →
I'd say another UNIX-y thing - "C is good enough" - is part of the problem. C makes programming painful which results in all sorts of hacks.
E.g. in Pascal you can can define a record and then defined file of record to have typed I/O. I.e. it comes with built-in serialization format supported by compiler. Not a good one - but perhaps a step in a good direction.
OTOH C people starving for serialization often believe that `struct` byte layout made by a specific version of a compiler is a serialization format - a view not endorsed by the standard.
Pascal's record typed files have the exact same problems regarding endianness and word size as directly writing structs to files in C, so I don't see how they're any better. They're also a complete deadend and unusable for pretty much any file format from the last 40 years+, since you can't even handle a header or magic bytes at the start.
It also looked to me a bit like a semipredicate problem:
https://en.wikipedia.org/wiki/Semipredicate_problem
Even using "no_password" and "error_while_hashing" would have prevented it. The problem was picking a generic, meaningless signal string.
That would've happened to fix it, because "error_while_hashing" and "no_password" happen to not be valid password hashes. The right way would've been to have different syntax for hashes and non-hashes; you could've made it so that a hash starts with a '#' and if it doesn't start with a '#', the rest of the string should be ignored.
Or, for backwards compatibility, start non-hash strings with a character which can't appear in a hash, such as '@'. Your examples would then become '@no_password' and '@error_while_hashing'.
There were different syntaxes for hashes and non hashes. The problem was that both a non hash "this password isn't set" and a non hash "this call to crypt produced an error" used the same non password value.
Well neither * nor ! (the two characters I've seen to indicate "no password") will appear in a valid hash. The problem was using a character that could also be returned as an error indicator, IIUC.
4 replies →
Technically they did use a character which can't appear in a hash, '*'.
1 reply →
Should have set crypt_errno.