From cbcaa6a7c8f8a6704f6b4a68f260020957214a07 Mon Sep 17 00:00:00 2001 From: Linus Nordberg Date: Mon, 7 Mar 2011 15:23:40 +0100 Subject: Move verification of response packets up to a level where it makes sense. Replace the user_dispatch_flag on connections with conn_user_dispatch_p(). Remove the 'original' member from packet and instead have an upper layer verify. Rename packet valid_flag --> received_flag to reflect that we don't verify. Move _close_conn() --> conn_close(). Move packet flags into a single unsigned int, for portability. (_read_packet): Don't verify packet. (rs_conn_receive_packet): Don't touch PKT_OUT if there isn't a packet. (rs_conn_receive_packet): Verify packet using packet_verify_response(). --- lib/conn.c | 73 ++++++++++++++++++++++++++++------------ lib/include/radsec/radsec-impl.h | 14 ++++---- lib/packet.c | 39 +++++++++++++++++++-- lib/packet.h | 3 ++ lib/send.c | 17 +++++++--- lib/tcp.c | 69 +++++++++---------------------------- 6 files changed, 125 insertions(+), 90 deletions(-) diff --git a/lib/conn.c b/lib/conn.c index 14693ae..028302c 100644 --- a/lib/conn.c +++ b/lib/conn.c @@ -12,10 +12,35 @@ #include #include #include +#include "conn.h" #include "event.h" +#include "packet.h" #include "tcp.h" int +conn_close (struct rs_connection **connp) +{ + int r; + assert (connp); + assert (*connp); + r = rs_conn_destroy (*connp); + if (!r) + *connp = NULL; + return r; +} + +int +conn_user_dispatch_p (const struct rs_connection *conn) +{ + assert (conn); + + return (conn->callbacks.connected_cb || + conn->callbacks.disconnected_cb || + conn->callbacks.received_cb || + conn->callbacks.sent_cb); +} + +int rs_conn_create (struct rs_context *ctx, struct rs_connection **conn, const char *config) { @@ -158,7 +183,6 @@ void rs_conn_set_callbacks (struct rs_connection *conn, struct rs_conn_callbacks *cb) { assert (conn); - conn->user_dispatch_flag = 1; memcpy (&conn->callbacks, cb, sizeof (conn->callbacks)); } @@ -166,7 +190,6 @@ void rs_conn_del_callbacks (struct rs_connection *conn) { assert (conn); - conn->user_dispatch_flag = 0; memset (&conn->callbacks, 0, sizeof (conn->callbacks)); } @@ -202,7 +225,9 @@ _rcb (struct rs_packet *packet, void *user_data) { struct rs_packet *pkt = (struct rs_packet *) user_data; assert (pkt); - pkt->valid_flag = 1; + assert (pkt->conn); + + pkt->flags |= rs_packet_received_flag; if (pkt->conn->bev) bufferevent_disable (pkt->conn->bev, EV_WRITE|EV_READ); else @@ -216,17 +241,18 @@ _rcb (struct rs_packet *packet, void *user_data) For any other use of libradsec, a the received_cb callback should be registered in the callbacks member of struct rs_connection. - On successful reception, verification and decoding of a RADIUS - message, PKT_OUT will upon return point at a pointer to a struct - rs_packet containing the message. + On successful reception of a RADIUS message it will be verified + against REQ_MSG, if !NULL. - If anything goes wrong or if the read times out (TODO: explain), - PKT_OUT will point at the NULL pointer and one or more errors are - pushed on the connection (available through rs_err_conn_pop()). */ + If PKT_OUT is !NULL it will upon return point at a pointer to a + struct rs_packet containing the message. + If anything goes wrong or if the read times out (TODO: explain), + PKT_OUT will not be changed and one or more errors are pushed on + the connection (available through rs_err_conn_pop()). */ int rs_conn_receive_packet (struct rs_connection *conn, - struct rs_packet *request, + struct rs_packet *req_msg, struct rs_packet **pkt_out) { int err = 0; @@ -234,13 +260,11 @@ rs_conn_receive_packet (struct rs_connection *conn, assert (conn); assert (conn->realm); - assert (!conn->user_dispatch_flag); /* Dispatching mode only. */ + assert (!conn_user_dispatch_p (conn)); /* Dispatching mode only. */ - if (rs_packet_create (conn, pkt_out)) + if (rs_packet_create (conn, &pkt)) return -1; - pkt = *pkt_out; pkt->conn = conn; - pkt->original = request; assert (conn->evb); assert (conn->bev); @@ -249,6 +273,8 @@ rs_conn_receive_packet (struct rs_connection *conn, conn->callbacks.received_cb = _rcb; conn->user_data = pkt; + pkt->flags &= ~rs_packet_received_flag; + if (conn->bev) { bufferevent_setwatermark (conn->bev, EV_READ, RS_HEADER_LEN, 0); @@ -264,7 +290,7 @@ rs_conn_receive_packet (struct rs_connection *conn, evutil_gai_strerror (err)); } - /* Dispatch. */ + rs_debug (("%s: entering event loop\n", __func__)); err = event_base_dispatch (conn->evb); conn->callbacks.received_cb = NULL; @@ -274,13 +300,16 @@ rs_conn_receive_packet (struct rs_connection *conn, evutil_gai_strerror (err)); rs_debug (("%s: event loop done\n", __func__)); - if (!pkt->valid_flag) - return -1; - -#if defined (DEBUG) - rs_dump_packet (pkt); -#endif + if ((pkt->flags & rs_packet_received_flag) == 0 + || (req_msg + && packet_verify_response (pkt->conn, pkt, req_msg) != RSE_OK)) + { + assert (rs_err_conn_peek_code (pkt->conn)); + return -1; + } - pkt->original = NULL; /* FIXME: Why? */ + if (pkt_out) + *pkt_out = pkt; return RSE_OK; } + diff --git a/lib/include/radsec/radsec-impl.h b/lib/include/radsec/radsec-impl.h index f8904ac..e790ccf 100644 --- a/lib/include/radsec/radsec-impl.h +++ b/lib/include/radsec/radsec-impl.h @@ -81,7 +81,6 @@ struct rs_connection { int fd; /* Socket. */ int tryagain; /* For server failover. */ int nextid; /* Next RADIUS packet identifier. */ - int user_dispatch_flag : 1; /* User does the dispatching. */ /* TCP transport specifics. */ struct bufferevent *bev; /* Buffer event. */ /* UDP transport specifics. */ @@ -95,14 +94,17 @@ struct rs_connection { #endif }; +enum rs_packet_flags { + rs_packet_hdr_read_flag, + rs_packet_received_flag, + rs_packet_sent_flag, +}; + struct rs_packet { struct rs_connection *conn; - char hdr_read_flag; - uint8_t hdr[4]; + unsigned int flags; + uint8_t hdr[RS_HEADER_LEN]; RADIUS_PACKET *rpkt; - struct rs_packet *original; - char valid_flag; - char written_flag; struct rs_packet *next; /* Used for UDP output queue. */ }; diff --git a/lib/packet.c b/lib/packet.c index 6ba9fd3..799234f 100644 --- a/lib/packet.c +++ b/lib/packet.c @@ -9,6 +9,7 @@ #include #include #include +#include "conn.h" #include "debug.h" #include "packet.h" @@ -18,15 +19,47 @@ #include #endif -/* Badly named helper function for preparing a RADIUS message and - queue it. FIXME: Rename. */ +int +packet_verify_response (struct rs_connection *conn, + struct rs_packet *response, + struct rs_packet *request) +{ + assert (conn); + assert (conn->active_peer); + assert (conn->active_peer->secret); + assert (response); + assert (response->rpkt); + assert (request); + assert (request->rpkt); + + /* Verify header and message authenticator. */ + if (rad_verify (response->rpkt, request->rpkt, conn->active_peer->secret)) + { + conn_close (&conn); + return rs_err_conn_push_fl (conn, RSE_FR, __FILE__, __LINE__, + "rad_verify: %s", fr_strerror ()); + } + + /* Decode and decrypt. */ + if (rad_decode (response->rpkt, request->rpkt, conn->active_peer->secret)) + { + conn_close (&conn); + return rs_err_conn_push_fl (conn, RSE_FR, __FILE__, __LINE__, + "rad_decode: %s", fr_strerror ()); + } + + return RSE_OK; +} + + +/* Badly named function for preparing a RADIUS message and queue it. + FIXME: Rename. */ int packet_do_send (struct rs_packet *pkt) { VALUE_PAIR *vp = NULL; assert (pkt->rpkt); - assert (!pkt->original); /* Add Message-Authenticator, RFC 2869. */ /* FIXME: Make Message-Authenticator optional? */ diff --git a/lib/packet.h b/lib/packet.h index 7053329..edff9de 100644 --- a/lib/packet.h +++ b/lib/packet.h @@ -2,3 +2,6 @@ See the file COPYING for licensing information. */ int packet_do_send (struct rs_packet *pkt); +int packet_verify_response (struct rs_connection *conn, + struct rs_packet *response, + struct rs_packet *request); diff --git a/lib/send.c b/lib/send.c index 871d657..cc7fd71 100644 --- a/lib/send.c +++ b/lib/send.c @@ -14,9 +14,16 @@ #include "packet.h" #include "event.h" #include "peer.h" +#include "conn.h" #include "tcp.h" #include "udp.h" +/* RFC 5080 2.2.1. Retransmission Behavior */ +#define IRT 2 +#define MRC 5 +#define MRT 16 +#define MRD 30 + static int _conn_open (struct rs_connection *conn, struct rs_packet *pkt) { @@ -62,7 +69,7 @@ _wcb (void *user_data) { struct rs_packet *pkt = (struct rs_packet *) user_data; assert (pkt); - pkt->written_flag = 1; + pkt->flags |= rs_packet_sent_flag; if (pkt->conn->bev) bufferevent_disable (pkt->conn->bev, EV_WRITE|EV_READ); else @@ -100,27 +107,27 @@ rs_packet_send (struct rs_packet *pkt, void *user_data) { err = event_add (conn->wev, NULL); if (err < 0) - return rs_err_conn_push_fl (pkt->conn, RSE_EVENT, __FILE__, __LINE__, + return rs_err_conn_push_fl (conn, RSE_EVENT, __FILE__, __LINE__, "event_add: %s", evutil_gai_strerror (err)); } /* Do dispatch, unless the user wants to do it herself. */ - if (!conn->user_dispatch_flag) + if (!conn_user_dispatch_p (conn)) { conn->callbacks.sent_cb = _wcb; conn->user_data = pkt; rs_debug (("%s: entering event loop\n", __func__)); err = event_base_dispatch (conn->evb); if (err < 0) - return rs_err_conn_push_fl (pkt->conn, RSE_EVENT, __FILE__, __LINE__, + return rs_err_conn_push_fl (conn, RSE_EVENT, __FILE__, __LINE__, "event_base_dispatch: %s", evutil_gai_strerror (err)); rs_debug (("%s: event loop done\n", __func__)); conn->callbacks.sent_cb = NULL; conn->user_data = NULL; - if (!pkt->written_flag) + if ((pkt->flags & rs_packet_sent_flag) == 0) return -1; } diff --git a/lib/tcp.c b/lib/tcp.c index acee94b..1801d62 100644 --- a/lib/tcp.c +++ b/lib/tcp.c @@ -16,6 +16,7 @@ #include #include "tcp.h" #include "packet.h" +#include "conn.h" #include "debug.h" #include "event.h" @@ -41,18 +42,6 @@ _conn_timeout_cb (int fd, short event, void *data) } } -static int -_close_conn (struct rs_connection **connp) -{ - int r; - assert (connp); - assert (*connp); - r = rs_conn_destroy (*connp); - if (!r) - *connp = NULL; - return r; -} - /* Read one RADIUS packet header. Return !0 on error. A return value of 0 means that we need more data. */ static int @@ -63,11 +52,11 @@ _read_header (struct rs_packet *pkt) n = bufferevent_read (pkt->conn->bev, pkt->hdr, RS_HEADER_LEN); if (n == RS_HEADER_LEN) { - pkt->hdr_read_flag = 1; + pkt->flags |= rs_packet_hdr_read_flag; pkt->rpkt->data_len = (pkt->hdr[2] << 8) + pkt->hdr[3]; if (pkt->rpkt->data_len < 20 || pkt->rpkt->data_len > 4096) { - _close_conn (&pkt->conn); + conn_close (&pkt->conn); return rs_err_conn_push (pkt->conn, RSE_INVALID_PKT, "invalid packet length: %d", pkt->rpkt->data_len); @@ -75,7 +64,7 @@ _read_header (struct rs_packet *pkt) pkt->rpkt->data = rs_malloc (pkt->conn->ctx, pkt->rpkt->data_len); if (!pkt->rpkt->data) { - _close_conn (&pkt->conn); + conn_close (&pkt->conn); return rs_err_conn_push_fl (pkt->conn, RSE_NOMEM, __FILE__, __LINE__, NULL); } @@ -91,7 +80,7 @@ _read_header (struct rs_packet *pkt) } else /* Error: libevent gave us less than the low watermark. */ { - _close_conn (&pkt->conn); + conn_close (&pkt->conn); return rs_err_conn_push_fl (pkt->conn, RSE_INTERNAL, __FILE__, __LINE__, "got %d octets reading header", n); } @@ -117,7 +106,7 @@ _read_packet (struct rs_packet *pkt) { bufferevent_disable (pkt->conn->bev, EV_READ); rs_debug (("%s: complete packet read\n", __func__)); - pkt->hdr_read_flag = 0; + pkt->flags &= ~rs_packet_hdr_read_flag; memset (pkt->hdr, 0, sizeof(*pkt->hdr)); /* Checks done by rad_packet_ok: @@ -125,39 +114,13 @@ _read_packet (struct rs_packet *pkt) - invalid code field - attribute lengths >= 2 - attribute sizes adding up correctly */ - if (!rad_packet_ok (pkt->rpkt, 0) != 0) + if (!rad_packet_ok (pkt->rpkt, 0)) { - _close_conn (&pkt->conn); + conn_close (&pkt->conn); return rs_err_conn_push_fl (pkt->conn, RSE_FR, __FILE__, __LINE__, "invalid packet: %s", fr_strerror ()); } - /* TODO: Verify that reception of an unsolicited response packet - results in connection being closed. */ - - /* If we have a request to match this response against, verify - and decode the response. */ - if (pkt->original) - { - /* Verify header and message authenticator. */ - if (rad_verify (pkt->rpkt, pkt->original->rpkt, - pkt->conn->active_peer->secret)) - { - _close_conn (&pkt->conn); - return rs_err_conn_push_fl (pkt->conn, RSE_FR, __FILE__, __LINE__, - "rad_verify: %s", fr_strerror ()); - } - - /* Decode and decrypt. */ - if (rad_decode (pkt->rpkt, pkt->original->rpkt, - pkt->conn->active_peer->secret)) - { - _close_conn (&pkt->conn); - return rs_err_conn_push_fl (pkt->conn, RSE_FR, __FILE__, __LINE__, - "rad_decode: %s", fr_strerror ()); - } - } - #if defined (DEBUG) /* Find out what happens if there's data left in the buffer. */ { @@ -169,8 +132,8 @@ _read_packet (struct rs_packet *pkt) } #endif - /* Hand over message to user, changes ownership of pkt. Don't - touch it afterwards -- it might have been freed. */ + /* Hand over message to user. This changes ownership of pkt. + Don't touch it afterwards -- it might have been freed. */ if (pkt->conn->callbacks.received_cb) pkt->conn->callbacks.received_cb (pkt, pkt->conn->user_data); } @@ -183,14 +146,12 @@ _read_packet (struct rs_packet *pkt) return 0; } -/* Read callback for TCP. +/* The read callback for TCP. Read exactly one RADIUS message from BEV and store it in struct - rs_packet passed in CTX (hereby called 'pkt'). - - Verify the received packet against pkt->original, if !NULL. + rs_packet passed in USER_DATA. - Inform upper layer about successful reception of valid RADIUS + Inform upper layer about successful reception of received RADIUS message by invoking conn->callbacks.recevied_cb(), if !NULL. */ void tcp_read_cb (struct bufferevent *bev, void *user_data) @@ -204,9 +165,9 @@ tcp_read_cb (struct bufferevent *bev, void *user_data) pkt->rpkt->sockfd = pkt->conn->fd; pkt->rpkt->vps = NULL; - if (!pkt->hdr_read_flag) + if ((pkt->flags & rs_packet_hdr_read_flag) == 0) if (_read_header (pkt)) - return; + return; /* Error. */ _read_packet (pkt); } -- cgit v1.1