Thread: BUG #16106: Patch - Radius secrets always gets lowercased
The following bug has been logged on the website: Bug reference: 16106 Logged by: Marcos David Email address: mdavid@palantir.com PostgreSQL version: 12.0 Operating system: Centos 7 Description: I'm using radius authentication in pg_hba.conf and I've run into the following issue. The radiussecrets is always getting lowercased even if I start it with double quotes. Seems the double quotes are removed by the tokenization process and then the secret gets lowercased by https://github.com/postgres/postgres/blob/REL_12_STABLE/src/backend/utils/adt/varlena.c#L3652 I'm attaching a patch for this since I don't think the secrets should ever be lowercased. --- src/backend/libpq/hba.c.old 2019-11-08 16:42:27.934508368 +0000 +++ src/backend/libpq/hba.c 2019-11-08 16:43:12.069173627 +0000 @@ -2011,7 +2011,7 @@ REQUIRE_AUTH_OPTION(uaRADIUS, "radiussecrets", "radius"); - if (!SplitIdentifierString(dupval, ',', &parsed_secrets)) + if (!SplitGUCList(dupval, ',', &parsed_secrets)) { /* syntax error in list */ ereport(elevel, @@ -2033,7 +2033,7 @@ REQUIRE_AUTH_OPTION(uaRADIUS, "radiusidentifiers", "radius"); - if (!SplitIdentifierString(dupval, ',', &parsed_identifiers)) + if (!SplitGUCList(dupval, ',', &parsed_identifiers)) { /* syntax error in list */ ereport(elevel,
PG Bug reporting form <noreply@postgresql.org> writes: > I'm using radius authentication in pg_hba.conf and I've run into the > following issue. > The radiussecrets is always getting lowercased even if I start it with > double quotes. Seems the double quotes are removed by the tokenization > process and then the secret gets lowercased by > https://github.com/postgres/postgres/blob/REL_12_STABLE/src/backend/utils/adt/varlena.c#L3652 > I'm attaching a patch for this since I don't think the secrets should ever > be lowercased. Hm. I know zip about RADIUS but this seems like generally a sane change to make. The other very-dubious-in-this-context assumption that is embedded in SplitIdentifierString is that the strings should be truncated at NAMEDATALEN. Why did you not change the parsing for all four RADIUS options? Probably case-folding wouldn't matter for the server names, but the length limitation could. (Hmm ... on the same principle, PostmasterMain probably shouldn't be using this function for parsing ListenAddresses.) I'm hesitant to back-patch a change like this, because in theory it could change a working configuration into a non-working one. But it'd be sensible to do in HEAD. regards, tom lane
On 11/11/2019, 20:24, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: > PG Bug reporting form <noreply@postgresql.org> writes: > > I'm using radius authentication in pg_hba.conf and I've run into the > > following issue. > > The radiussecrets is always getting lowercased even if I start it with > > double quotes. Seems the double quotes are removed by the tokenization > > process and then the secret gets lowercased by > > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_postgres_postgres_blob_REL-5F12-5FSTABLE_src_backend_utils_adt_varlena.c-23L3652&d=DwIFAg&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=RfiS9YBgBDc5Zhy12-AtR1hy_bcIbiTRHiI6ByZ4K0g&m=WfTxWV_TdwLYGd3LXR1CA-JOl_p-ykuP6TwfdpvoXzA&s=IoON5fYePjnpuFN1eX2piFEkT8LfV-g4D7jA-evG_Ew&e= > > I'm attaching a patch for this since I don't think the secrets should ever > > be lowercased. > > Hm. I know zip about RADIUS but this seems like generally a sane > change to make. The other very-dubious-in-this-context assumption > that is embedded in SplitIdentifierString is that the strings should > be truncated at NAMEDATALEN. > > Why did you not change the parsing for all four RADIUS options? > Probably case-folding wouldn't matter for the server names, > but the length limitation could. > > (Hmm ... on the same principle, PostmasterMain probably shouldn't > be using this function for parsing ListenAddresses.) > > I'm hesitant to back-patch a change like this, because in theory > it could change a working configuration into a non-working one. > But it'd be sensible to do in HEAD. > > regards, tom lane > Hi Tom, Thanks for taking the time to review this. I'm resubmitting the patch with changes to all fields. Should I send the patch to pgsql-hackers? The issue here is that this is currently broken, the setup won't work as described in the documentation since the secretsent to Radius won't match (unless it's already lowercased) We only noticed this because we were upgrading from 9.6 and it seems this bug was introduced in 10 in this commit: https://github.com/postgres/postgres/commit/6b76f1bb58f53aec25cfec76391270ea36ad1170 I don't think patch would break anything in current configs since the secret would currently need to be lowercased anywayfor the radius auth to work. Kind regards, Marcos David --- src/backend/libpq/hba.copy.c 2019-11-12 14:29:12.000000000 +0000 +++ src/backend/libpq/hba.c 2019-11-12 14:17:29.000000000 +0000 @@ -1927,7 +1927,7 @@ REQUIRE_AUTH_OPTION(uaRADIUS, "radiusservers", "radius"); - if (!SplitIdentifierString(dupval, ',', &parsed_servers)) + if (!SplitGUCList(dupval, ',', &parsed_servers)) { /* syntax error in list */ ereport(elevel, @@ -1976,7 +1976,7 @@ REQUIRE_AUTH_OPTION(uaRADIUS, "radiusports", "radius"); - if (!SplitIdentifierString(dupval, ',', &parsed_ports)) + if (!SplitGUCList(dupval, ',', &parsed_ports)) { ereport(elevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), @@ -2011,7 +2011,7 @@ REQUIRE_AUTH_OPTION(uaRADIUS, "radiussecrets", "radius"); - if (!SplitIdentifierString(dupval, ',', &parsed_secrets)) + if (!SplitGUCList(dupval, ',', &parsed_secrets)) { /* syntax error in list */ ereport(elevel, @@ -2033,7 +2033,7 @@ REQUIRE_AUTH_OPTION(uaRADIUS, "radiusidentifiers", "radius"); - if (!SplitIdentifierString(dupval, ',', &parsed_identifiers)) + if (!SplitGUCList(dupval, ',', &parsed_identifiers)) { /* syntax error in list */ ereport(elevel,
Marcos David <mdavid@palantir.com> writes: > On 11/11/2019, 20:24, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: >> I'm hesitant to back-patch a change like this, because in theory >> it could change a working configuration into a non-working one. >> But it'd be sensible to do in HEAD. > We only noticed this because we were upgrading from 9.6 and it seems this bug was introduced in 10 in this commit: > https://github.com/postgres/postgres/commit/6b76f1bb58f53aec25cfec76391270ea36ad1170 Oh! Hm, if it can be painted as a regression, that changes the calculus a bit. In that case I'd be inclined to go ahead and back-patch. > I don't think patch would break anything in current configs since the secret would currently need to be lowercased anywayfor the radius auth to work. The case I was imagining was where the secret was entered in the PG configuration with some uppercase letters, but the server actually expects lowercase, so the forced lowercasing makes it work. I admit that's a bit of a stretch, but if it had always worked like that then it's at least possible someone was relying on the behavior. But if we changed the behavior from correct to less correct, that's another story. BTW, it looks to me like it should work to double-quote the secret, although doing so is really tedious because there is an additional layer of double-quoting required by the pg_hba syntax: host ... radiussecrets="""ServerSecret"",""OtherServersSecret""" However, while you can defeat the downcasing that way, you can't bypass the truncation to NAMEDATALEN. So it's arguably broken even if this point had been documented, which it was not, at least not in any adequate way (the reference to quoting in the docs is mighty unclear to my eyes, and for sure it doesn't give a working example). Unfortunately, we've missed the window to get it into this week's releases, but I'll see about getting this committed after the release cycle finishes. regards, tom lane
On Tue, Nov 12, 2019 at 10:33 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Marcos David <mdavid@palantir.com> writes:
> On 11/11/2019, 20:24, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>> I'm hesitant to back-patch a change like this, because in theory
>> it could change a working configuration into a non-working one.
>> But it'd be sensible to do in HEAD.
> We only noticed this because we were upgrading from 9.6 and it seems this bug was introduced in 10 in this commit:
> https://github.com/postgres/postgres/commit/6b76f1bb58f53aec25cfec76391270ea36ad1170
Oh! Hm, if it can be painted as a regression, that changes the calculus
a bit. In that case I'd be inclined to go ahead and back-patch.
Ugh. Yeah.
> I don't think patch would break anything in current configs since the secret would currently need to be lowercased anyway for the radius auth to work.
The case I was imagining was where the secret was entered in the PG
configuration with some uppercase letters, but the server actually
expects lowercase, so the forced lowercasing makes it work. I admit
that's a bit of a stretch, but if it had always worked like that
then it's at least possible someone was relying on the behavior.
But if we changed the behavior from correct to less correct, that's
another story.
I agree that this is definitely a risk, and probably something that happens. But one can't really argue that the current behaviour isn't a bug, since the lowercasing certainly isn't documented.
I bet a lot of people just use autogenerated lowercase passwords though, which is why we don't hear much about it. Or randomness passed through a hash function turned into a hex-string.
BTW, it looks to me like it should work to double-quote the
secret, although doing so is really tedious because there is an
additional layer of double-quoting required by the pg_hba syntax:
host ... radiussecrets="""ServerSecret"",""OtherServersSecret"""
However, while you can defeat the downcasing that way, you can't
bypass the truncation to NAMEDATALEN. So it's arguably broken
even if this point had been documented, which it was not,
at least not in any adequate way (the reference to quoting in
the docs is mighty unclear to my eyes, and for sure it doesn't
give a working example).
I believe the RADIUS standard doesn't actually specify the length of the key, so different implementations have different limits. For example freeradius has 48 characters, cisco has 63.
*silent* truncation is not great of course...
Magnus Hagander <magnus@hagander.net> writes:
> On Tue, Nov 12, 2019 at 10:33 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However, while you can defeat the downcasing that way, you can't
>> bypass the truncation to NAMEDATALEN. So it's arguably broken
> I believe the RADIUS standard doesn't actually specify the length of the
> key, so different implementations have different limits. For example
> freeradius has 48 characters, cisco has 63.
I agree that it's somewhat unlikely that truncation at 63 bytes would
matter in practical use-cases, for any of these four parameters.
Still, it's not a good thing, and the fix is trivial given that we
have a suitable function at hand already.
I'll try to improve the docs while I'm at it.
regards, tom lane