Merge pull request #340 from artgins/exact-alg-pin-verify jwt: pin the exact algorithm in both verify paths (RFC 8725 3.1)
diff --git a/CMakeLists.txt b/CMakeLists.txt index 7bd3a6f..fc7646a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt
@@ -446,7 +446,7 @@ list (APPEND UNIT_TESTS jwt_claims jwt_cnf) # Security - list (APPEND UNIT_TESTS jwt_security) + list (APPEND UNIT_TESTS jwt_security jwt_alg_downgrade) foreach (TEST ${UNIT_TESTS}) jwt_add_test(NAME ${TEST})
diff --git a/libjwt/jwt-verify.c b/libjwt/jwt-verify.c index c3c4e72..6db7b50 100644 --- a/libjwt/jwt-verify.c +++ b/libjwt/jwt-verify.c
@@ -630,6 +630,27 @@ // LCOV_EXCL_STOP } + /* @rfc{8725,3.1} Pin to the EXACT algorithm, not just the key family. + * The alg-vs-alg checks above compare config and key to each OTHER, but + * on the common pinned path -- both config->alg and config->key->alg set + * and equal -- none of them compares the token's own alg (jwt->alg). The + * jwt_alg_required_kty() backstop below is only family-granular (every + * RSn/PSn alg maps to JWK_KEY_TYPE_RSA, every ESn to EC), so without this + * a token presenting e.g. "RS512" verifies against a key the caller + * pinned as "RS256": same family, so the kty guard passes. Not a forgery + * (the signature must still be valid), but a pinned verifier accepting an + * algorithm it never pinned is a policy/conformance gap. Require exact + * agreement with whichever alg the caller pinned. Purely additive: no + * legitimately-pinned token is newly rejected. */ + if (config->alg != JWT_ALG_NONE && jwt->alg != config->alg) { + jwt_write_error(jwt, "JWT alg does not match pinned config alg"); + return 1; + } else if (config->key->alg != JWT_ALG_NONE && + jwt->alg != config->key->alg) { + jwt_write_error(jwt, "JWT alg does not match pinned key alg"); + return 1; + } + /* Algorithm is now bound (jwt->alg). Defensively confirm that the * JWK's actual key type can carry it. This blocks algorithm * confusion (GHSA-q843-6q5f-w55g) even if a malformed JWK has an @@ -794,6 +815,16 @@ if (jwt_alg_required_kty(s->alg) != jwks_item_kty(key)) return; + /* @rfc{8725,3.1} Exact-alg pin: if the candidate key declares an "alg", + * the signature's alg must match it exactly, not merely its family (the + * kty gate above is family-granular -- every RSn/PSn maps to RSA). This + * is the JSON-path mirror of the Compact pinned-alg check and the keyring + * scan's own "kalg != s->alg" skip, applied at the single choke point so + * the explicit-key and "kid"-matched branches are covered too: without + * it a key declaring "RS256" would verify an "RS512" signature. */ + if (jwks_item_alg(key) != JWT_ALG_NONE && jwks_item_alg(key) != s->alg) + return; + jwt->alg = s->alg; jwt->key = key; jwt->error = 0;
diff --git a/tests/jwt_alg_downgrade.c b/tests/jwt_alg_downgrade.c new file mode 100644 index 0000000..95d5891 --- /dev/null +++ b/tests/jwt_alg_downgrade.c
@@ -0,0 +1,125 @@ +/* Public domain, no copyright. Use at your own risk. */ + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include "jwt_tests.h" + +/* Same-family algorithm downgrade in the Compact verify path. + * + * On the pinned path of __verify_config_post() -- both config->alg and + * config->key->alg set and equal -- the token's own alg (jwt->alg) was never + * compared against the pinned alg, and the jwt_alg_required_kty() backstop is + * only family-granular (every RSn/PSn alg maps to RSA). So a token presenting + * "RS512" verified against a key pinned as "RS256" -- same family, valid + * signature -- was accepted, i.e. a pinned verifier accepted an algorithm it + * never pinned. Not a forgery; a policy/conformance gap. These tests pin it + * shut and guard the legitimate (matching) cases against over-rejection. + * + * Runs under each crypto backend (SET_OPS); RSA only, so JSON-backend-agnostic. + */ + +static jwk_set_t *load(const char *file) +{ + char *path; + jwk_set_t *set; + int ret; + + ret = asprintf(&path, KEYDIR "/%s", file); + ck_assert_int_gt(ret, 0); + set = jwks_create_fromfile(path); + free(path); + ck_assert_ptr_nonnull(set); + + return set; +} + +/* Sign a token with the given alg and serialization using a key that carries no + * "alg" hint (so the builder lets us choose any RSA alg for the same modulus). */ +static char *sign(const jwk_item_t *key, jwt_alg_t alg, jwt_serialization_t fmt) +{ + jwt_builder_auto_t *b = jwt_builder_new(); + + ck_assert_int_eq(jwt_builder_setkey(b, alg, key), 0); + ck_assert_int_eq(jwt_builder_set_format(b, fmt), 0); + + return jwt_builder_generate(b); +} + +/* The same downgrade check on both serializations: the Compact path runs + * through __verify_config_post(), the JSON path through verify_entry() / + * try_candidate(). jwt_checker_verify() auto-detects the format. */ +START_TEST(test_same_family_downgrade) +{ + jwk_set_t *sks, *vks; + const jwk_item_t *skey, *vkey; + jwt_serialization_t fmts[] = { JWT_FORMAT_COMPACT, JWT_FORMAT_JSON_FLAT }; + size_t i; + + SET_OPS(); + + /* Same 2048-bit RSA modulus, two views: one with no "alg" (to sign any + * RSA alg), one pinned to RS256 (to verify). */ + sks = load("rsa_key_2048_no_alg.json"); + skey = jwks_item_get(sks, 0); + ck_assert_ptr_nonnull(skey); + + vks = load("rsa_key_2048.json"); /* carries "alg":"RS256" */ + vkey = jwks_item_get(vks, 0); + ck_assert_ptr_nonnull(vkey); + + for (i = 0; i < ARRAY_SIZE(fmts); i++) { + char_auto *rs512 = NULL, *rs256 = NULL; + jwt_checker_auto_t *c1 = NULL, *c2 = NULL, *c3 = NULL; + + /* A genuinely RS512-signed token (valid signature on this key). */ + rs512 = sign(skey, JWT_ALG_RS512, fmts[i]); + ck_assert_ptr_nonnull(rs512); + + /* THE GAP: pin RS256, present RS512. The signature is otherwise + * valid, so without the alg pin this verifies; it must be rejected. */ + c1 = jwt_checker_new(); + ck_assert_int_eq(jwt_checker_setkey(c1, JWT_ALG_RS256, vkey), 0); + ck_assert_int_ne(jwt_checker_verify(c1, rs512), 0); + + /* Positive control 1: pin RS256, present RS256 -> accepted. */ + rs256 = sign(skey, JWT_ALG_RS256, fmts[i]); + ck_assert_ptr_nonnull(rs256); + c2 = jwt_checker_new(); + ck_assert_int_eq(jwt_checker_setkey(c2, JWT_ALG_RS256, vkey), 0); + ck_assert_int_eq(jwt_checker_verify(c2, rs256), 0); + + /* Positive control 2: legitimately accept RS512 (via the no-alg key, + * which pins no alg) -> the pin does not over-reject. */ + c3 = jwt_checker_new(); + ck_assert_int_eq(jwt_checker_setkey(c3, JWT_ALG_RS512, skey), 0); + ck_assert_int_eq(jwt_checker_verify(c3, rs512), 0); + } + + jwks_free(sks); + jwks_free(vks); +} +END_TEST + +static Suite *libjwt_suite(const char *title) +{ + Suite *s; + TCase *tc_core; + int i = ARRAY_SIZE(jwt_test_ops); + + s = suite_create(title); + tc_core = tcase_create("jwt_alg_downgrade"); + + tcase_add_loop_test(tc_core, test_same_family_downgrade, 0, i); + + tcase_set_timeout(tc_core, 30); + suite_add_tcase(s, tc_core); + + return s; +} + +int main(void) +{ + JWT_TEST_MAIN("LibJWT same-family algorithm downgrade in verify"); +}
diff --git a/tests/keys/rsa_key_2048_no_alg.json b/tests/keys/rsa_key_2048_no_alg.json new file mode 100644 index 0000000..24f1198 --- /dev/null +++ b/tests/keys/rsa_key_2048_no_alg.json
@@ -0,0 +1,17 @@ + { + "use": "sig", + "key_ops": [ + "verify", + "sign" + ], + "kid": "28c243a4-a9ac-473f-ab61-d3a68f408279", + "kty": "RSA", + "n": "wtpMAM4l1H995oqlqdMhuqNuffp4-4aUCwuFE9B5s9MJr63gyf8jW0oDr7Mb1Xb8y9iGkWfhouZqNJbMFry-iBs-z2TtJF06vbHQZzajDsdux3XVfXv9v6dDIImyU24MsGNkpNt0GISaaiqv51NMZQX0miOXXWdkQvWTZFXhmsFCmJLE67oQFSar4hzfAaCulaMD-b3Mcsjlh0yvSq7g6swiIasEU3qNLKaJAZEzfywroVYr3BwM1IiVbQeKgIkyPS_85M4Y6Ss_T-OWi1OeK49NdYBvFP-hNVEoeZzJz5K_nd6C35IX0t2bN5CVXchUFmaUMYk2iPdhXdsC720tBw", + "e": "AQAB", + "d": "D8dTnkETSSjlzhRuI9loAtAXM3Zj86JLPLW7GgaoxEoTn7lJ2bGicFMHB2ROnbOb9vnas82gtOtJsGaBslmoaCckp_C5T1eJWTEb-i-vdpPpwZcmKZovyyRFSE4-NYlU17fEv6DRvuaGBpDcW7QgHJIl45F8QWEM-msee2KE-V4Gz_9vAQ-sOlvsb4mJP1tJIBx9Lb5loVREwCRy2Ha9tnWdDNar8EYkOn8si4snPT-E3ZCy8mlcZyUkZeiS_HdtydxZfoiwrSRYamd1diQpPhWCeRteQ802a7ds0Y2YzgfFUaYjNuRQm7zA__hwbXS7ELPyNMU15N00bajlG0tUOQ", + "p": "5y8tNZdtDp3lugNnCAyA8mIcuTu7rpbGhLSUXlJXIxAzJIp_G--u6tlWR2__92R_oqZ0WAclFMmFIXTrNg2ERhngHm5x_MKU0-BQmMVLCFKQ3we0cH3QTjYGinsEyc_xT7MMtWHtG-R9nOr1YpthWd9x0HKGd8LxRashlZzM6Ts", + "q": "18S_ecqMVBimvLzM3BNwp9NbM4dHRatLKnBaTPh1i1a_OAiLbhvR-i9CmkrRBCDoibMTlaXF3xRYbCbzhoTAOA0E8B-Frl5l6dT6FTA5VeuT2Kr4c8C-PkIb2Ttm7KxLpyE1zplOwiOpIpZwDdy7j-B26mYKF_ZajyN9VWMy7qU", + "dp": "YRhyT3DSz_HHG1H0gu_ldGd6kt2gnNocdH33VooUqNhT8oPskMog1-gCEazbf4cJCEIK2THfBBUDQiL96szQgjS56W4Pl84NfdNXZmJuegdbayCsSxa8VyzfoGe8ghpAym1z5_ZCBJX5n98awphp0bpD7f07tq78cHtIdrLNaSM", + "dq": "yC0XOyWn1Old32IFaPN8I6cZSI_rln4ZaRD9JcWoP5JGKvT6bjfPMZ2g28Ynbf4d3opN1BsMnS6h7gyhB56nOhkSCLgl7KRVRn-5V-j6eHTrICtV_wXFObtZXMsYbOBX-4D7C2X9xG0TICyTXrj3Jb8oc8Qg_yQl1gAl6g7zFKU", + "qi": "ERMrkFR7KGYZG1eFNRdVmJMq-Ibxyw8ks_CbiI-n3yUyk1U8962ol2Q0T4qjBmb26L5rrhNQhneM4e8mo9FXLlQapYkPvkdrqW0Bp72A_UNAvcGTmN7z5OCJGMUutx2hmEAlrYmpLKS8pM_p9zpKtEOtzsP5GMDYVlEp1jYSjzQ" + }