Re: BlastRADIUS mitigation

On Tue, Aug 6, 2024 at 2:41 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> Seems that on linux or freebsd, you'd plow ahead even if the binary is
> not found, and fail later, while on macOS you'd skip the tests. I think
> we should always error out if the dependencies are not found. If you
> make an effort to add PG_TEST_EXTRA=radius, presumably you really do
> want to run the tests, and if dependencies are missing you'd like to
> know about it.

Fixed.

> > + secret = "shared-secret"
>
> Let's use a random value for this, and for the Cleartext-Password. This
> only runs locally, and only if you explicitly add it to PG_TEST_EXTRA,
> but it still seems nice to protect from other users on the system when
> we can do so easily.

OK, done.

> > +security {
> > + require_message_authenticator = "yes"
> > +}
>
> > +# Note that require_message_authenticator defaulted to "no" before 3.2.5, and
> > +# then switched to "auto" (a new mode that fills the logs up with warning
> > +# messages about clients that don't send MA), and presumably a later version
> > +# will default to "yes".
>
> That's not quite accurate: the option didn't exist before version 3.2.5.
> What happens if you set it on an older server version? /me tests: seems
> that FreeRadius 3.2.1 silently accepts the setting, or any other setting
> it doesn't recognize, and will do nothing with it. A little surprising,
> but ok. I didn't find any mention in the docs on that.

Huh. Thanks, I was confused by that. Fixed.

> (Also, that will make the test fail, until the
> v1-0003-Mitigation-for-BlastRADIUS.patch is also applied. You could
> leave that out of the test in this first patch, and add it
> v1-0003-Mitigation-for-BlastRADIUS.patch)

Yeah, done.

> Review on v1-0003-Mitigation-for-BlastRADIUS.patch:
>
> > + <varlistentry>
> > + <term><literal>radiusrequirema</literal></term>
> > + <listitem>
> > + <para>
> > + Whether to require a valid <literal>Message-Authenticator</literal>
> > + attribute in messages received from RADIUS servers, and ignore messages
> > + that don't contain it. The default value
> > + is <literal>0</literal>, but it can be set to <literal>1</literal>
> > + to enable that requirement.
> > + This setting does not affect requests sent by
> > + <productname>PostgreSQL</productname> to the RADIUS server, which
> > + always include a <literal>Message-Authenticator</literal> attribute
> > + (but didn't in earlier releases).
> > + </para>
> > + </listitem>
> > + </varlistentry>
>
> I think this should include some advise on why and when you should set
> it. Something like:
>
> Enabling this mitigates the RADIUS protocol vulnerability described at
> blastradius.fail. It is recommended to always set this to 1, unless you
> are running an older RADIUS server version that does include the
> mitigation to include Message-Authenticator in all replies. The default
> will be changed to 1 in a future PostgreSQL version.

Done, with small tweaks.

> > attr = (radius_attribute *) (packet + RADIUS_HEADER_LENGTH);
> >
> > while ((uint8 *) attr < end)
> > {
> > /* Would this attribute overflow the buffer? */
> > if (&attr->length >= end || (uint8 *) attr + attr->length > end)
> > return false;
>
> Is this kind of pointer arithmetic safe? In theory, if a packet is
> malformed so that attr->length points beyond the end of the packet,
> "(uint8 *) attr + attr-length" might overflow. That would require the
> packet buffer to be allocated just before the end of the address space,
> so probably cannot happen in practice. I don't remember if there are
> some guarantees on that in C99 or other standards. Still, perhaps it
> would be better to write this differently, e.g. using a separate "size_t
> position" variable to track the current position in the buffer.

Done.

> (This also relies on the fact that "struct radius_attribute" doesn't
> require alignment, which is valid, and radius_add_attribute() made that
> assumption already. Maybe worth a comment though while we're at it; it
> certainly raised my eyebrow while reading this)

Comment added.

> What if the message contains multiple attribute of the same type? If
> there's a duplicate Message-Authenticator, we should surely reject the
> packet. I don't know if duplicate attributes are legal in general.

Duplicate attributes are legal in general per RFC 2865, which has a
table of attributes and their possible quantity; unfortunately this
one is an extension from RFC 2869, and I didn't find where it pins
that down. I suppose we could try to detect an unexpected duplicate,
which might have the side benefit of checking the rest of the
attributes for well-formedness (though in practice there aren't any).
Is it worth bothering with that?

I suppose if we wanted to be extra fastidious, we could also test with
a gallery of malformed packets crafted by a Perl script, but that
feels like overkill. On the other hand it would be bad if you could
crash a server by lobbing UDP packets at it because of some dumb
thinko.

> > + /*
> > + * Add Message-Authenticator attribute first, which for now holds zeroes.
> > + * We remember where it is in the message so that we can fill it in later.
> > + */
>
> Let's mention Blast-RADIUS here as the reason to put this first. Reading
> the paper though, I think it's only important in the server->client
> messages, but I'm not sure, and shouldn't hurt anyway.

Done.

> > + else if (message_authenticator_location == NULL)
> > + {
> > + ereport(LOG,
> > + (errmsg("RADIUS response from %s has no Message-Authenticator",
> > + server)));
> > +
> > + /* We'll ignore this message, unless pg_hba.conf told us not to. */
> > + if (requirema)
> > + continue;
> > + }
>
> This is going to be very noisy if you are running an older server.

Silenced.

> > + uint8 message_authenticator_key[RADIUS_VECTOR_LENGTH];
> > + uint8 message_authenticator[RADIUS_VECTOR_LENGTH];
>
> Perhaps use MD5_DIGEST_LENGTH for these. The Message-Authenticator is an
> HMAC-MD5, which indeed has the same length as the MD5 hash used on the
> password, so it's just pro forma, but it seems a little coincidental.
> There's no fundamental reason they would have to be the same length, if
> the RFC author's had chosen to use a different hash algorithm for
> Message-Authenticator, for example.

The first one is now gone (see next).

Now I have message_authenticator[MD5_DIGEST_LENGTH], and then the
other places that need that number use
lengthof(message_authenticator).

> If the secret is longer than 16 bytes, this truncates it. Is that
> correct? According to https://en.wikipedia.org/wiki/HMAC, you're
> supposed derive the suitably-sized key by calling the hash function on
> the longer key in that case.

Oh, actually I don't think we need that step at all: the HMAC init
function takes a variable length key and does the required
padding/hashing itself.

> # v1-0004-Teach-007_radius-test-about-Message-Authenticator.patch
>
> Looks good to me, although I'm not sure it's worthwhile to do this.
> We're not reaching the codepath where we'd reject a message because of a
> missing Message-Authenticator anyway. If the radiusrequirema option was
> broken and had no effect, for example, the test would still pass.

Dropped.

> # v1-0005-XXX-BlastRADIUS-back-patch-kludge-for-12-and-13.patch
>
> Perhaps throw an error if you set "radiusrequirema=1", but the server is
> compiled without OpenSSL.

Done.

I don't think I would turn this on in the build farm, because of the 3
second timeout which might cause noise. Elsewhere I had a patch to
make the timeout configurable, so it could be set long for positive
tests and short for negative tests, so we could maybe do that in
master and think about turning the test on somewhere.