Thread: Refactor SCRAM code to dynamically handle hash type and key length

Hi all,
(Adding Daniel and Jonathan per recent threads)

While investigating on what it would take to extend SCRAM to use new
hash methods (say like the RFC draft for SCRAM-SHA-512), I have been
quickly reminded of the limitations created by SCRAM_KEY_LEN, which is
the key length that we use in the HMAC and hash computations when
creating a SCRAM verifier or when doing a SASL exchange.

Back in v10 when SCRAM was implemented, we have kept the
implementation simple and took the decision to rely heavily on buffers
with a static size of SCRAM_KEY_LEN during the exchanges.  This was a
good choice back then, because we did not really have a way to handle
errors and there were no need to worry about things like OOMs or even
SHA computations errors.  This was also incorrect in its own ways,
because we failed to go through OpenSSL for the hash/HMAC computations
which would be an issue with FIPS certifications.  This has led to the
introduction of the new cryptohash and HMAC APIs, and the SCRAM code
has been changed so as it is able to know and pass through its layers
any errors (OOM, hash/MAC computation, OpenSSL issue), as of 87ae969
and e6bdfd9.

Taking advantage of all the error stack and logic introduced
previously, it becomes rather straight-forward to remove the
hardcoded assumptions behind SHA-256 in the SCRAM code path.  Attached
is a patch that does so:
- SCRAM_KEY_LEN is removed from all the internal SCRAM routines, this
is replaced by a logic where the hash type and the key length are
stored in fe_scram_state for the frontend and scram_state for the
backend.
- The frontend assigns the hash type and the key length depending on
its choice of SASL mechanism in scram_init()@fe-auth-scram.c.
- The backend assigns the hash type and the key length based on the
parsed SCRAM entry from pg_authid, which works nicely when we need to
handle a raw password for a SASL exchange, for example.  That's
basically what we do now, but scram_state is just changed to work
through it.

We have currently on HEAD 68 references to SCRAM_KEY_LEN.  This brings
down these references to 6, that cannot really be avoided because we
still need to handle SCRAM-SHA-256 one way or another:
- When parsing a SCRAM password from pg_authid (to get a password type
or fill in scram_state in the backend).
- For the mock authentication, where SHA-256 is forced.  We are going
to fail anyway, so any hash would be fine as long as we just let the
user know about the failure transparently.
- When initializing the exchange in libpq based on the SASL mechanism
choice.
- scram-common.h itself.
- When building a verifier in the be-fe interfaces.  These could be
changed as well but I did not see a point in doing so yet.
- SCRAM_KEY_LEN is renamed to SCRAM_SHA_256_KEY_LEN to reflect its
dependency to SHA-256.

With this patch in place, the internals of SCRAM for SASL are
basically able to handle any hash methods.  There is much more to
consider like how we'd need to treat uaSCRAM (separate code for more
hash methods or use the same) or HBA entries, but this removes what I
consider to be 70%~80% of the pain in terms of extensibility with the
current code, and this is enough to be a submission on its own to move
towards more methods.  I am planning to tackle more things in terms of
pluggability after what's done here, btw :)

This patch passes check-world and the CI is green.  I have tested as
well the patch with SCRAM verifiers coming from a server initially on
HEAD, so it looks pretty solid seen from here, being careful of memory
leaks in the frontend, mainly.

Thoughts or comments?
--
Michael

On 14.12.22 03:38, Michael Paquier wrote:
> This patch passes check-world and the CI is green.  I have tested as
> well the patch with SCRAM verifiers coming from a server initially on
> HEAD, so it looks pretty solid seen from here, being careful of memory
> leaks in the frontend, mainly.

The changes from local arrays to dynamic allocation appear to introduce 
significant complexity.  I would reconsider that.  If we consider your 
reasoning

 > While investigating on what it would take to extend SCRAM to use new
 > hash methods (say like the RFC draft for SCRAM-SHA-512), I have been
 > quickly reminded of the limitations created by SCRAM_KEY_LEN, which is
 > the key length that we use in the HMAC and hash computations when
 > creating a SCRAM verifier or when doing a SASL exchange.

then the obvious fix there is to change the definition of SCRAM_KEY_LEN 
to PG_SHA512_DIGEST_LENGTH, which would be a much smaller and simpler 
change.  We don't have to support arbitrary key sizes, so a fixed-size 
array seems appropriate.

On Wed, Dec 14, 2022 at 02:39:43PM +0100, Peter Eisentraut wrote:
> On 14.12.22 03:38, Michael Paquier wrote:
>> While investigating on what it would take to extend SCRAM to use new
>> hash methods (say like the RFC draft for SCRAM-SHA-512), I have been
>> quickly reminded of the limitations created by SCRAM_KEY_LEN, which is
>> the key length that we use in the HMAC and hash computations when
>> creating a SCRAM verifier or when doing a SASL exchange.
>
> then the obvious fix there is to change the definition of SCRAM_KEY_LEN to
> PG_SHA512_DIGEST_LENGTH, which would be a much smaller and simpler change.
> We don't have to support arbitrary key sizes, so a fixed-size array seems
> appropriate.

Yeah, I was considering doing that as well for the static arrays, with
something like a Max() to combine but perhaps that's not necessary for
the digest lengths anyway.  Perhaps I just over-engineered the
approach.

However, that's only half of the picture.  The key length and the hash
type (or just the hash type to know what's the digest/key length to
use but that's more invasive) still need to be sent across the
internal routines of SCRAM and attached to the state data of the
frontend and the backend or we won't be able to do the hash and HMAC
computations dependent on that.
--
Michael

On Thu, Dec 15, 2022 at 04:59:52AM +0900, Michael Paquier wrote:
> However, that's only half of the picture.  The key length and the hash
> type (or just the hash type to know what's the digest/key length to
> use but that's more invasive) still need to be sent across the
> internal routines of SCRAM and attached to the state data of the
> frontend and the backend or we won't be able to do the hash and HMAC
> computations dependent on that.

Attached is a patch to do exactly that, and as a result v2 is half the
size of v1:
- SCRAM_KEY_LEN is now named SCRAM_MAX_KEY_LEN, adding a note that
this should be kept in sync as the maximum digest size of the
supported hash methods.  This is used as the method to size all the
internal buffers of the SCRAM routines.
- SCRAM_SHA_256_KEY_LEN is used to track the key length for
SCRAM-SHA-256, the one initialized with the state data.
- No changes in the internal, the buffers are just resized based on
the max defined.

I'd like to move on with that in the next couple of days (still need
to study more the other areas of the code to see what else could be
made more pluggable), so let me know if there are any objections..
--
Michael

On 12/16/22 10:08 PM, Michael Paquier wrote:
> On Thu, Dec 15, 2022 at 04:59:52AM +0900, Michael Paquier wrote:
>> However, that's only half of the picture.  The key length and the hash
>> type (or just the hash type to know what's the digest/key length to
>> use but that's more invasive) still need to be sent across the
>> internal routines of SCRAM and attached to the state data of the
>> frontend and the backend or we won't be able to do the hash and HMAC
>> computations dependent on that.
> 
> Attached is a patch to do exactly that, and as a result v2 is half the
> size of v1:
> - SCRAM_KEY_LEN is now named SCRAM_MAX_KEY_LEN, adding a note that
> this should be kept in sync as the maximum digest size of the
> supported hash methods.  This is used as the method to size all the
> internal buffers of the SCRAM routines.
> - SCRAM_SHA_256_KEY_LEN is used to track the key length for
> SCRAM-SHA-256, the one initialized with the state data.
> - No changes in the internal, the buffers are just resized based on
> the max defined.

Thanks! I looked through this and ran tests. I like the approach overall 
and I think this sets us up pretty well for expanding our SCRAM support.

Only a couple of minor comments:

- I noticed a couple of these in "scram_build_secret" and "scram_mock_salt":

   Assert(hash_type == PG_SHA256);

Presumably there to ensure 1/ We're setting a hash_type and 2/ as 
possibly a reminder to update the assertions if/when we support more 
digests.

With the assertion in "scram_build_secret", that value is set from the 
"PG_SHA256" constant anyway, so I don't know if it actually gives us 
anything other than a reminder? With "scram_mock"salt" the value 
ultimately comes from state (which is currently set from the constant), 
so perhaps there is a guard there.

At a minimum, I'd suggest a comment around it, especially if it's set up 
to be removed at a future date.

- I do like the "SCRAM_MAX_KEY_LEN" change, and I see we're now passing 
"key_length" around to ensure we're only using the desired number of 
bytes. I am a little queasy that once we expand "SCRAM_MAX_KEY_LEN" we 
run the risk of having the smaller hashes accidentally use the extra 
bytes in their calculations. However, I think that's more a fear than 
not, an we can mitigate the risk with testing.

> I'd like to move on with that in the next couple of days (still need
> to study more the other areas of the code to see what else could be
> made more pluggable), so let me know if there are any objections..

No objections. I think this decreases the lift to supporting more 
variations of SCRAM.

Once committed, I'll rebase the server-side SCRAM functions patch with 
this. I may want to rethink the interface for that to allow the digest 
to be "selectable" (vs. from the function) but I'll discuss on that 
thread[1].

Thanks,

Jonathan

[1] 
https://www.postgresql.org/message-id/fce7228e-d0d6-64a1-3dcb-bba85c2fac85@postgresql.org

On Mon, Dec 19, 2022 at 02:58:24PM -0500, Jonathan S. Katz wrote:
> With the assertion in "scram_build_secret", that value is set from the
> "PG_SHA256" constant anyway, so I don't know if it actually gives us
> anything other than a reminder? With "scram_mock"salt" the value ultimately
> comes from state (which is currently set from the constant), so perhaps
> there is a guard there.

Yes, these mostly act as reminders to anybody touching this code, so
I'd like to keep both.  For the mock part, we may also want to use
something different than SHA-256.

> At a minimum, I'd suggest a comment around it, especially if it's set up to
> be removed at a future date.

Okay, sure.

> - I do like the "SCRAM_MAX_KEY_LEN" change, and I see we're now passing
> "key_length" around to ensure we're only using the desired number of bytes.
> I am a little queasy that once we expand "SCRAM_MAX_KEY_LEN" we run the risk
> of having the smaller hashes accidentally use the extra bytes in their
> calculations. However, I think that's more a fear than not, an we can
> mitigate the risk with testing.

A few code paths relied on the size of these local buffers, now they
just use the passed-in key length from the state.

> No objections. I think this decreases the lift to supporting more variations
> of SCRAM.
>
> Once committed, I'll rebase the server-side SCRAM functions patch with this.
> I may want to rethink the interface for that to allow the digest to be
> "selectable" (vs. from the function) but I'll discuss on that thread[1].

Thanks!  I have applied for I have here..  There are other pieces to
think about in this area.
--
Michael

On Tue, Dec 20, 2022 at 08:58:38AM +0900, Michael Paquier wrote:
> Thanks!  I have applied for I have here..  There are other pieces to
> think about in this area.

FYI, I have spent a few hours looking at the remaining parts of the
SCRAM code that could be simplified if a new hash method is added, and
this b3bb7d1 has really made things easier.  There are a few things
that will need more thoughts.  Here are my notes, assuming that
SHA-512 is done:
1) HBA entries had better use a new keyword for scram-sha-512, implying
a new uaSCRAM512 to combine with the existing uaSCRAM.  One reason
behind that it to advertise the mechanisms supported back to the
client depending on the matching HBA entry.
2) If a role has a SCRAM-SHA-256 password and the HBA entry matches
scram-sha-512, the SASL exchange needs to go through the mock process
with SHA-512 and fail.
3) If a role has a SCRAM-SHA-512 password and the HBA entry matches
scram-sha-256, the SASL exchange needs to go through the mock process
with SHA-256 and fail.
4) The case of MD5 is something that looks a bit tricky at quick
glance.  We know that if the role has a MD5 password stored, we will
fail anyway.  So we could just advertise the SHA-256 mechanisms in
this case and map the mock to that?
5) The mechanism choice in libpq needs to be reworked a bit based on
what the backend sends.  There may be no point in advertising all the
SHA-256 and SHA-512 mechanisms at the same time, I guess.

Attached is a WIP patch that I have played with.  This shows the parts
of the code that would need more thoughts if implementing such
things.  This works for the cases 1~3 (see the TAP tests).  I've given
up on the MD5 case 4 for now, but perhaps I just missed a simple trick.
5 in libpq uses dirty tricks.  I have marked this CF entry as
committed, and I'll come back to each relevant part on new separate
threads.
--
Michael

On 12/20/22 2:25 AM, Michael Paquier wrote:
> On Tue, Dec 20, 2022 at 08:58:38AM +0900, Michael Paquier wrote:
>> Thanks!  I have applied for I have here..  There are other pieces to
>> think about in this area.
> 
> FYI, I have spent a few hours looking at the remaining parts of the
> SCRAM code that could be simplified if a new hash method is added, and
> this b3bb7d1 has really made things easier.

Great! Thanks for doing a quick "stress test" on this.

>  There are a few things
> that will need more thoughts.  Here are my notes, assuming that
> SHA-512 is done:
> 1) HBA entries had better use a new keyword for scram-sha-512, implying
> a new uaSCRAM512 to combine with the existing uaSCRAM.  One reason
> behind that it to advertise the mechanisms supported back to the
> client depending on the matching HBA entry.

This does seem like a challenge, particularly if we have to support 
multiple different SCRAM hashes.

Perhaps this can be done with an interface change in HBA. For example, 
we could rename the auth method from "scram-sha-256" to "scram" and 
support an option list of hashes (e.g. "hash=sha-512,sha-256"). We can 
then advertise the user-selected hashes as part of the handshake.

For backwards compatibility, we can take an auth method of 
"scram-sha-256" to mean "scram" + using a sha-256 hash. Similarly, if no 
hash map is defined, we can default to "scram-sha-256".

Anyway, I understand this point would require more discussion, but 
perhaps it is a way to simplify the amount of code we would need to 
write to support more hashes.

> 2) If a role has a SCRAM-SHA-256 password and the HBA entry matches
> scram-sha-512, the SASL exchange needs to go through the mock process
> with SHA-512 and fail.
> 3) If a role has a SCRAM-SHA-512 password and the HBA entry matches
> scram-sha-256, the SASL exchange needs to go through the mock process
> with SHA-256 and fail.

*nods*

> 4) The case of MD5 is something that looks a bit tricky at quick
> glance.  We know that if the role has a MD5 password stored, we will
> fail anyway.  So we could just advertise the SHA-256 mechanisms in
> this case and map the mock to that?

Is this the case where we specify "md5" as the auth method but the 
user-password is stored in SCRAM?

> 5) The mechanism choice in libpq needs to be reworked a bit based on
> what the backend sends.  There may be no point in advertising all the
> SHA-256 and SHA-512 mechanisms at the same time, I guess.

Yeah, I think a user may want to select which ones they want to use 
(e.g. they may not want to advertise SHA-256).

> Attached is a WIP patch that I have played with.  This shows the parts
> of the code that would need more thoughts if implementing such
> things.  This works for the cases 1~3 (see the TAP tests).  I've given
> up on the MD5 case 4 for now, but perhaps I just missed a simple trick.
> 5 in libpq uses dirty tricks.  I have marked this CF entry as
> committed, and I'll come back to each relevant part on new separate
> threads.

Thanks for starting this.

Jonathan

On Tue, Dec 20, 2022 at 08:45:29PM -0500, Jonathan S. Katz wrote:
> On 12/20/22 2:25 AM, Michael Paquier wrote:
>> 4) The case of MD5 is something that looks a bit tricky at quick
>> glance.  We know that if the role has a MD5 password stored, we will
>> fail anyway.  So we could just advertise the SHA-256 mechanisms in
>> this case and map the mock to that?
>
> Is this the case where we specify "md5" as the auth method but the
> user-password is stored in SCRAM?

Yes.  A port storing uaMD5 with a SCRAM password makes the backend use
SASL for the whole exchange.  At quick glance, we could fallback to
look at the password of the user sent by the startup packet and
advertise the mechanisms based on that because we know that one user
=> one password currently.  I'd need to double-check on the RFCs to
see if there is anything specific here to worry about.  The recent
ones being worked on may tell more.

>> 5) The mechanism choice in libpq needs to be reworked a bit based on
>> what the backend sends.  There may be no point in advertising all the
>> SHA-256 and SHA-512 mechanisms at the same time, I guess.
>
> Yeah, I think a user may want to select which ones they want to use (e.g.
> they may not want to advertise SHA-256).

Yep, they should be able to do so.  libpq should select the strongest
one if the server sends all of them, but things like
https://commitfest.postgresql.org/41/3716/ should be able to enforce
one over the other.
--
Michael