Horton

Thread

Commits

  1. Avoid using SplitIdentifierString to parse ListenAddresses, too.

  2. Avoid downcasing/truncation of RADIUS authentication parameters.

  3. Support multiple RADIUS servers

  1. BUG #16106: Patch - Radius secrets always gets lowercased

    The Post Office <noreply@postgresql.org> — 2019-11-11T18:01:35Z

    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,

  2. Re: BUG #16106: Patch - Radius secrets always gets lowercased

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-11T20:23:07Z

    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

  3. Re: BUG #16106: Patch - Radius secrets always gets lowercased

    Marcos David <mdavid@palantir.com> — 2019-11-12T15:57:57Z

    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 secret sent 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 anyway for 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,

  4. Re: BUG #16106: Patch - Radius secrets always gets lowercased

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-12T16:33:46Z

    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 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.

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

  5. Re: BUG #16106: Patch - Radius secrets always gets lowercased

    Magnus Hagander <magnus@hagander.net> — 2019-11-12T16:45:13Z

    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
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  6. Re: BUG #16106: Patch - Radius secrets always gets lowercased

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-12T18:07:14Z

    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