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

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