Removed hack-y generate_state() and verify_state() functions.
authoraland <aland>
Tue, 27 Nov 2007 15:51:54 +0000 (15:51 +0000)
committeraland <aland>
Tue, 27 Nov 2007 15:51:54 +0000 (15:51 +0000)
There's no need for much of what they do, and the timer_expire
config item already takes care of expiring old attributes.

Added instance-specific random pool for EAP.
This also fixes a DoS issue where too many simultaneous calls
to fr_rand() could result in issues...

src/modules/rlm_eap/Makefile.in
src/modules/rlm_eap/mem.c
src/modules/rlm_eap/rlm_eap.c
src/modules/rlm_eap/rlm_eap.h
src/modules/rlm_eap/state.c [deleted file]

index a0c7845..227c3e4 100644 (file)
@@ -3,7 +3,7 @@
 #
 
 TARGET      = @targetname@
-SRCS        = rlm_eap.c eap.c mem.c state.c
+SRCS        = rlm_eap.c eap.c mem.c
 HEADERS     = eap.h rlm_eap.h
 RLM_CFLAGS  = $(INCLTDL) -Ilibeap
 CLIENTLIBS  = libeap/$(LIBPREFIX)freeradius-eap.la
index 516fdd5..37181b1 100644 (file)
@@ -166,6 +166,22 @@ void eaplist_free(rlm_eap_t *inst)
 }
 
 /*
+ *     Return a 32-bit random number.
+ */
+static uint32_t eap_rand(fr_randctx *ctx)
+{
+       uint32_t num;
+
+       num = ctx->randrsl[ctx->randcnt++];
+       if (ctx->randcnt == 256) {
+               ctx->randcnt = 0;
+               fr_isaac(ctx);
+       }
+
+       return num;
+}
+
+/*
  *     Add a handler to the set of active sessions.
  *
  *     Since we're adding it to the list, we guess that this means
@@ -173,7 +189,8 @@ void eaplist_free(rlm_eap_t *inst)
  */
 int eaplist_add(rlm_eap_t *inst, EAP_HANDLER *handler)
 {
-       int             status;
+       int             i, status;
+       uint32_t        lvalue;
        VALUE_PAIR      *state;
 
        rad_assert(handler != NULL);
@@ -183,14 +200,10 @@ int eaplist_add(rlm_eap_t *inst, EAP_HANDLER *handler)
         *      Generate State, since we've been asked to add it to
         *      the list.
         */
-       state = generate_state(handler->request->timestamp);
+       state = pairmake("State", "0x00", T_OP_EQ);
+       if (!state) return 0;
        pairadd(&(handler->request->reply->vps), state);
-
-       /*
-        *      Create a unique 'key' for the handler, based
-        *      on State, Client-IP-Address, and EAP ID.
-        */
-       rad_assert(state->length == EAP_STATE_LEN);
+       state->length = EAP_STATE_LEN;
 
        /*
         *      The time at which this request was made was the time
@@ -199,7 +212,6 @@ int eaplist_add(rlm_eap_t *inst, EAP_HANDLER *handler)
        handler->timestamp = handler->request->timestamp;
        handler->status = 1;
 
-       memcpy(handler->state, state->vp_strvalue, sizeof(handler->state));
        handler->src_ipaddr = handler->request->packet->src_ipaddr;
        handler->eap_id = handler->eap_ds->request->id;
 
@@ -215,6 +227,16 @@ int eaplist_add(rlm_eap_t *inst, EAP_HANDLER *handler)
        pthread_mutex_lock(&(inst->session_mutex));
 
        /*
+        *      Create a completely random state.
+        */
+       for (i = 0; i < 4; i++) {
+               lvalue = eap_rand(&inst->rand_pool);
+               memcpy(state->vp_octets + i * 4, &lvalue, sizeof(lvalue));
+       }
+       state->vp_octets[15] = handler->eap_id;
+       memcpy(handler->state, state->vp_strvalue, sizeof(handler->state));
+
+       /*
         *      Big-time failure.
         */
        status = rbtree_insert(inst->session_tree, handler);
@@ -314,38 +336,24 @@ EAP_HANDLER *eaplist_find(rlm_eap_t *inst, REQUEST *request,
                handler = rbtree_node2data(inst->session_tree, node);
 
                /*
-                *      Check against replays.  The client can re-play
-                *      a State attribute verbatim, so we wish to
-                *      ensure that the attribute falls within the
-                *      valid time window, which is the second at
-                *      which it was sent out.
-                *
-                *      Hmm... I'm not sure that this step is
-                *      necessary, or even that it does anything.
+                *      Delete old handler from the tree.
+                */
+               rbtree_delete(inst->session_tree, node);
+               
+               /*
+                *      And unsplice it from the linked list.
                 */
-               if (verify_state(state, handler->timestamp) != 0) {
-                       handler = NULL;
+               if (handler->prev) {
+                       handler->prev->next = handler->next;
                } else {
-                       /*
-                        *      It's OK, delete it from the tree.
-                        */
-                       rbtree_delete(inst->session_tree, node);
-
-                       /*
-                        *      And unsplice it from the linked list.
-                        */
-                       if (handler->prev) {
-                               handler->prev->next = handler->next;
-                       } else {
-                               inst->session_head = handler->next;
-                       }
-                       if (handler->next) {
-                               handler->next->prev = handler->prev;
-                       } else {
-                               inst->session_tail = handler->prev;
-                       }
-                       handler->prev = handler->next = NULL;
+                       inst->session_head = handler->next;
+               }
+               if (handler->next) {
+                       handler->next->prev = handler->prev;
+               } else {
+                       inst->session_tail = handler->prev;
                }
+               handler->prev = handler->next = NULL;
        }
 
        pthread_mutex_unlock(&(inst->session_mutex));
index 0ca83e5..02a361d 100644 (file)
@@ -94,7 +94,7 @@ static int eap_handler_cmp(const void *a, const void *b)
  */
 static int eap_instantiate(CONF_SECTION *cs, void **instance)
 {
-       int             eap_type;
+       int             i, eap_type;
        int             num_types;
        CONF_SECTION    *scs;
        rlm_eap_t       *inst;
@@ -109,6 +109,14 @@ static int eap_instantiate(CONF_SECTION *cs, void **instance)
                return -1;
        }
 
+       /*
+        *      Create our own random pool.
+        */
+       for (i = 0; i < 256; i++) {
+               inst->rand_pool.randrsl[i] = fr_rand();
+       }
+       fr_randinit(&inst->rand_pool, 1);
+
        /* Load all the configured EAP-Types */
        num_types = 0;
        for(scs=cf_subsection_find_next(cs, NULL, NULL);
index 67ea755..4d0b2aa 100644 (file)
@@ -65,6 +65,8 @@ typedef struct rlm_eap_t {
 #ifdef HAVE_PTHREAD_H
        pthread_mutex_t session_mutex;
 #endif
+
+       fr_randctx      rand_pool;
 } rlm_eap_t;
 
 /*
diff --git a/src/modules/rlm_eap/state.c b/src/modules/rlm_eap/state.c
deleted file mode 100644 (file)
index d5ebb0c..0000000
+++ /dev/null
@@ -1,193 +0,0 @@
-/*
- * state.c  To generate and verify State attribute
- *
- * $Id$
- *
- *   This program is free software; you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY; without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *   GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program; if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
- *
- * Copyright 2001  hereUare Communications, Inc. <raghud@hereuare.com>
- * Copyright 2006  The FreeRADIUS server project
- */
-
-#include <freeradius-devel/ident.h>
-RCSID("$Id$")
-
-#include <fcntl.h>
-#include <stdlib.h>
-#include <string.h>
-#include "rlm_eap.h"
-
-/*
- *     Global key to generate & verify State
- *
- *     This is needed only once per instance of the server,
- *     and putting it in the rlm_eap_t is just too much effort.
- *
- *     Putting it here is ugly, but it works.
- */
-static int key_initialized = 0;
-static unsigned char state_key[AUTH_VECTOR_LEN];
-
-/*
- *     Generate & Verify the State attribute
- *
- *     In the simplest implementation, we would just use the
- *     challenge as state.  Unfortunately, the RADIUS secret protects
- *     only the User-Password attribute; an attacker that can remove
- *     packets from the wire and insert new ones can simply insert a
- *     replayed state without having to know the secret.
- *
- *     However, RADIUS packets containing EAP conversations MUST be
- *     signed with Message-Authenticator, at which point, they MUST
- *     know the secret, in order to get to the EAP module.  And if
- *     they know the secret, they can do many worse things than
- *     re-playing a State attribute.  Their only alternative is to
- *     re-play entire packets, which is caught by the server core.
- *
- *     In any case, we sign our state with data unique to this
- *     specific request.  A NAS would use the Request Authenticator,
- *     we don't know what that will be when the State is returned to
- *     us, so we'll use a time stamp.
- *
- *     Our replay prevention is limited to a time interval
- *     (inst->maxdelay).  We could keep track of all challenges
- *     issued over that time interval, to ensure that the challenges
- *     were unique.  However, they're 8-bytes of data from a good
- *     PRNG, which means that it's pretty damn unlikely that they'll
- *     be re-used.
- *
- *     Our state, then, is (challenge + hmac(challenge + time, key)),
- *     where '+' denotes concatentation, 'challenge' is the octets
- *     of the challenge, 'time' is the 'time_t' in host byte order,
- *     and 'key' is a random key, generated once in eap_init().
- *
- *     This means that only the server which generates a challenge
- *     can verify it; this should be OK if your NAS's load balance
- *     across RADIUS servers by a "first available" algorithm.  If
- *     your NAS's round-robin (ugh), you could use the RADIUS
- *     secret instead, but read RFC 2104 first, and make very sure
- *     you really want to do this.
- */
-void generate_key(void)
-{
-       unsigned int i;
-
-       if (key_initialized) return;
-
-       /*
-        *      Use a cryptographically strong method to generate
-        *      pseudo-random numbers.
-        */
-       for (i = 0; i < sizeof(state_key); i++) {
-               state_key[i] = fr_rand();
-       }
-
-       key_initialized = 1;
-}
-
-/*
- *     For clarity.  Also, to avoid giving away
- *     too much information, we only put 8 octets of the HMAC
- *     into the State attribute, instead of all 16.
- *
- *     As a security feature, it's a little hokey, but WTF.
- *
- *     Also, ensure that EAP_CHALLENGE_LEN + EAP_USE_OF_HMAC = EAP_STATE_LEN
- */
-#define EAP_CHALLENGE_LEN (8)
-#define EAP_HMAC_SIZE (16)
-#define EAP_USE_OF_HMAC (8)
-
-/*
- * Our state, is (challenge + time + hmac(challenge + time, key))
- *
- *  If it's too long, then some clients chop it (sigh)
- */
-VALUE_PAIR *generate_state(time_t timestamp)
-{
-       unsigned int i;
-       unsigned char challenge[EAP_CHALLENGE_LEN];
-       unsigned char hmac[EAP_HMAC_SIZE];
-       unsigned char value[EAP_CHALLENGE_LEN + sizeof(timestamp)];
-       VALUE_PAIR    *state;
-
-       /* Generate challenge (a random value).  */
-       for (i = 0; i < sizeof(challenge); i++) {
-               challenge[i] = fr_rand();
-       }
-
-       memcpy(value, challenge, sizeof(challenge));
-       memcpy(value + sizeof(challenge), &timestamp, sizeof(timestamp));
-
-       /*
-        *      hmac(challenge + timestamp)
-        */
-       fr_hmac_md5(value, sizeof(value),
-                     state_key, sizeof(state_key), hmac);
-
-       /*
-        *      Create the state attribute.
-        *
-        *      Note that the timestamp is used internally, but is NOT
-        *      sent to the client!
-        */
-       state = paircreate(PW_STATE, PW_TYPE_OCTETS);
-       if (state == NULL) {
-               radlog(L_ERR, "rlm_eap: out of memory");
-               return NULL;
-       }
-       memcpy(state->vp_strvalue, challenge, sizeof(challenge));
-       memcpy(state->vp_strvalue + sizeof(challenge), hmac,
-              EAP_USE_OF_HMAC);
-
-       state->length = sizeof(challenge) + EAP_USE_OF_HMAC;
-
-       return state;
-}
-
-/*
- * Returns 0 on success, non-zero otherwise.
- */
-int verify_state(VALUE_PAIR *state, time_t timestamp)
-{
-       unsigned char hmac[EAP_HMAC_SIZE];
-       unsigned char value[EAP_CHALLENGE_LEN + sizeof(timestamp)];
-
-       /*
-        *      The length is wrong.  Don't do anything.
-        */
-       if (state->length != EAP_STATE_LEN) {
-               return -1;
-       }
-
-       /*
-        *      The first 16 octets of the State attribute constains
-        *      the random challenge.
-        */
-       memcpy(value, state->vp_strvalue, EAP_CHALLENGE_LEN);
-       memcpy(value + EAP_CHALLENGE_LEN, &timestamp, sizeof(timestamp));
-
-       /* Generate hmac.  */
-       fr_hmac_md5(value, sizeof(value),
-                     state_key, sizeof(state_key), hmac);
-
-       /*
-        *      Compare the hmac we calculated to the one in the
-        *      packet.
-        */
-       return memcmp(hmac, state->vp_strvalue + EAP_CHALLENGE_LEN,
-                     EAP_USE_OF_HMAC);
-}
-