Re: BlastRADIUS mitigation

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)