Skip to content

Commit

Permalink
jwt_decode_2(): Security vulnerability for alg:none
Browse files Browse the repository at this point in the history 
This function had faulty logic based on some assumptions that it could
trust the JWT in that if it was alg:none, it would not run the callback.

The assumption would allow an attacker to modify the JWT header and body
and trick the function into returning without having retrieved a key
from the cb.

The caller of jwt_decode_2 has no real way to know that their cb was
never run.

New logic runs the callback ALWAYS. This way the key_provider is
guaranteed to have been called if jwt_decode_2 returns with a successful
jwt_t object and 0 return code.

As an aside, making this change found that some of our test cases were
assuming that you could call jwt_decode_2 with key_provider of NULL.
This doesn't make much sense, considering there's no way to pass a key
without a key_provider.

In this instance, if passed a JWT with alg:none, this was fine. If
called with any other alg type, the code would attempt to run the
NULL key_provider and produce a SEGV.

RESOLUTION:
- jwt_decode_2 will always run the key_provider assuming there was not a
  previous error (jwt_decode_2() ==  means it ran).
- Always check key_provider for NULL before using it
- If no key_provider, but JWT had alg!=none, processing fails

NOTES:
- jwt_decode() and jwt_decode_2() are being deprecated in favor more
  robust functionality.

Signed-off-by: Ben Collins <[email protected]>
  • Loading branch information
@benmcollins
benmcollins committed Dec 21, 2024
1 parent 2863d88 commit 096082e
Show file tree
Hide file tree
Showing 2 changed files with 177 additions and 55 deletions.
99 changes: 67 additions & 32 deletions libjwt/jwt.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,9 @@ int jwt_set_alg(jwt_t *jwt, jwt_alg_t alg, const unsigned char *key, int len)

jwt_alg_t jwt_get_alg(const jwt_t *jwt)
{
if (jwt == NULL)
return JWT_ALG_INVAL;

return jwt->alg;
}

Expand Down Expand Up @@ -665,23 +668,27 @@ static int jwt_parse_head(jwt_t *jwt, char *head)
return 0;
}

static int jwt_verify_head(jwt_t *jwt)
/**
* @brief Smoke test to save the user from themselves.
*/
static int jwt_verify_alg(jwt_t *jwt)
{
int ret = 0;

if (jwt->alg != JWT_ALG_NONE) {
if (jwt->key) {
if (jwt->key_len <= 0)
ret = EINVAL;
} else {
jwt_scrub_key(jwt);
}
} else {
/* If alg is NONE, there should not be a key */
if (jwt->key)
if (jwt->alg == JWT_ALG_NONE) {
/* If the user gave us a key but the JWT has alg = none,
* then we shouldn't even proceed. */
if (jwt->key || jwt->key_len)
ret = EINVAL;
} else if (!(jwt->key && (jwt->key_len > 0))) {
/* If alg != none, then we should have a key to use */
ret = EINVAL;
}

/* Releive ourselves of the burden of this secret. */
if (ret)
jwt_scrub_key(jwt);

return ret;
}

Expand Down Expand Up @@ -745,13 +752,21 @@ static int jwt_copy_key(jwt_t *jwt, const unsigned char *key, int key_len)
{
int ret = 0;

if (key_len) {
jwt->key = jwt_malloc(key_len);
if (jwt->key == NULL)
return ENOMEM;
memcpy(jwt->key, key, key_len);
jwt->key_len = key_len;
}
if (!key_len)
return 0;

/* Always allocate one extra byte. For PEM, it ensures against
* not having a nil at the end (although all crypto backends
* should honor length), and for binary keys, it wont hurt
* because we use key_len for those operations. */
jwt->key = jwt_malloc(key_len + 1);
if (jwt->key == NULL)
return ENOMEM;

jwt->key[key_len] = '\0';

memcpy(jwt->key, key, key_len);
jwt->key_len = key_len;

return ret;
}
Expand All @@ -762,21 +777,19 @@ static int jwt_decode_complete(jwt_t **jwt, const unsigned char *key, int key_le
int ret = EINVAL;
jwt_t *new = *jwt;

/* Copy the key over for verify_head. */
/* Copy the key over for verify_alg. */
ret = jwt_copy_key(new, key, key_len);
if (ret)
goto decode_done;

ret = jwt_verify_head(new);
ret = jwt_verify_alg(new);
if (ret)
goto decode_done;

/* Check the signature, if needed. */
if (new->alg != JWT_ALG_NONE) {
const char *sig = token + (payload_len + 1);
ret = jwt_verify(new, token, payload_len, sig);
} else {
ret = 0;
}

decode_done:
Expand All @@ -794,6 +807,9 @@ int jwt_decode(jwt_t **jwt, const char *token, const unsigned char *key,
int ret;
unsigned int payload_len;

if (jwt == NULL)
return EINVAL;

if ((ret = jwt_parse(jwt, token, &payload_len)))
return ret;

Expand All @@ -805,22 +821,41 @@ int jwt_decode_2(jwt_t **jwt, const char *token, jwt_key_p_t key_provider)
int ret;
unsigned int payload_len;
jwt_key_t key = { NULL, 0 };
jwt_t *new;
jwt_t *new = NULL;

ret = jwt_parse(jwt, token, &payload_len);
if (jwt == NULL)
return EINVAL;
*jwt = NULL;

ret = jwt_parse(&new, token, &payload_len);
if (ret)
return ret;
new = *jwt;

/* Obtain the key. */
if (new->alg != JWT_ALG_NONE) {
if ((ret = key_provider(new, &key))) {
jwt_free(new);
*jwt = NULL;
return ret;
}
if (key_provider) {
/* The previous code trusted the JWT alg too much. If it was
* NONE, then it wouldn't even bother calling the cb.
*
* We also had some test cases that called this func with no
* key_provider and exptected it to work. True, this code
* allowed for that. My gut tells me that should never have
* been the case.
*
* For one, the previous code didn't check for NULL, so if
* you got a key that wasn't alg == none, instant SEGV.
*
* However, since this func is getting deprecated, we'll
* just let that case be like calling jwt_decode()
*/
ret = key_provider(new, &key);
}

if (ret) {
jwt_free(new);
return ret;
}

*jwt = new;

return jwt_decode_complete(jwt, key.jwt_key, key.jwt_key_len, token, payload_len);
}

Expand Down
133 changes: 110 additions & 23 deletions tests/jwt_new.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,41 @@ START_TEST(test_jwt_decode)
}
END_TEST

START_TEST(test_jwt_decode_2)
static int jwt_decode_2_cb_done;

static int test_jwt_decode_2_alg_none_cb(const jwt_t *jwt, jwt_key_t *key)
{
jwt_alg_t alg = jwt_get_alg(jwt);

jwt_decode_2_cb_done = 1;

return (alg == JWT_ALG_NONE) ? 0 : -1;
}

START_TEST(test_jwt_decode_2_alg_none)
{
const char token[] = "eyJhbGciOiJub25lIn0.eyJpc3MiOiJmaWxlcy5jeXBo"
"cmUuY29tIiwic3ViIjoidXNlcjAifQ.";
jwt_alg_t alg;
jwt_t *jwt;
int ret;

SET_OPS();

jwt_decode_2_cb_done = 0;

ret = jwt_decode_2(&jwt, token, test_jwt_decode_2_alg_none_cb);
ck_assert_int_ne(jwt_decode_2_cb_done, 0);
ck_assert_int_eq(ret, 0);
ck_assert_ptr_nonnull(jwt);

alg = jwt_get_alg(jwt);
ck_assert_int_eq(alg, JWT_ALG_NONE);

jwt_free(jwt);
}

START_TEST(test_jwt_decode_2_compat)
{
const char token[] = "eyJhbGciOiJub25lIn0.eyJpc3MiOiJmaWxlcy5jeXBo"
"cmUuY29tIiwic3ViIjoidXNlcjAifQ.";
Expand All @@ -136,7 +170,7 @@ START_TEST(test_jwt_decode_2)
ck_assert_ptr_nonnull(jwt);

alg = jwt_get_alg(jwt);
ck_assert(alg == JWT_ALG_NONE);
ck_assert_int_eq(alg, JWT_ALG_NONE);

jwt_free(jwt);
}
Expand Down Expand Up @@ -178,19 +212,80 @@ START_TEST(test_jwt_decode_invalid_alg)
}
END_TEST

/* { "typ": "JWT", "alg": "none" } . . */
START_TEST(test_jwt_decode_dot_dot)
{
char token[] = "eyJ0eXAiOiJKV1QiLCJhbGciOiJub25lIn0..";
jwt_t *jwt;
int ret;

SET_OPS();

/* Two dots */
ret = jwt_decode(&jwt, token, NULL, 0);
ck_assert_int_ne(ret, 0);
ck_assert_ptr_null(jwt);

/* One dot */
ret = strlen(token);
token[ret - 1] = '\0';

ret = jwt_decode(&jwt, token, NULL, 0);
ck_assert_int_ne(ret, 0);
ck_assert_ptr_null(jwt);

/* No dot */
ret = strlen(token);
token[ret - 1] = '\0';

ret = jwt_decode(&jwt, token, NULL, 0);
ck_assert_int_ne(ret, 0);
ck_assert_ptr_null(jwt);
}

/* { "typ": "JWT", "alg": "none" } . {} . */
START_TEST(test_jwt_decode_empty_body)
{
const char token[] = "eyJ0eXAiOiJKV1QiLCJhbGciOiJub25lIn0.e30.";
jwt_t *jwt;
int ret;

SET_OPS();

ret = jwt_decode(&jwt, token, NULL, 0);
ck_assert_int_eq(ret, 0);
ck_assert_ptr_nonnull(jwt);

jwt_free(jwt);
}

/* { "typ": "JWT", "alg": "HS256" } . { "test": 1 } . */
START_TEST(test_jwt_decode_nokey_alg_hs256)
{
const char token[] = "eyJ0eXAiOiJBTEwiLCJhbGciOiJOT05FIn0.eyJ0ZXN0IjoxfQ.";
jwt_t *jwt;
int ret;

SET_OPS();

ret = jwt_decode(&jwt, token, NULL, 0);
ck_assert_int_ne(ret, 0);
ck_assert_ptr_null(jwt);
}
END_TEST

/* { "typ": "ALL", "alg": "none" } . { "test": 1 } */
START_TEST(test_jwt_decode_ignore_typ)
{
const char token[] = "eyJ0eXAiOiJBTEwiLCJhbGciOiJIUzI1NiJ9."
"eyJpc3MiOiJmaWxlcy5jeXBocmUuY29tIiwic"
"3ViIjoidXNlcjAifQ.";
const char token[] = "eyJ0eXAiOiJBTEwiLCJhbGciOiJub25lIn0.eyJ0ZXN0IjoxfQ.";
jwt_t *jwt;
int ret;

SET_OPS();

ret = jwt_decode(&jwt, token, NULL, 0);
ck_assert_int_eq(ret, 0);
ck_assert(jwt);
ck_assert_ptr_nonnull(jwt);

jwt_free(jwt);
}
Expand Down Expand Up @@ -270,32 +365,20 @@ START_TEST(test_jwt_decode_hs256)
END_TEST

/* Fron issue #201. Adding tests for alg checks. */
/* { "typ": "JWT", "alg": "HS256" } . { ... } . sig */
START_TEST(test_jwt_decode_hs256_no_key_alg)
{
const char token[] = "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3Mi"
"OiJmaWxlcy5jeXBocmUuY29tIiwic3ViIjoidXNlcjAif"
"Q.dLFbrHVViu1e3VD1yeCd9aaLNed-bfXhSsF0Gh56fBg";
jwt_t *jwt;
int ret;
const char *alg_str;
jwt_alg_t alg;

SET_OPS();

ret = jwt_decode(&jwt, token, NULL, 0);
ck_assert_int_eq(ret, 0);
ck_assert_ptr_nonnull(jwt);

alg_str = jwt_get_header(jwt, "alg");
ck_assert_str_eq(alg_str, "HS256");

alg = jwt_str_alg(alg_str);
ck_assert_int_eq(alg, JWT_ALG_HS256);

alg_str = jwt_alg_str(alg);
ck_assert_str_eq(alg_str, "HS256");

jwt_free(jwt);
ck_assert_int_ne(ret, 0);
ck_assert_ptr_null(jwt);
}
END_TEST

Expand Down Expand Up @@ -394,7 +477,7 @@ END_TEST
unsigned char __key512[64] = "012345678901234567890123456789XY"
"012345678901234567890123456789XY";

int test_jwt_decode_2_hs512_kp(const jwt_t *jwt, jwt_key_t *key)
static int test_jwt_decode_2_hs512_kp(const jwt_t *jwt, jwt_key_t *key)
{
if (jwt_get_alg(jwt) == JWT_ALG_HS512) {
key->jwt_key = __key512;
Expand Down Expand Up @@ -495,9 +578,13 @@ static Suite *libjwt_suite(const char *title)
tcase_add_loop_test(tc_core, test_jwt_dup, 0, i);
tcase_add_loop_test(tc_core, test_jwt_dup_signed, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_2, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_2_compat, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_2_alg_none, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_invalid_alg, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_ignore_typ, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_dot_dot, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_empty_body, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_nokey_alg_hs256, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_invalid_head, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_alg_none_with_key, 0, i);
tcase_add_loop_test(tc_core, test_jwt_decode_invalid_body, 0, i);
Expand Down

0 comments on commit 096082e

Please sign in to comment.