the new ssh musl vuln is incredible

@karolherbst (it's not a musl vuln)

@fun yeah it's not a musl vuln, it's a vuln in openssh caused by musl...

@karolherbst Not even that

@fun I mean.. not even sure why you are trying to argue here and for what.

Yeah musl showed unexpected behavior that was causing an error in an application. Or do you want to say that the behavior was in fact expected?

@karolherbst Let's just say that ssh was being sketchy here by checking crypt(attacker_controlled, someone_elses_salt)

@fun @karolherbst I mean, musl is not being POSIX-compliant here. The musl devs attempted to work around bugs in other programs by adding this behavior and then inadvertently made other, standards-compliant programs* vulnerable. How is that _not_ a musl bug?

(* at least in regards to the usage of crypt here, I don't know the OpenSSH source that well)

@cyberslak @karolherbst musl is being POSIX-compliant here, crypt only has one error defined (ENOSYS). Invalid input is undefined.

Also, libxcrypt does exactly the same, except it returns "*0" not "*". NetBSD and FreeBSD also.

@fun sure, and musl showed behavior that is a C spec violation. And if you show behavior, in your libc implementation, that isn't according to the specification, then.... you need to be really careful what to do.

Sure, applications _can_ be more careful, and they shouldn't do weird things, but as it looks today, it's a spec violation.

@karolherbst the only defined error in POSIX is ENOSYS. There's nothing about how to handle invalid input.

@fun @cyberslak isn't EINVAL defined for invalid inputs?

@karolherbst @cyberslak not at all

@karolherbst @cyberslak https://pubs.opengroup.org/onlinepubs/9799919799/

@fun @cyberslak I'd argue that it is pretty clear that it shall return NULL on error though.

@fun @karolherbst it's a real stretch to call this "successful completion".. I would say it is much more like "this functionality is not supported on this implementation" (the functionality being, hashing a >256 byte string with this algo). Though that is also stretching the traditional meaning of ENOSYS a little..

@karolherbst @cyberslak then libxcrypt (impl of crypt() used by all glibc distros) should do it too. They don't currently, they do the same as musl (except it's a different string they return, "*0" instead of "*").

@fun @cyberslak yes, they should return NULL and applications should get fixed instead.

@fun @cyberslak like sure it sucks. I do understand the choice being made here, but... you also have to be careful about how to do implementation defined behavior that isn't screwing over users.

This is just one of those cases where it went badly.

@karolherbst @cyberslak agreed.

IMO the POSIX spec is just broken. It's not clear at all, but yes, they should all return NULL and apps should fix themselves.

@fun @cyberslak POSIX spec being broken is a safe assumption to make generally...

like yes.. the API isn't well designed.

@fun @karolherbst I would argue it should, yeah, though undoubtedly it would work out that making the other programs crash would induce a vulnerability somewhere else because that's just the state of the world right now. But then at least it would be a bug in that software :P

@cyberslak @karolherbst I am *also* baffled though that sshd does generate a hash out of thin air to compare with "*" on users that have no password... I think that is also bad behavior and it should be fixed too. (it should do that but disregard the result Imo)

@fun @cyberslak yeah.. I haven't looked at the workaround yet, but I'd expect them that they did something like that to fix it.

@karolherbst @cyberslak no they didn't, they now just check that the output of crypt() is more than 4 bytes now, among some amount of obfuscation.

@fun @karolherbst "disregard the result" also feels like a recipe for "oops! that got optimized out!" though. there's kind of no winning writing security-critical code in C unfortunately I fear.

@cyberslak @karolherbst you can disregard the result without it getting optimized out, I'm sure there's tricks for that

@fun @karolherbst I agree there probably are, but then you're relying on the tricks, and the fact this requires tricks in the first place makes me uneasy. This stuff should be well-specified. I imagine they structured their workaround the way they did because it keeps the code flow from diverging and keeps the timing as consistent as possible. But honestly at this point it's a bunch of speculation from me and I don't write this kind of code (thank God 😅).

@cyberslak @karolherbst That code they currently have is already relying on fragile tricks