On Thu, Apr 22, 2021 at 11:04 AM Kamil Tekiela <tekiela...@gmail.com> wrote: > > I don't like throwing exceptions for pretty much the same reasons as Nikita described. > This is a rather limited attack vector. It depends either on the user going out of their way > to make their password vulnerable or on the developer introducing the vulnerability with > the use of another function mangling the passwords. The latter is more likely. In your > example, you had to make the output of MD5 binary to make the function fail. In fact, > as far as I know, browsers strip NUL bytes or convert them to Unicode unknown character. > See spec https://html.spec.whatwg.org/multipage/parsing.html#parse-error-unexpected-null-character > To simplify this discussion, let's take it as a given that we're not getting nul bytes directly from the end user, or if we are that they deserve what they get.
I'd like to focus on the other case, where pre-digests are done. I'll agree that it's the framework author doing something unexpected that puts us in this situation, but I don't agree that them doing so is unreasonable. It's known that bcrypt has a length limitation. It's also clear that bits of collision space are wasted by using ascii input. Those two facts make producing binary output from a pre-digest function entirely reasonable. Specifically, the largest output digest function available for the input space. sha512 produces 64 bytes of output which fit neatly into bcrypt's 72 character limit. There is a 64 * (1/256) chance that at least one of those octets is a \0. Or more concisely 1 in 8. The degree to which this weakens the password depends on the specific position, anywhere from negligible (last position) to complete (first position). I don't like 1/8 odds of my password being made less secure due to a poor understanding of the limits of the default* password algorithm. That's my motivation. * bcrypt being the only "always available" algorithm is an issue unto itself and deserves a separate thread. > As for PHP's behaviour, what we could do is strip NUL bytes before hashing with bcrypt. > This would still weaken passwords, but at least wouldn't discard the rest of the string. > That's an option, but then we run into multi-version incompatibilities. If a machine running 8.1 hashes "foo\0bar" (meaning it effectively hashes "foobar"), other machines running 8.1+ can verify it just fine, but any machine running 8.0- try to verify against "foo", and fail. This is a specific issue for heterogeneous deployments, so maybe that's a tolerable thing, but I'd probably introduce it in a major (e.g. 9.0). We could add the fallback check in verify now, and the change to hash in 9.0 so that the only way it'd fail would be producing on 9.0+ but verifying on 8.0- which is a bit extreme in terms of mixed deployments. I'll be honest, I'd probably defer the exception in this PR until 9.0 as well, given the pushback. > We could also add a warning to the manual explaining that the function is not binary safe. > 100% Gonna do that regardless of any decision made here. -Sara >
