Thread: BUG #5687: RADIUS Authentication issues
The following bug has been logged online:
Bug reference: 5687
Logged by: Alan DeKok
Email address: aland@freeradius.org
PostgreSQL version: 9.0.0
Operating system: All
Description: RADIUS Authentication issues
Details:
CheckRADIUSAuth() in src/backend/libpq/auth.c is subject to spoofing attacks
which can force all RADIUS authentications to fail.
The current code does (at a high level)
read packet
close socket
if (!verify packet) return STATUS_ERROR
if (success) return STATUS_OK
return STATUS_ERROR
The source IP/port/RADIUS ID && authentication vector fields are checked
*after* the socket is closed. This allows an attacker to "race" the RADIUS
server, and spoof the response, forcing PostgreSQL to treat the
authentication as failed.
The code should instead do something like:
do {
read packet
} while (! verify_packet);
close socket
if (success) return STATUS_OK
return STATUS_ERROR
The "verify packet" code could be moved to a separate function for this
purpose. For similar code, see the rad_verify() function in:
http://github.com/alandekok/freeradius-server/blob/v2.1.x/src/lib/radius.c
"Alan DeKok" <aland@freeradius.org> writes:
> CheckRADIUSAuth() in src/backend/libpq/auth.c is subject to spoofing attacks
> which can force all RADIUS authentications to fail.
> ...
> The source IP/port/RADIUS ID && authentication vector fields are checked
> *after* the socket is closed. This allows an attacker to "race" the RADIUS
> server, and spoof the response, forcing PostgreSQL to treat the
> authentication as failed.
[ scratches head ... ] I don't see the problem. AFAICS the "verify
packet" code is just looking at local storage. Where is the spoofing
possibility, and why would delaying the socket close accomplish
anything?
regards, tom lane
On Sun, Oct 3, 2010 at 00:52, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Alan DeKok" <aland@freeradius.org> writes: >> CheckRADIUSAuth() in src/backend/libpq/auth.c is subject to spoofing att= acks >> which can force all RADIUS authentications to fail. >> ... >> The source IP/port/RADIUS ID && authentication vector fields are checked >> *after* the socket is closed. =A0This allows an attacker to "race" the R= ADIUS >> server, and spoof the response, forcing PostgreSQL to treat the >> authentication as failed. > > [ scratches head ... ] =A0I don't see the problem. =A0AFAICS the "verify > packet" code is just looking at local storage. =A0Where is the spoofing > possibility, and why would delaying the socket close accomplish > anything? I think he's referring to the ability to flood the postgresql server with radius packets with spoofed IP source, correct? If we then looped until we got one that validated as a proper packet, we'd still be able to authenticate with that one, just throwing the invalid ones away. Notice how the "read packet" part is moved inside the loop in his suggestion. --=20 =A0Magnus Hagander =A0Me: http://www.hagander.net/ =A0Work: http://www.redpill-linpro.com/
Tom Lane wrote:
> [ scratches head ... ] I don't see the problem. AFAICS the "verify
> packet" code is just looking at local storage. Where is the spoofing
> possibility, and why would delaying the socket close accomplish
> anything?
Looking at local storage isn't the issue. There is no buffer overflow
or any problem related to local storage.
The issue is that the "verify packet code" treats malformed packets as
failed authentication. Since an attacker can easily send malformed
packets, he can force authentication to fail. The code then stops
looking for a *good* response, so the real "authentication succeeded"
message is ignored.
Similarly, the code checks the signature of the response, as required.
But it also treats "bad signature" as failed authentication. This is
despite the requirements of RFC 2865 Section 4.2 (among others):
... The Response Authenticator field
MUST contain the correct response for the pending Access-Request.
Invalid packets are silently discarded.
i.e. the invalid packet should be ignored, and the socket left open,
so that it can continue to look for the *good* response.
I've attached a set of patches which gradually re-arrange the code so
that it ignores invalid packets, and keeps trying to read a "good"
response until the timer expires. I've also changed the messages saying
"bad packet" from 'errors' to 'warnings'. Being attacked isn't an error. :)
Alan DeKok.
Magnus Hagander wrote: > I think he's referring to the ability to flood the postgresql server > with radius packets with spoofed IP source, correct? Yes. Or, with any number of other "bad" packets. > If we then looped > until we got one that validated as a proper packet, we'd still be able > to authenticate with that one, just throwing the invalid ones away. > Notice how the "read packet" part is moved inside the loop in his > suggestion. The patches from my previous message implements that. Alan DeKok.
Magnus Hagander <magnus@hagander.net> writes:
> On Sun, Oct 3, 2010 at 00:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> [ scratches head ... ] I don't see the problem.
> I think he's referring to the ability to flood the postgresql server
> with radius packets with spoofed IP source, correct?
Hm ... seems to me that is a network security problem, not our problem.
Who's to say one of the spoofed packets won't pass verification?
> If we then looped
> until we got one that validated as a proper packet, we'd still be able
> to authenticate with that one, just throwing the invalid ones away.
> Notice how the "read packet" part is moved inside the loop in his
> suggestion.
If you want to change it, I won't stand in the way, but I have real
doubts about both the credibility of this threat and the usefulness
of the proposed fix.
regards, tom lane
Tom Lane wrote: > Hm ... seems to me that is a network security problem, not our problem. > Who's to say one of the spoofed packets won't pass verification? The packets are signed with a shared key. Passing verification means either the attacker knows the key, or the attacker has broken MD5 in ways that are currently unknown. > If you want to change it, I won't stand in the way, but I have real > doubts about both the credibility of this threat and the usefulness > of the proposed fix. The credibility of the threat is high. Anyone can trivially send a packet which will cause authentication to fail. This is a DoS attack. The usefulness of the fix is to mitigate the threat, and the implement the security features mandated by RFC 2865. It's also how *all* RADIUS implementations work. Alan DeKok.
On Sun, Oct 3, 2010 at 18:30, Alan T DeKok <aland@freeradius.org> wrote: > Tom Lane wrote: >> Hm ... seems to me that is a network security problem, not our problem. >> Who's to say one of the spoofed packets won't pass verification? > > =A0The packets are signed with a shared key. =A0Passing verification means > either the attacker knows the key, or the attacker has broken MD5 in > ways that are currently unknown. > >> If you want to change it, I won't stand in the way, but I have real >> doubts about both the credibility of this threat and the usefulness >> of the proposed fix. > > =A0The credibility of the threat is high. =A0Anyone can trivially send a > packet which will cause authentication to fail. =A0This is a DoS attack. I don't agree about how high it is - unless I misunderstand the wording. You still need to have unfiltered access to the network that the database server is on (unlikely) and you need to guess/bruteforce the port (using bruteforce not really hard, but likely to be detected by an IDS pretty quickly) It is definitely an opportunity for a DoS attack though, so it should be fi= xed. I find your suggested patches kind of hard to read posted inline that way - any chance you can repost as attachment or publish it as a git repository I can fetch from? --=20 =A0Magnus Hagander =A0Me: http://www.hagander.net/ =A0Work: http://www.redpill-linpro.com/
On Tue, Oct 5, 2010 at 11:01, Magnus Hagander <magnus@hagander.net> wrote: > On Sun, Oct 3, 2010 at 18:30, Alan T DeKok <aland@freeradius.org> wrote: >> Tom Lane wrote: >>> Hm ... seems to me that is a network security problem, not our problem. >>> Who's to say one of the spoofed packets won't pass verification? >> >> The packets are signed with a shared key. Passing verification means >> either the attacker knows the key, or the attacker has broken MD5 in >> ways that are currently unknown. >> >>> If you want to change it, I won't stand in the way, but I have real >>> doubts about both the credibility of this threat and the usefulness >>> of the proposed fix. >> >> The credibility of the threat is high. Anyone can trivially send a >> packet which will cause authentication to fail. This is a DoS attack. > > I don't agree about how high it is - unless I misunderstand the > wording. You still need to have unfiltered access to the network that > the database server is on (unlikely) and you need to guess/bruteforce > the port (using bruteforce not really hard, but likely to be detected > by an IDS pretty quickly) > > It is definitely an opportunity for a DoS attack though, so it should be fixed. > > I find your suggested patches kind of hard to read posted inline that > way - any chance you can repost as attachment or publish it as a git > repository I can fetch from? Actually, nevermind that one. Here's a patch I worked up from your description, and that turns out to be fairly similar to yours in what it does I think - except I'm not rearranging the code into a separate function. We already have a while-loop. See attached context diff, and I've also included a diff without whitespace changes since the majority of the diff is otherwise coming from indenting the code one tab... (so far untested, I seem to have deleted my test-instance of the radius server, but I figured I should post my attempt anyway) Also, my patch does not change from log to warning - note that warning is actually *below* log when it comes to the logfile (see log_min_messages comments in postgresql.conf). I keep making that mistake myself... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Attachment
Magnus Hagander wrote: > Actually, nevermind that one. Here's a patch I worked up from your > description, and that turns out to be fairly similar to yours in what > it does I think - except I'm not rearranging the code into a separate > function. We already have a while-loop. Thanks. The only comment I have is that the hard-code 100000 could be USECS_PER_SEC. > See attached context diff, and I've also included a diff without > whitespace changes since the majority of the diff is otherwise coming > from indenting the code one tab... > > (so far untested, I seem to have deleted my test-instance of the > radius server, but I figured I should post my attempt anyway) I can set up a test server if you want. > Also, my patch does not change from log to warning - note that warning > is actually *below* log when it comes to the logfile (see > log_min_messages comments in postgresql.conf). I keep making that > mistake myself... OK. My only interest there was to ensure that a DoS attack wouldn't result in the log being flooded with "invalid packet" messages. Alan DeKok.
Alan T DeKok <aland@freeradius.org> wrote: > the hard-code 100000 could be USECS_PER_SEC. To save others the time of checking, it's actually 1000000 in the patch. -Kevin
On Tue, Oct 5, 2010 at 19:11, Alan T DeKok <aland@freeradius.org> wrote: > Magnus Hagander wrote: >> Actually, nevermind that one. Here's a patch I worked up from your >> description, and that turns out to be fairly similar to yours in what >> it does I think - except I'm not rearranging the code into a separate >> function. We already have a while-loop. > > =A0Thanks. =A0The only comment I have is that the hard-code 100000 could = be > USECS_PER_SEC. That's hardcoded elsewhere in the backend though, and we've not used USECS_PER_SEC anywhere else. So for consistency.. >> See attached context diff, and I've also included a diff without >> whitespace changes since the majority of the diff is otherwise coming >> from indenting the code one tab... >> >> (so far untested, I seem to have deleted my test-instance of the >> radius server, but I figured I should post my attempt anyway) > > =A0I can set up a test server if you want. Nah, I should get mine back up. If you can test the complete patch in your environment (particularly if you already have a "bad packet injector" that you know creates the issue on 9.0), that would be great though. >> Also, my patch does not change from log to warning - note that warning >> is actually *below* log when it comes to the logfile (see >> log_min_messages comments in postgresql.conf). I keep making that >> mistake myself... > > =A0OK. =A0My only interest there was to ensure that a DoS attack wouldn't > result in the log being flooded with "invalid packet" messages. Uh, how exactly does your patch prevent that? --=20 =A0Magnus Hagander =A0Me: http://www.hagander.net/ =A0Work: http://www.redpill-linpro.com/
Magnus Hagander wrote: > If you can test the complete patch in your environment (particularly > if you already have a "bad packet injector" that you know creates the > issue on 9.0), that would be great though. If you use FreeRADIUS, use "radclient" to send the following text: User-Name = "bob" User-Password = "hello" Raw-Attribute = 0x0501 The last bit is a malformed RADIUS attribute. >> OK. My only interest there was to ensure that a DoS attack wouldn't >> result in the log being flooded with "invalid packet" messages. > > Uh, how exactly does your patch prevent that? Hmm.... not so much. Alan DeKok.
Alan T DeKok <aland@freeradius.org> writes: > Magnus Hagander wrote: >> If you can test the complete patch in your environment (particularly >> if you already have a "bad packet injector" that you know creates the >> issue on 9.0), that would be great though. > > If you use FreeRADIUS, use "radclient" to send the following text: > > User-Name = "bob" > User-Password = "hello" > Raw-Attribute = 0x0501 > > The last bit is a malformed RADIUS attribute. Would using this lib help here? http://caca.zoy.org/wiki/zzuf Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Tue, Oct 5, 2010 at 15:18, Magnus Hagander <magnus@hagander.net> wrote: > On Tue, Oct 5, 2010 at 11:01, Magnus Hagander <magnus@hagander.net> wrote: >> On Sun, Oct 3, 2010 at 18:30, Alan T DeKok <aland@freeradius.org> wrote: >>> Tom Lane wrote: >>>> Hm ... seems to me that is a network security problem, not our problem. >>>> Who's to say one of the spoofed packets won't pass verification? >>> >>> =A0The packets are signed with a shared key. =A0Passing verification me= ans >>> either the attacker knows the key, or the attacker has broken MD5 in >>> ways that are currently unknown. >>> >>>> If you want to change it, I won't stand in the way, but I have real >>>> doubts about both the credibility of this threat and the usefulness >>>> of the proposed fix. >>> >>> =A0The credibility of the threat is high. =A0Anyone can trivially send a >>> packet which will cause authentication to fail. =A0This is a DoS attack. >> >> I don't agree about how high it is - unless I misunderstand the >> wording. You still need to have unfiltered access to the network that >> the database server is on (unlikely) and you need to guess/bruteforce >> the port (using bruteforce not really hard, but likely to be detected >> by an IDS pretty quickly) >> >> It is definitely an opportunity for a DoS attack though, so it should be= fixed. >> >> I find your suggested patches kind of hard to read posted inline that >> way - any chance you can repost as attachment or publish it as a git >> repository I can fetch from? > > Actually, nevermind that one. Here's a patch I worked up from your > description, and that turns out to be fairly similar to yours in what > it does I think - except I'm not rearranging the code into a separate > function. We already have a while-loop. > > See attached context diff, and I've also included a diff without > whitespace changes since the majority of the diff is otherwise coming > from indenting the code one tab... > > (so far untested, I seem to have deleted my test-instance of the > radius server, but I figured I should post my attempt anyway) ok, I've run the patch through my tests of both valid and invalid packets, and it seems to work the correct way now. Thus, applied. --=20 =A0Magnus Hagander =A0Me: http://www.hagander.net/ =A0Work: http://www.redpill-linpro.com/