Thread: BlastRADIUS mitigation

Hi,

(This discussion started on the security@ list, but was deemed
suitable for the -hackers@ list.)

PostgreSQL supports RADIUS authentication[1].  It's probably not
widely used, and generally any security technology based on MD5 alone
should by now be considered weak and obsolete, but there are a few
reports of users.  Recently, a paper was published[2] showing
successful attacks on a range of RADIUS clients, though no mention of
PostgreSQL.  It's not just an MD5 collision search, it's a more
specific cryptographic weakness in the protocol that allows messages
to be forged in quite practical amounts of CPU time.  We might be
quite lucky though: our hard-coded RADIUS_TIMEOUT = 3s doesn't seem to
be enough time, based on my non-crypto-expert reading of the paper at
least.

Even if we assume that is true, FreeRADIUS 3.2.5 has recently[3]
started spewing scary warnings when PostgreSQL sends requests to it by
default, and advising administrators to adjust the relevant setting
further to just drop unsigned messages.  I assume the commercial
RADIUS implementations will do something like that too, based on
comments about the intended direction and expectations of clients in
an in-development RFC[4].

The attached patch-set adds a basic TAP test for RADIUS
authentication, and then adds a Message-Authenticator attribute to all
outgoing requests (an HMAC-MD5 signature for the whole message that
was already defined by the RFCs but marked optional and often
omitted), and also *optionally* requires a Message-Authenticator to
appear in responses from the RADIUS server, and verifies it if
present.  With the change, FreeRADIUS 3.2.5 is happy to talk to
PostgreSQL again.

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?

It's quite easy to test this locally, if you have FreeRADIUS installed
on any Unixoid system.  See the TAP test for some minimal
configuration files required to try it out.

I had originally developed the TAP test as part of a larger project[5]
really about something else, which is why it has Michael listed as a
reviewer already, and this version incorporates some new improvements
he recommended (thanks!).  I've created this new thread and new
minimal test just to deal with the BlastRADIUS mitigation topic.

We might also consider just dropping RADIUS support in 18, if we don't
get patches to bring it up to date with modern RADIUS recommendations
beyond the mitigation (deprecating UDP, adding TLS, probably more
things).  Such patches would ideally be written by someone with a more
direct interest in the protocol (ie I am not volunteering).  But even
if we decide to drop it instead.  I think we'd still want the change
I'm proposing here for released branches.

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?

[1] https://www.postgresql.org/docs/current/auth-radius.html
[2] https://www.blastradius.fail/
[3] https://github.com/FreeRADIUS/freeradius-server/commit/6616be90346beb6050446bd00c8ed5bca1b8ef29
[4] https://datatracker.ietf.org/doc/draft-ietf-radext-deprecating-radius/
[5]
https://www.postgresql.org/message-id/flat/CA%2BhUKGKxNoVjkMCksnj6z3BwiS3y2v6LN6z7_CisLK%2Brv%2B0V4g%40mail.gmail.com

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)

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> 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.

We should think about that not in terms of the situation today,
but the situation when we ship this fix, possibly as much as
three months from now.  (There was some mention in the security-list
discussion of maybe making an off-cycle release to get this out
sooner; but nothing was decided, and I doubt we'll do that unless
we start getting user complaints.)  It seems likely to me that
most up-to-date systems will have BlastRADIUS mitigation in place
by then, so maybe we should lean towards secure-by-default.

We don't necessarily have to make that decision today, either.
We could start with not-secure-by-default but reconsider
whenever the release is imminent.

            regards, tom lane

On Tue, Aug 6, 2024 at 2:41 AM Heikki Linnakangas <hlinnaka@iki.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.

On Mon, Aug 05, 2024 at 05:41:21PM +0300, Heikki Linnakangas wrote:
> On 05/08/2024 15:43, Thomas Munro wrote:
>> 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.

It's a bit more than just backporting hmac.h and hmac.c.
hmac_openssl.c only depends on OpenSSL to do its business, but the
non-OpenSSL fallback implementation depends also on the cryptohash
fallbacks for SHA-NNN and MD5.  So you would also need the parts
related to cryptohash.c, sha{1,2}.c, etc.  Not really complex as these
could be dropped as-is into the stable branches of 12 and 13, but not
that straight-forward either as we had the bad idea to use the
fallback MD5 implementation even if linking to OpenSSL in v12 and v13,
meaning that you may need some tweaks to avoid API conflicts.

Requiring OpenSSL and its HMAC APIs to do the job is much safer for a
stable branch, IMO.
--
Michael

On 06/08/2024 03:58, Thomas Munro wrote:
> On Tue, Aug 6, 2024 at 2:41 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> 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?

Hmm, it does feel sloppy to not verify the format of the rest of the 
attributes. That's not new with this patch though. Maybe have a separate 
function to verify the packet's format, and call that before 
radius_find_attribute().

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

This would also be a easy target for fuzz testing. I'm not too worried 
though, the packet format is pretty simple. Still, bugs happen. (Not a 
requirement for this patch in any case)

> +my $radius_port = PostgreSQL::Test::Cluster::get_free_port();

This isn't quite right because get_free_port() finds a free TCP port, 
while radius uses UDP.

> +#else
> +            ereport(elevel,
> +                    (errcode(ERRCODE_CONFIG_FILE_ERROR),
> +                     errmsg("this build does not support radiusrequirema=1"),
> +                     errcontext("line %d of configuration file \"%s\"",
> +                                line_num, file_name)));
> +#endif

Maybe something like:

   errmsg("radiusrequirema=1 is not supported because the server was 
built without OpenSSL")

to give the user a hint what they need to do to enable it.

Other than those little things, looks good to me.

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

On Wed, Aug 7, 2024 at 5:55 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 06/08/2024 03:58, Thomas Munro wrote:
> > On Tue, Aug 6, 2024 at 2:41 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >> 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.

There's a similar table near the end of 2869:

    https://datatracker.ietf.org/doc/html/rfc2869#section-5.19

> > 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.
>
> This would also be a easy target for fuzz testing. I'm not too worried
> though, the packet format is pretty simple. Still, bugs happen. (Not a
> requirement for this patch in any case)

<tangent>

I've been working on fuzzing JSON, and I spent some time adapting that
to test this RADIUS code. No pressure to use any of it (the
refactoring to pull out the response validation is cowboy-quality at
best), but it might help anyone who wants to pursue it in the future?

This fuzzer hasn't been able to find anything in the response parser.
(But the modifications I made make that claim a little weaker, since
I'm not testing what's actually shipping.) The attached response
corpus could be used to seed a malformed-packet gallery like Thomas
mentioned; it's built with the assumption that the request packet was
all zeroes and the shared secret is `secret`.

I was impressed with how quickly LLVM was able to find the packet
shape, including valid signatures. The only thing I had to eventually
add manually was a valid Message-Authenticator case; I assume the
interdependency between the authenticator and the packet checksum was
a little too much for libfuzzer to figure out on its own.

</tangent>

--Jacob