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.
>From 4d710886f380359b461f6a767fd4b7099f545cce Mon Sep 17 00:00:00 2001
From: Alan T. DeKok <al...@freeradius.org>
Date: Thu, 30 Sep 2010 18:18:01 +0200
Subject: [PATCH 1/4] Move RADIUS verify checks to a common function
This is in preparation for treating bad packets as errors to ignore,
rather than treating them as rejected authentication. If bad packets
are treated as reject, then an attacker can force authentication
failure by flooding the port with fake replies.
---
src/backend/libpq/auth.c | 126 +++++++++++++++++++++++++--------------------
1 files changed, 70 insertions(+), 56 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 5d82ad1..9b8ce12 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2592,6 +2592,71 @@ radius_add_attribute(radius_packet *packet, uint8 type,
const unsigned char *dat
packet->length += attr->length;
}
+static int radius_verify(radius_packet *packet, radius_packet *receivepacket,
+ int packetlength, Port *port)
+{
+ uint8 *cryptvector;
+ uint8 calculated_vector[RADIUS_VECTOR_LENGTH];
+
+ if (packetlength < RADIUS_HEADER_LENGTH)
+ {
+ ereport(LOG,
+ (errmsg("RADIUS response too short: %i",
packetlength)));
+ return STATUS_ERROR;
+ }
+
+ if (packetlength != ntohs(receivepacket->length))
+ {
+ ereport(LOG,
+ (errmsg("RADIUS response has corrupt length: %i (actual length
%i)",
+ ntohs(receivepacket->length), packetlength)));
+ return STATUS_ERROR;
+ }
+
+ if (packet->id != receivepacket->id)
+ {
+ ereport(LOG,
+ (errmsg("RADIUS response is to a different
request: %i (should be %i)",
+ receivepacket->id,
packet->id)));
+ return STATUS_ERROR;
+ }
+
+ /*
+ * Verify the response authenticator, which is calculated as
+ * MD5(Code+ID+Length+RequestAuthenticator+Attributes+Secret)
+ */
+ cryptvector = palloc(packetlength + strlen(port->hba->radiussecret));
+
+ memcpy(cryptvector, receivepacket, 4); /* code+id+length */
+ memcpy(cryptvector + 4, packet->vector, RADIUS_VECTOR_LENGTH);
/* request
+
*
authenticator, from
+
* original
packet */
+ if (packetlength > RADIUS_HEADER_LENGTH) /* there may be no
attributes
+
* at all */
+ memcpy(cryptvector + RADIUS_HEADER_LENGTH, receivepacket + 1,
packetlength - RADIUS_HEADER_LENGTH);
+ memcpy(cryptvector + packetlength, port->hba->radiussecret,
strlen(port->hba->radiussecret));
+
+ if (!pg_md5_binary(cryptvector,
+ packetlength +
strlen(port->hba->radiussecret),
+ calculated_vector))
+ {
+ ereport(LOG,
+ (errmsg("could not perform MD5 encryption of received
packet")));
+ pfree(cryptvector);
+ return STATUS_ERROR;
+ }
+ pfree(cryptvector);
+
+ if (memcmp(receivepacket->vector, calculated_vector,
RADIUS_VECTOR_LENGTH) != 0)
+ {
+ ereport(LOG,
+ (errmsg("RADIUS response has incorrect MD5
signature")));
+ return STATUS_ERROR;
+ }
+
+ return STATUS_OK;
+}
+
static int
CheckRADIUSAuth(Port *port)
{
@@ -2605,6 +2670,7 @@ CheckRADIUSAuth(Port *port)
uint8 *cryptvector;
uint8 encryptedpassword[RADIUS_VECTOR_LENGTH];
int packetlength;
+ int verify_result;
pgsocket sock;
#ifdef HAVE_IPV6
@@ -2841,62 +2907,10 @@ CheckRADIUSAuth(Port *port)
return STATUS_ERROR;
}
- if (packetlength < RADIUS_HEADER_LENGTH)
- {
- ereport(LOG,
- (errmsg("RADIUS response too short: %i",
packetlength)));
- return STATUS_ERROR;
- }
-
- if (packetlength != ntohs(receivepacket->length))
- {
- ereport(LOG,
- (errmsg("RADIUS response has corrupt length: %i (actual length
%i)",
- ntohs(receivepacket->length), packetlength)));
- return STATUS_ERROR;
- }
-
- if (packet->id != receivepacket->id)
- {
- ereport(LOG,
- (errmsg("RADIUS response is to a different
request: %i (should be %i)",
- receivepacket->id,
packet->id)));
- return STATUS_ERROR;
- }
-
- /*
- * Verify the response authenticator, which is calculated as
- * MD5(Code+ID+Length+RequestAuthenticator+Attributes+Secret)
- */
- cryptvector = palloc(packetlength + strlen(port->hba->radiussecret));
-
- memcpy(cryptvector, receivepacket, 4); /* code+id+length */
- memcpy(cryptvector + 4, packet->vector, RADIUS_VECTOR_LENGTH);
/* request
-
*
authenticator, from
-
* original
packet */
- if (packetlength > RADIUS_HEADER_LENGTH) /* there may be no
attributes
-
* at all */
- memcpy(cryptvector + RADIUS_HEADER_LENGTH, receive_buffer +
RADIUS_HEADER_LENGTH, packetlength - RADIUS_HEADER_LENGTH);
- memcpy(cryptvector + packetlength, port->hba->radiussecret,
strlen(port->hba->radiussecret));
-
- if (!pg_md5_binary(cryptvector,
- packetlength +
strlen(port->hba->radiussecret),
- encryptedpassword))
- {
- ereport(LOG,
- (errmsg("could not perform MD5 encryption of received
packet")));
- pfree(cryptvector);
- return STATUS_ERROR;
- }
- pfree(cryptvector);
-
- if (memcmp(receivepacket->vector, encryptedpassword,
RADIUS_VECTOR_LENGTH) != 0)
- {
- ereport(LOG,
- (errmsg("RADIUS response has incorrect MD5
signature")));
- return STATUS_ERROR;
- }
-
+ verify_result = radius_verify(packet, receivepacket, packetlength,
port);
+ if (verify_result != STATUS_OK)
+ return verify_result;
+
if (receivepacket->code == RADIUS_ACCESS_ACCEPT)
return STATUS_OK;
else if (receivepacket->code == RADIUS_ACCESS_REJECT)
--
1.6.4.2
>From f989b717a7c2389c07b488405b8e13b7136a6b2f Mon Sep 17 00:00:00 2001
From: Alan T. DeKok <al...@freeradius.org>
Date: Fri, 1 Oct 2010 09:18:29 +0200
Subject: [PATCH 2/4] Move read / verify RADIUS packet code into the loop.
This means that for the most part, it will keep reading packets
until it receives a valid response. There are still situations
where it will return errors, but those will be fixed in later commits.
---
src/backend/libpq/auth.c | 53 ++++++++++++++++++++++-----------------------
1 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 9b8ce12..4c2d016 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2849,7 +2849,7 @@ CheckRADIUSAuth(Port *port)
FD_ZERO(&fdset);
FD_SET(sock, &fdset);
- while (true)
+ do
{
r = select(sock + 1, &fdset, NULL, NULL, &timeout);
if (r < 0)
@@ -2871,43 +2871,42 @@ CheckRADIUSAuth(Port *port)
return STATUS_ERROR;
}
- /* else we actually have a packet ready to read */
- break;
- }
-
- /* Read the response packet */
- addrsize = sizeof(remoteaddr);
- packetlength = recvfrom(sock, receive_buffer, RADIUS_BUFFER_SIZE, 0,
- (struct sockaddr *) &
remoteaddr, &addrsize);
- if (packetlength < 0)
- {
- ereport(LOG,
+ /* Read the response packet */
+ addrsize = sizeof(remoteaddr);
+ packetlength = recvfrom(sock, receive_buffer,
RADIUS_BUFFER_SIZE, 0,
+ (struct sockaddr *) & remoteaddr,
&addrsize);
+ if (packetlength < 0)
+ {
+ ereport(LOG,
(errmsg("could not read RADIUS response: %m")));
- closesocket(sock);
- return STATUS_ERROR;
- }
-
- closesocket(sock);
+ closesocket(sock);
+ return STATUS_ERROR;
+ }
#ifdef HAVE_IPV6
- if (remoteaddr.sin6_port != htons(port->hba->radiusport))
+ if (remoteaddr.sin6_port != htons(port->hba->radiusport))
#else
- if (remoteaddr.sin_port != htons(port->hba->radiusport))
+ if (remoteaddr.sin_port != htons(port->hba->radiusport))
#endif
- {
+ {
#ifdef HAVE_IPV6
- ereport(LOG,
- (errmsg("RADIUS response was sent from
incorrect port: %i",
+ ereport(LOG,
+ (errmsg("RADIUS response was sent from
incorrect port: %i",
ntohs(remoteaddr.sin6_port))));
#else
- ereport(LOG,
- (errmsg("RADIUS response was sent from
incorrect port: %i",
+ ereport(LOG,
+ (errmsg("RADIUS response was sent from
incorrect port: %i",
ntohs(remoteaddr.sin_port))));
#endif
- return STATUS_ERROR;
- }
+ return STATUS_ERROR;
+ }
+
+ } while ((verify_result = radius_verify(packet,
+ receivepacket, packetlength,
+ port)) != STATUS_OK);
+
+ closesocket(sock);
- verify_result = radius_verify(packet, receivepacket, packetlength,
port);
if (verify_result != STATUS_OK)
return verify_result;
--
1.6.4.2
>From 596e55bc24d9fdc77353837641c48cb53e17e7b2 Mon Sep 17 00:00:00 2001
From: Alan T. DeKok <al...@freeradius.org>
Date: Fri, 1 Oct 2010 09:22:32 +0200
Subject: [PATCH 3/4] Turn errors into warnings for invalid RADIUS packets
We should log a warning for bad packets, as they may come from an
attacker. But they are not errors.
---
src/backend/libpq/auth.c | 38 +++++++++++++++++++-------------------
1 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 4c2d016..805db0e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2600,24 +2600,24 @@ static int radius_verify(radius_packet *packet,
radius_packet *receivepacket,
if (packetlength < RADIUS_HEADER_LENGTH)
{
- ereport(LOG,
- (errmsg("RADIUS response too short: %i",
packetlength)));
+ elog(WARNING,
+ "RADIUS response too short: %i", packetlength);
return STATUS_ERROR;
}
if (packetlength != ntohs(receivepacket->length))
{
- ereport(LOG,
- (errmsg("RADIUS response has corrupt length: %i (actual length
%i)",
- ntohs(receivepacket->length), packetlength)));
+ elog(WARNING,
+ "RADIUS response has corrupt length: %i (actual length
%i)",
+ ntohs(receivepacket->length), packetlength);
return STATUS_ERROR;
}
if (packet->id != receivepacket->id)
{
- ereport(LOG,
- (errmsg("RADIUS response is to a different
request: %i (should be %i)",
- receivepacket->id,
packet->id)));
+ elog(WARNING,
+ "RADIUS response is to a different request: %i (should be
%i)",
+ receivepacket->id, packet->id);
return STATUS_ERROR;
}
@@ -2640,8 +2640,8 @@ static int radius_verify(radius_packet *packet,
radius_packet *receivepacket,
packetlength +
strlen(port->hba->radiussecret),
calculated_vector))
{
- ereport(LOG,
- (errmsg("could not perform MD5 encryption of received
packet")));
+ elog(WARNING,
+ "could not perform MD5 encryption of received packet");
pfree(cryptvector);
return STATUS_ERROR;
}
@@ -2649,8 +2649,8 @@ static int radius_verify(radius_packet *packet,
radius_packet *receivepacket,
if (memcmp(receivepacket->vector, calculated_vector,
RADIUS_VECTOR_LENGTH) != 0)
{
- ereport(LOG,
- (errmsg("RADIUS response has incorrect MD5
signature")));
+ elog(WARNING,
+ "RADIUS response has incorrect MD5 signature");
return STATUS_ERROR;
}
@@ -2890,15 +2890,15 @@ CheckRADIUSAuth(Port *port)
#endif
{
#ifdef HAVE_IPV6
- ereport(LOG,
- (errmsg("RADIUS response was sent from
incorrect port: %i",
- ntohs(remoteaddr.sin6_port))));
+ elog(WARNING,
+ "RADIUS response was sent from incorrect
port: %i",
+ ntohs(remoteaddr.sin6_port));
#else
- ereport(LOG,
- (errmsg("RADIUS response was sent from
incorrect port: %i",
- ntohs(remoteaddr.sin_port))));
+ elog(WARNING,
+ "RADIUS response was sent from incorrect
port: %i",
+ ntohs(remoteaddr.sin_port));
#endif
- return STATUS_ERROR;
+ continue;
}
} while ((verify_result = radius_verify(packet,
--
1.6.4.2
>From 79c0326eb8e17564d07ff61552fcfc016b82f67d Mon Sep 17 00:00:00 2001
From: Alan T. DeKok <al...@freeradius.org>
Date: Fri, 1 Oct 2010 09:39:44 +0200
Subject: [PATCH 4/4] Recalculate RADIUS select() timeout if we receive a bad
packet.
we wait for RADIUS_TIMEOUT after sending one packet, and we do not
re-transmit the packet. See RFC 5080, Section 2.2.1 for a recommended
retransmission algorithm.
---
src/backend/libpq/auth.c | 48 ++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 805db0e..accc26b 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2657,6 +2657,25 @@ static int radius_verify(radius_packet *packet,
radius_packet *receivepacket,
return STATUS_OK;
}
+static void tv_sub(struct timeval *end, struct timeval *start,
+ struct timeval *elapsed)
+{
+ elapsed->tv_sec = end->tv_sec - start->tv_sec;
+ if (elapsed->tv_sec > 0) {
+ elapsed->tv_sec--;
+ elapsed->tv_usec = USECS_PER_SEC;
+ } else {
+ elapsed->tv_usec = 0;
+ }
+ elapsed->tv_usec += end->tv_usec;
+ elapsed->tv_usec -= start->tv_usec;
+
+ if (elapsed->tv_usec >= USECS_PER_SEC) {
+ elapsed->tv_usec -= USECS_PER_SEC;
+ elapsed->tv_sec++;
+ }
+}
+
static int
CheckRADIUSAuth(Port *port)
{
@@ -2685,7 +2704,7 @@ CheckRADIUSAuth(Port *port)
char portstr[128];
ACCEPT_TYPE_ARG3 addrsize;
fd_set fdset;
- struct timeval timeout;
+ struct timeval timeout, now, endtime;
int i,
r;
@@ -2843,14 +2862,31 @@ CheckRADIUSAuth(Port *port)
/* Don't need the server address anymore */
pg_freeaddrinfo_all(hint.ai_family, serveraddrs);
- /* Wait for a response */
- timeout.tv_sec = RADIUS_TIMEOUT;
- timeout.tv_usec = 0;
- FD_ZERO(&fdset);
- FD_SET(sock, &fdset);
+ /* Find out when we need to stop */
+ gettimeofday(&endtime, NULL);
+ endtime.tv_sec += RADIUS_TIMEOUT;
do
{
+ /* Wait for a response */
+ gettimeofday(&now, NULL);
+
+ /* the select may have finished just before the timeout */
+ if ((now.tv_sec > endtime.tv_sec) ||
+ ((now.tv_sec == endtime.tv_sec) &&
+ (now.tv_usec > endtime.tv_usec)))
+ {
+ ereport(LOG,
+ (errmsg("timeout waiting for RADIUS
response")));
+ closesocket(sock);
+ return STATUS_ERROR;
+ }
+
+ /* calculate how long we need to wait before "end time" */
+ tv_sub(&endtime, &now, &timeout);
+ FD_ZERO(&fdset);
+ FD_SET(sock, &fdset);
+
r = select(sock + 1, &fdset, NULL, NULL, &timeout);
if (r < 0)
{
--
1.6.4.2
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
