Thread: pgcrypto support for bcrypt $2b$ hashes
Hello, I've recently been working with a database containing bcrypt hashes generated by a 3rd-party which use the $2b$ prefix. Thisprefix was introduced in 2014 and has since been recognised by a number of bcrypt implementations. [1][2][3][4] At the moment, pgcrypto’s `crypt` doesn’t recognise this prefix. However, simply `replace`ing the prefix with $2a$ allowscrypt to validate the hashes. This patch simply adds recognition for the prefix and treats the hash identically tothe $2a$ hashes. Is this a reasonable change to pgcrypto? I note that the last upstream change brought into crypt-blowfish.c was in 2011,predating this prefix. [5] Are there deeper concerns or other upstream changes that need to be addressed alongside this?Is there a better approach to this? At the moment, the $2x$ variant is supported but not mentioned in the docs, so I haven’t included any documentation updates. Thanks, Daniel [1]: https://marc.info/?l=openbsd-misc&m=139320023202696 [2]: https://www.openwall.com/lists/announce/2014/08/31/1 [3]: https://github.com/kelektiv/node.bcrypt.js/pull/549/files#diff-c55280c5e4da52b0f86244d3b95c5ae0abf2fcd121a071dba1363540875b32bc [4]: https://github.com/bcrypt-ruby/bcrypt-ruby/commit/d19ea481618420922b533a8b0ed049109404cb13 [5]: https://github.com/postgres/postgres/commit/ca59dfa6f727fe3bf3a01904ec30e87f7fa5a67e
Attachment
> On 24 Sep 2021, at 04:12, Daniel Fone <daniel@fone.net.nz> wrote:
> At the moment, pgcrypto’s `crypt` doesn’t recognise this prefix. However, simply `replace`ing the prefix with $2a$
allowscrypt to validate the hashes. This patch simply adds recognition for the prefix and treats the hash identically
tothe $2a$ hashes.
But 2b and 2a hashes aren't equal, although very similar. 2a should have the
many-buggy to one-correct collision safety and 2b hashes shouldn't. The fact
that your hashes work isn't conclusive evidence.
> Is this a reasonable change to pgcrypto?
I think it's reasonable to support 2b hashes, but not like how this patch does
it.
> I note that the last upstream change brought into crypt-blowfish.c was in 2011, predating this prefix. [5] Are there
deeperconcerns or other upstream changes that need to be addressed alongside this?
Upgrading our crypt_blowfish.c to the upstream 1.3 version would be the correct
fix IMO, but since we have a few local modifications it's not a drop-in. I
don't think it would be too hairy, but one needs to be very careful when
dealing with crypto.
> Is there a better approach to this?
Compile with OpenSSL support, then pgcrypto will use the libcrypto implementation.
> At the moment, the $2x$ variant is supported but not mentioned in the docs, so I haven’t included any documentation
updates.
Actually it is, in table F.16 in the below documentation page we refer to our
supported level as "Blowfish-based, variant 2a".
https://www.postgresql.org/docs/devel/pgcrypto.html
--
Daniel Gustafsson https://vmware.com/
Hi Daniel, Thanks for the feedback. > On 26/09/2021, at 12:09 AM, Daniel Gustafsson <daniel@yesql.se> wrote: > > But 2b and 2a hashes aren't equal, although very similar. 2a should have the > many-buggy to one-correct collision safety and 2b hashes shouldn't. The fact > that your hashes work isn't conclusive evidence. I was afraid this might be a bit naive. Re-reading the crypt_blowfish release notes, it’s principally the changes introducing$2y$ into 1.2 that we need, with support for OpenBSD $2b$ introduced in 1.3. Do I understand this correctly? > Upgrading our crypt_blowfish.c to the upstream 1.3 version would be the correct > fix IMO, but since we have a few local modifications it's not a drop-in. I > don't think it would be too hairy, but one needs to be very careful when > dealing with crypto. My C experience is limited, but I can make an initial attempt if the effort would be worthwhile. Is this realistically apatch that a newcomer to the codebase should attempt? > Actually it is, in table F.16 in the below documentation page we refer to our > supported level as "Blowfish-based, variant 2a”. Sorry I wasn’t clear. My point was that the docs only mention $2a$, and $2x$ isn’t mentioned even though pgcrypto supportsit. As part of the upgrade to 1.3, perhaps the docs can be updated to mention variants x, y, and b as well. Thanks, Daniel
> On 28 Sep 2021, at 05:15, Daniel Fone <daniel@fone.net.nz> wrote: > > Hi Daniel, > > Thanks for the feedback. > >> On 26/09/2021, at 12:09 AM, Daniel Gustafsson <daniel@yesql.se> wrote: >> >> But 2b and 2a hashes aren't equal, although very similar. 2a should have the >> many-buggy to one-correct collision safety and 2b hashes shouldn't. The fact >> that your hashes work isn't conclusive evidence. > > I was afraid this might be a bit naive. Re-reading the crypt_blowfish release notes, it’s principally the changes introducing$2y$ into 1.2 that we need, with support for OpenBSD $2b$ introduced in 1.3. Do I understand this correctly? Yeah, we'd want a port of 1.3 into pgcrypto essentially. >> Upgrading our crypt_blowfish.c to the upstream 1.3 version would be the correct >> fix IMO, but since we have a few local modifications it's not a drop-in. I >> don't think it would be too hairy, but one needs to be very careful when >> dealing with crypto. > > My C experience is limited, but I can make an initial attempt if the effort would be worthwhile. Is this realisticallya patch that a newcomer to the codebase should attempt? I don't see why not, the best first patches are those scratching an itch. If you feel up for it then give it a go, I - and the rest of pgsql-hackers - can help if you need to bounce ideas. Many of the changes in the pgcrypto BF code is whitespace and formatting, which are performed via pgindent. I would suggest to familiarize yourself with pgindent in order to tease them out easier. Another set of changes are around error handling and reporting, which is postgres specific. >> Actually it is, in table F.16 in the below documentation page we refer to our >> supported level as "Blowfish-based, variant 2a”. > > Sorry I wasn’t clear. My point was that the docs only mention $2a$, and $2x$ isn’t mentioned even though pgcrypto supportsit. As part of the upgrade to 1.3, perhaps the docs can be updated to mention variants x, y, and b as well. Aha, now I see what you mean, yes you are right. I think the docs should be updated regardless of the above as a first step to properly match what's in the tree. Unless there are objections I propose to apply the attached. -- Daniel Gustafsson https://vmware.com/
Attachment
> On 29/09/2021, at 2:33 AM, Daniel Gustafsson <daniel@yesql.se> wrote: > >> On 28 Sep 2021, at 05:15, Daniel Fone <daniel@fone.net.nz> wrote: >> >>> On 26/09/2021, at 12:09 AM, Daniel Gustafsson <daniel@yesql.se> wrote: >>> >>> Upgrading our crypt_blowfish.c to the upstream 1.3 version would be the correct >>> fix IMO, but since we have a few local modifications it's not a drop-in. I >>> don't think it would be too hairy, but one needs to be very careful when >>> dealing with crypto. >> >> My C experience is limited, but I can make an initial attempt if the effort would be worthwhile. Is this realisticallya patch that a newcomer to the codebase should attempt? > > I don't see why not, the best first patches are those scratching an itch. If > you feel up for it then give it a go, I - and the rest of pgsql-hackers - can > help if you need to bounce ideas. I’m glad you said that. I couldn’t resist trying and have attached a patch. By referencing the respective git logs, I didn’thave too much difficulty identifying the material changes in each codebase. I’ve documented all the postgres-specificchanges to upstream in the header comment for each file. Daniel
Attachment
On 9/28/21 11:58 PM, Daniel Fone wrote:
>> On 29/09/2021, at 2:33 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
>> I don't see why not, the best first patches are those scratching an itch. If
>> you feel up for it then give it a go, I - and the rest of pgsql-hackers - can
>> help if you need to bounce ideas.
>
> I’m glad you said that. I couldn’t resist trying and have attached a patch. By referencing the respective git logs, I
didn’thave too much difficulty identifying the material changes in each codebase. I’ve documented all the
postgres-specificchanges to upstream in the header comment for each file.
I took a quick look and on a cursory glance it looks good but I got
these compilation warnings.
crypt-blowfish.c: In function ‘BF_crypt’:
crypt-blowfish.c:789:3: warning: ISO C90 forbids mixed declarations and
code [-Wdeclaration-after-statement]
789 | int done;
| ^~~
crypt-blowfish.c: In function ‘_crypt_blowfish_rn’:
crypt-blowfish.c:897:8: warning: variable ‘save_errno’ set but not used
[-Wunused-but-set-variable]
897 | int save_errno,
| ^~~~~~~~~~
Andreas
Hi Andreas, > On 1/10/2021, at 12:17 AM, Andreas Karlsson <andreas@proxel.se> wrote: > > On 9/28/21 11:58 PM, Daniel Fone wrote: >>> On 29/09/2021, at 2:33 AM, Daniel Gustafsson <daniel@yesql.se> wrote: >>> I don't see why not, the best first patches are those scratching an itch. If >>> you feel up for it then give it a go, I - and the rest of pgsql-hackers - can >>> help if you need to bounce ideas. >> I’m glad you said that. I couldn’t resist trying and have attached a patch. By referencing the respective git logs, Ididn’t have too much difficulty identifying the material changes in each codebase. I’ve documented all the postgres-specificchanges to upstream in the header comment for each file. > > I took a quick look and on a cursory glance it looks good but I got these compilation warnings. > > crypt-blowfish.c: In function ‘BF_crypt’: > crypt-blowfish.c:789:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] > 789 | int done; > | ^~~ > crypt-blowfish.c: In function ‘_crypt_blowfish_rn’: > crypt-blowfish.c:897:8: warning: variable ‘save_errno’ set but not used [-Wunused-but-set-variable] > 897 | int save_errno, > | ^~~~~~~~~~ I don’t get these compiler warnings and I can’t find any settings to use that might generate them. I’m compiling on macOS11.6 configured with `--enable-cassert --enable-depend --enable-debug CFLAGS=-O0` I’ve optimistically updated the patch to hopefully address them, but I’d like to know what I need to do to get those warnings. Thanks, Daniel
Attachment
On 02.10.2021 06:48, Daniel Fone wrote: > I don’t get these compiler warnings and I can’t find any settings to use that might generate them. I’m compiling on macOS11.6 configured with `--enable-cassert --enable-depend --enable-debug CFLAGS=-O0` > Hi, Daniel! I don't get them from clang on macOS either. > I’ve optimistically updated the patch to hopefully address them, but I’d like to know what I need to do to get those warnings. But gcc-11 on Ubuntu 20.04 emits them. Regards, -- Sergey Shinderuk https://postgrespro.com/
On 10/2/21 5:48 AM, Daniel Fone wrote: > I don’t get these compiler warnings and I can’t find any settings to use that might generate them. I’m compiling on macOS11.6 configured with `--enable-cassert --enable-depend --enable-debug CFLAGS=-O0` > > I’ve optimistically updated the patch to hopefully address them, but I’d like to know what I need to do to get those warnings. I run "gcc (Debian 10.3.0-11) 10.3.0" and your new patch silenced the warnings. Please add your patch to the current open commitfest. https://commitfest.postgresql.org/35/ Andreas
On Sat, 2 Oct 2021 at 23:22, Andreas Karlsson <andreas@proxel.se> wrote:
Please add your patch to the current open commitfest.
Thanks for the guidance.
Daniel
As discussed in [1], we're taking this opportunity to return some
patchsets that don't appear to be getting enough reviewer interest.
This is not a rejection, since we don't necessarily think there's
anything unacceptable about the entry, but it differs from a standard
"Returned with Feedback" in that there's probably not much actionable
feedback at all. Rather than code changes, what this patch needs is more
community interest. You might
- ask people for help with your approach,
- see if there are similar patches that your code could supplement,
- get interested parties to agree to review your patch in a CF, or
- possibly present the functionality in a way that's easier to review
overall.
(Doing these things is no guarantee that there will be interest, but
it's hopefully better than endlessly rebasing a patchset that is not
receiving any feedback from the community.)
Once you think you've built up some community support and the patchset
is ready for review, you (or any interested party) can resurrect the
patch entry by visiting
https://commitfest.postgresql.org/38/3338/
and changing the status to "Needs Review", and then changing the
status again to "Move to next CF". (Don't forget the second step;
hopefully we will have streamlined this in the near future!)
Thanks,
--Jacob
[1] https://postgr.es/m/86140760-8ba5-6f3a-3e6e-5ca6c060bd24@timescale.com