Skip to content

Commit

Permalink
EAP-pwd: fix DoS due to multithreaded BN_CTX access
Browse files Browse the repository at this point in the history
The EAP-pwd module created one global OpenSSL BN_CTX instance, and
used this instance in all incoming requests. This means that different
threads used the same BN_CTX instance, which can result in a crash.
An adversary can trigger these crashes by concurrently initiating
multiple EAP-pwd handshakes from different clients.

Fix this bug by creating a separate BN_CTX instance for each request.
  • Loading branch information
vanhoefm authored and alandekok committed Nov 9, 2019
1 parent c66137b commit 6b522f8
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 14 deletions.
1 change: 1 addition & 0 deletions src/modules/rlm_eap/types/rlm_eap_pwd/eap_pwd.h
Expand Up @@ -90,6 +90,7 @@ typedef struct _pwd_session_t {
uint8_t *out; /* message to fragment */
size_t out_pos;
size_t out_len;
BN_CTX *bnctx;
EC_GROUP *group;
EC_POINT *pwe;
BIGNUM *order;
Expand Down
24 changes: 12 additions & 12 deletions src/modules/rlm_eap/types/rlm_eap_pwd/rlm_eap_pwd.c
Expand Up @@ -55,8 +55,6 @@ static int mod_detach (void *arg)

inst = (eap_pwd_t *) arg;

if (inst->bnctx) BN_CTX_free(inst->bnctx);

return 0;
}

Expand All @@ -76,11 +74,6 @@ static int mod_instantiate (CONF_SECTION *cs, void **instance)
return -1;
}

if ((inst->bnctx = BN_CTX_new()) == NULL) {
cf_log_err_cs(cs, "Failed to get BN context");
return -1;
}

return 0;
}

Expand All @@ -96,6 +89,7 @@ static int _free_pwd_session (pwd_session_t *session)
EC_POINT_clear_free(session->pwe);
BN_clear_free(session->order);
BN_clear_free(session->prime);
BN_CTX_free(session->bnctx);

return 0;
}
Expand Down Expand Up @@ -217,6 +211,12 @@ static int mod_session_init (void *instance, eap_handler_t *handler)
session->order = NULL;
session->prime = NULL;

session->bnctx = BN_CTX_new();
if (session->bnctx == NULL) {
ERROR("rlm_eap_pwd: Failed to get BN context");
return 0;
}

/*
* The admin can dynamically change the MTU.
*/
Expand Down Expand Up @@ -496,7 +496,7 @@ static int mod_process(void *arg, eap_handler_t *handler)
/*
* compute our scalar and element
*/
if (compute_scalar_element(session, inst->bnctx)) {
if (compute_scalar_element(session, session->bnctx)) {
DEBUG2("failed to compute server's scalar and element");
return 0;
}
Expand All @@ -508,7 +508,7 @@ static int mod_process(void *arg, eap_handler_t *handler)
* element is a point, get both coordinates: x and y
*/
if (!EC_POINT_get_affine_coordinates_GFp(session->group, session->my_element, x, y,
inst->bnctx)) {
session->bnctx)) {
DEBUG2("server point assignment failed");
BN_clear_free(x);
BN_clear_free(y);
Expand Down Expand Up @@ -552,15 +552,15 @@ static int mod_process(void *arg, eap_handler_t *handler)
/*
* process the peer's commit and generate the shared key, k
*/
if (process_peer_commit(session, in, in_len, inst->bnctx)) {
if (process_peer_commit(session, in, in_len, session->bnctx)) {
RDEBUG2("failed to process peer's commit");
return 0;
}

/*
* compute our confirm blob
*/
if (compute_server_confirm(session, session->my_confirm, inst->bnctx)) {
if (compute_server_confirm(session, session->my_confirm, session->bnctx)) {
ERROR("rlm_eap_pwd: failed to compute confirm!");
return 0;
}
Expand Down Expand Up @@ -591,7 +591,7 @@ static int mod_process(void *arg, eap_handler_t *handler)
RDEBUG2("pwd exchange is incorrect: not commit!");
return 0;
}
if (compute_peer_confirm(session, peer_confirm, inst->bnctx)) {
if (compute_peer_confirm(session, peer_confirm, session->bnctx)) {
RDEBUG2("pwd exchange cannot compute peer's confirm");
return 0;
}
Expand Down
2 changes: 0 additions & 2 deletions src/modules/rlm_eap/types/rlm_eap_pwd/rlm_eap_pwd.h
Expand Up @@ -40,8 +40,6 @@
#include <freeradius-devel/modules.h>

typedef struct _eap_pwd_t {
BN_CTX *bnctx;

uint32_t group;
uint32_t fragment_size;
char const *server_id;
Expand Down

0 comments on commit 6b522f8

Please sign in to comment.