diff --git a/libjwt/jwt.c b/libjwt/jwt.c index 1034a60e..03c5ba29 100644 --- a/libjwt/jwt.c +++ b/libjwt/jwt.c @@ -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; } @@ -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; } @@ -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; } @@ -762,12 +777,12 @@ 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; @@ -775,8 +790,6 @@ static int jwt_decode_complete(jwt_t **jwt, const unsigned char *key, int key_le 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: @@ -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; @@ -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); } diff --git a/tests/jwt_new.c b/tests/jwt_new.c index 52690f3c..ecc562ef 100644 --- a/tests/jwt_new.c +++ b/tests/jwt_new.c @@ -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."; @@ -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); } @@ -178,11 +212,72 @@ 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; @@ -190,7 +285,7 @@ START_TEST(test_jwt_decode_ignore_typ) ret = jwt_decode(&jwt, token, NULL, 0); ck_assert_int_eq(ret, 0); - ck_assert(jwt); + ck_assert_ptr_nonnull(jwt); jwt_free(jwt); } @@ -270,6 +365,7 @@ 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" @@ -277,25 +373,12 @@ START_TEST(test_jwt_decode_hs256_no_key_alg) "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 @@ -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; @@ -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);