From 1ca10fd167f57fa400355ccb3e9902724d898d7b Mon Sep 17 00:00:00 2001 From: Dmitry Podgorny Date: Sun, 5 Jan 2020 18:46:15 +0200 Subject: [PATCH] Fix SASL SCRAM implementation * Add SCRAM_DIGEST_SIZE macro for maximum possible digest size. It will avoid new buffer overflow errors when new digests are added. * Fix buffer overflow in sasl_scram(). Buffers were allocated for SHA1 digest size. * Fix bug with handler re-registration when a SASL SCRAM mechanism fails. --- src/auth.c | 34 ++++++++++++++++++++++------------ src/sasl.c | 13 +++++++------ src/scram.c | 6 +++--- src/scram.h | 8 ++++++-- 4 files changed, 38 insertions(+), 23 deletions(-) diff --git a/src/auth.c b/src/auth.c index badf469..a5f5acb 100644 --- a/src/auth.c +++ b/src/auth.c @@ -82,9 +82,9 @@ static int _handle_digestmd5_challenge(xmpp_conn_t *const conn, static int _handle_digestmd5_rspauth(xmpp_conn_t *const conn, xmpp_stanza_t *const stanza, void *const userdata); -static int _handle_scram_sha1_challenge(xmpp_conn_t *const conn, - xmpp_stanza_t *const stanza, - void *const userdata); +static int _handle_scram_challenge(xmpp_conn_t *const conn, + xmpp_stanza_t *const stanza, + void *const userdata); static char *_make_scram_init_msg(xmpp_conn_t *const conn); static int _handle_missing_features_sasl(xmpp_conn_t *const conn, @@ -440,18 +440,18 @@ struct scram_user_data { }; /* handle the challenge phase of SCRAM-SHA-1 auth */ -static int _handle_scram_sha1_challenge(xmpp_conn_t *const conn, - xmpp_stanza_t *const stanza, - void *const userdata) +static int _handle_scram_challenge(xmpp_conn_t *const conn, + xmpp_stanza_t *const stanza, + void *const userdata) { char *text; char *response; xmpp_stanza_t *auth; xmpp_stanza_t *authdata; const char *name; - const char *scram_name; char *challenge; struct scram_user_data *scram_ctx = (struct scram_user_data *)userdata; + int rc; name = xmpp_stanza_get_name(stanza); xmpp_debug(conn->ctx, "xmpp", "handle %s (challenge) called for %s", @@ -491,14 +491,24 @@ static int _handle_scram_sha1_challenge(xmpp_conn_t *const conn, xmpp_send(conn, auth); xmpp_stanza_release(auth); + rc = 1; /* Keep handler */ } else { - scram_name = scram_ctx->alg->scram_name; + /* + * Free scram_ctx after calling _handle_sasl_result(). If authentication + * fails, we want to try other mechanism which may be different SCRAM + * mechanism. If we freed scram_ctx before the function, _auth() would + * be able to allocate new scram_ctx object with the same address and + * handler_add() would consider new SCRAM handler as duplicate, because + * current handler is not removed yet. As result, libstrophe wouldn't + * handle incoming challenge stanza. + */ + rc = _handle_sasl_result(conn, stanza, + (void *)scram_ctx->alg->scram_name); xmpp_free(conn->ctx, scram_ctx->scram_init); xmpp_free(conn->ctx, scram_ctx); - return _handle_sasl_result(conn, stanza, (void *)scram_name); } - return 1; + return rc; err_release_auth: xmpp_stanza_release(auth); @@ -694,8 +704,8 @@ static void _auth(xmpp_conn_t *const conn) xmpp_stanza_add_child(auth, authdata); xmpp_stanza_release(authdata); - handler_add(conn, _handle_scram_sha1_challenge, XMPP_NS_SASL, NULL, - NULL, (void *)scram_ctx); + handler_add(conn, _handle_scram_challenge, XMPP_NS_SASL, NULL, NULL, + (void *)scram_ctx); xmpp_send(conn, auth); xmpp_stanza_release(auth); diff --git a/src/sasl.c b/src/sasl.c index a41353a..728eaa7 100644 --- a/src/sasl.c +++ b/src/sasl.c @@ -21,7 +21,6 @@ #include "ostypes.h" #include "sasl.h" #include "md5.h" -#include "sha1.h" #include "scram.h" #include "rand.h" #include "util.h" @@ -379,7 +378,7 @@ char *sasl_digest_md5(xmpp_ctx_t *ctx, return response; } -/** generate auth response string for the SASL SCRAM-SHA-1 mechanism */ +/** generate auth response string for the SASL SCRAM mechanism */ char *sasl_scram(xmpp_ctx_t *ctx, const struct hash_alg *alg, const char *challenge, @@ -387,8 +386,8 @@ char *sasl_scram(xmpp_ctx_t *ctx, const char *jid, const char *password) { - uint8_t key[SHA1_DIGEST_SIZE]; - uint8_t sign[SHA1_DIGEST_SIZE]; + uint8_t key[SCRAM_DIGEST_SIZE]; + uint8_t sign[SCRAM_DIGEST_SIZE]; char *r = NULL; char *s = NULL; char *i = NULL; @@ -439,7 +438,8 @@ char *sasl_scram(xmpp_ctx_t *ctx, goto out_sval; } - response_len = 39 + strlen(r); + /* "c=biws," + r + ",p=" + sign_b64 + '\0' */ + response_len = 7 + strlen(r) + 3 + ((alg->digest_size + 2) / 3 * 4) + 1; response = xmpp_alloc(ctx, response_len); if (!response) { goto out_auth; @@ -454,11 +454,12 @@ char *sasl_scram(xmpp_ctx_t *ctx, SCRAM_ClientSignature(alg, key, (uint8_t *)auth, strlen(auth), sign); SCRAM_ClientProof(alg, sign, key, sign); - sign_b64 = xmpp_base64_encode(ctx, sign, sizeof(sign)); + sign_b64 = xmpp_base64_encode(ctx, sign, alg->digest_size); if (!sign_b64) { goto out_response; } + /* Check for buffer overflow */ if (strlen(response) + strlen(sign_b64) + 3 + 1 > response_len) { xmpp_free(ctx, sign_b64); goto out_response; diff --git a/src/scram.c b/src/scram.c index 7bcc21d..9701e38 100644 --- a/src/scram.c +++ b/src/scram.c @@ -75,7 +75,7 @@ static void crypto_HMAC(const struct hash_alg *alg, uint8_t key_pad[HMAC_BLOCK_SIZE]; uint8_t key_ipad[HMAC_BLOCK_SIZE]; uint8_t key_opad[HMAC_BLOCK_SIZE]; - uint8_t sha_digest[SHA512_DIGEST_SIZE]; + uint8_t sha_digest[SCRAM_DIGEST_SIZE]; int i; union common_hash_ctx ctx; @@ -148,7 +148,7 @@ void SCRAM_ClientKey(const struct hash_alg *alg, uint32_t i, uint8_t *key) { - uint8_t salted[SHA512_DIGEST_SIZE]; + uint8_t salted[SCRAM_DIGEST_SIZE]; /* XXX: Normalize(password) is omitted */ @@ -163,7 +163,7 @@ void SCRAM_ClientSignature(const struct hash_alg *alg, size_t len, uint8_t *sign) { - uint8_t stored[SHA512_DIGEST_SIZE]; + uint8_t stored[SCRAM_DIGEST_SIZE]; alg->hash(ClientKey, alg->digest_size, stored); crypto_HMAC(alg, stored, alg->digest_size, AuthMessage, len, sign); diff --git a/src/scram.h b/src/scram.h index a0049e0..b571396 100644 --- a/src/scram.h +++ b/src/scram.h @@ -1,5 +1,5 @@ /* scram.h - * strophe XMPP client library -- SCRAM-SHA1 helper functions + * strophe XMPP client library -- SCRAM helper functions * * Copyright (C) 2013 Dmitry Podgorny * @@ -10,7 +10,7 @@ */ /** @file - * SCRAM-SHA1 helper functions. + * SCRAM helper functions. */ #ifndef __LIBSTROPHE_SCRAM_H__ @@ -19,6 +19,10 @@ /* make sure the stdint.h types are available */ #include "ostypes.h" +/* Maximum possible digest size. Used for buffers allocation. */ +#include "sha512.h" +#define SCRAM_DIGEST_SIZE SHA512_DIGEST_SIZE + struct hash_alg { const char *scram_name; int mask;