Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:114105 Return-Path: Delivered-To: mailing list internals@lists.php.net Received: (qmail 16980 invoked from network); 22 Apr 2021 16:40:19 -0000 Received: from unknown (HELO php-smtp4.php.net) (45.112.84.5) by pb1.pair.com with SMTP; 22 Apr 2021 16:40:19 -0000 Received: from php-smtp4.php.net (localhost [127.0.0.1]) by php-smtp4.php.net (Postfix) with ESMTP id CAD151804D8 for ; Thu, 22 Apr 2021 09:43:23 -0700 (PDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on php-smtp4.php.net X-Spam-Level: * X-Spam-Status: No, score=1.4 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_SOFTFAIL autolearn=no autolearn_force=no version=3.4.2 X-Spam-Virus: No X-Envelope-From: Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by php-smtp4.php.net (Postfix) with ESMTPS for ; Thu, 22 Apr 2021 09:43:23 -0700 (PDT) Received: by mail-lf1-f41.google.com with SMTP id y4so32984154lfl.10 for ; Thu, 22 Apr 2021 09:43:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0Jh1JXTCLAyqKCLSePLqpFRvoFisAGzsgpZdzUEI3Tc=; b=UdjXVVMLMRJYUE/f+r2hxf+2fk24ABhDi0FxlorBysVV+EsdAFm7ZQFbv8XKDk9Tbh hrKGpY9KzrIbUZtgvK4AujHtnoI52AYmkP5XSQ/Kf9UnuiP48pbWSm0nsEOo6rXFQun3 2HfoTaXfyXLBZEW3lB0KSBOQ4KEoKMfOzJ7W3dtNkvoR0siyLFY+muBYAYw6cPIEpvKz 7h7P5nsD0KksggoxpmGu0xj/H5s/milvQLtUtrONTaQTJv2c00Vbb/xHJ6hdRXGPlW7K uymZV/loMgxPre+0ywQFOc3LdWJjUFi9vxMHXNK5quZlzh/0sdiRq6cJAnV2THMo/1bq iR3w== X-Gm-Message-State: AOAM5301I9ziLwUfaYDabDMCDM9E2/aShS1ka/WTBA40oIh7LRS4TEVV u7dWbU4xikwYLu100ZME6QD5GPgIgDzvVR6dyqWLzQ== X-Google-Smtp-Source: ABdhPJzGAXfC5jTqLxS2LuQlLuUGoxE/cx7kbtWSPDO3cKXZDsS7VCaKsv7iVV7uqiSOkOvSZCaQ/CI/FX6h7C+dpok= X-Received: by 2002:ac2:52b7:: with SMTP id r23mr3042632lfm.451.1619109798774; Thu, 22 Apr 2021 09:43:18 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: Date: Thu, 22 Apr 2021 11:43:08 -0500 Message-ID: To: Kamil Tekiela Cc: Niklas Keller , PHP internals Content-Type: multipart/alternative; boundary="000000000000689a2d05c0926078" Subject: Re: [PHP-DEV] Binary (un)safety of password_hash() used with PASSWORD_BCRYPT From: pollita@php.net (Sara Golemon) --000000000000689a2d05c0926078 Content-Type: text/plain; charset="UTF-8" On Thu, Apr 22, 2021 at 11:04 AM Kamil Tekiela 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 > --000000000000689a2d05c0926078--