Re: BlastRADIUS mitigation

On 05/08/2024 15:43, Thomas Munro wrote:
> The response requirement can be enabled by radiusrequirema=1 in
> pg_hba.conf. For example, Debian stable is currently shipping
> FreeRADIUS 3.2.1 which doesn't yet send the MA in its responses, but
> FreeBSD and Debian "testing" have started shipping FreeRADIUS 3.2.5
> which is how I noticed all this. So it doesn't seem quite right to
> require it by default, yet?

Agreed.

> Since PostgreSQL v12 and v13 don't have the modern "common/hmac.h"
> API, I came up with a cheap kludge: locally #define those interfaces
> to point directly to the OpenSSL HMAC API, or just give up and drop
> Message-Authenticator support if you didn't build with OpenSSL support
> (in practice everyone does). Better ideas?

Seems reasonable. It probably wouldn't be hard to backport common/hmac.h
either, perhaps in a limited fashion with just md5 support.

Review on v1-0001-Add-simple-test-for-RADIUS-authentication.patch:

> +if ($ENV{PG_TEST_EXTRA} !~ /\bradius\b/)
> +{
> + plan skip_all => 'radius not enabled in PG_TEST_EXTRA';
> +}
> +elsif ($^O eq 'freebsd')
> +{
> + $radiusd = '/usr/local/sbin/radiusd';
> +}
> +elsif ($^O eq 'linux' && -f '/usr/sbin/freeradius')
> +{
> + $radiusd = '/usr/sbin/freeradius';
> +}
> +elsif ($^O eq 'linux')
> +{
> + $radiusd = '/usr/sbin/radiusd';
> +}
> +elsif ($^O eq 'darwin' && -d '/opt/local')
> +{
> + # typical path for MacPorts
> + $radiusd = '/opt/local/sbin/radiusd';
> + $radiusd_prefix = '/opt/local';
> +}
> +elsif ($^O eq 'darwin' && -d '/opt/homebrew')
> +{
> + # typical path for Homebrew on ARM
> + $radiusd = '/opt/homebrew/bin/radiusd';
> + $radiusd_prefix = '/opt/homebrew';
> +}
> +elsif ($^O eq 'darwin' && -d '/usr/local')
> +{
> + # typical path for Homebrew on Intel
> + $radiusd = '/usr/local/bin/radiusd';
> + $radiusd_prefix = '/usr/local';
> +}
> +else
> +{
> + plan skip_all =>
> + "radius tests not supported on $^O or dependencies not installed";
> +}

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.

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

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

(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)

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.

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

(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)

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.

> + /*
> + * 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.

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

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

> + /*
> + * We use the first 16 bytes of the shared secret, zero-padded if too
> + * short, as an HMAC-MD5 key for creating and validating
> + * Message-Authenticator attributes.
> + */
> + memset(message_authenticator_key, 0, lengthof(message_authenticator_key));
> + memcpy(message_authenticator_key,
> + secret,
> + Min(lengthof(message_authenticator_key), strlen(secret)));

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.

> + else if (strcmp(name, "radiusrequirema") == 0)
> + {
> + REQUIRE_AUTH_OPTION(uaRADIUS, "radiusrequirema", "radius");
> + if (strcmp(val, "1") == 0)
> + hbaline->radiusrequirema = true;
> + else
> + hbaline->radiusrequirema = false;
> + }

I was going to suggest throwing an error on any other val than "1" or
"0", but I see that we're doing the same in many other boolean options,
like ldaptls. So I guess that's fine, but would be nice to tighten that
up for all the options as a separate patch.

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

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

--
Heikki Linnakangas
Neon (https://neon.tech)