diff --git a/changes-entries/69946.txt b/changes-entries/69946.txt new file mode 100644 index 00000000000..94221f40710 --- /dev/null +++ b/changes-entries/69946.txt @@ -0,0 +1,3 @@ + *) mod_autht_jwt: Fix an authentication bypass where a JWT claim whose + name was a prefix of "aud", "sub", "exp" or "nbf" (e.g. "a" or "au") + could be mistaken for that claim. PR 69946 [Arturo Bernal] diff --git a/modules/aaa/mod_autht_jwt.c b/modules/aaa/mod_autht_jwt.c index 617344dfdd7..dea0606586a 100644 --- a/modules/aaa/mod_autht_jwt.c +++ b/modules/aaa/mod_autht_jwt.c @@ -966,28 +966,32 @@ static autht_status check_token(request_rec *r, const char *type, continue; } - if (!strncmp("aud", kv->k->value.string.p, kv->k->value.string.len)) { + if (kv->k->value.string.len == (sizeof("aud") - 1) + && !strncmp("aud", kv->k->value.string.p, kv->k->value.string.len)) { if (kv->v->type == APR_JSON_STRING) { aud = apr_pstrndup(r->pool, kv->v->value.string.p, kv->v->value.string.len); } } - if (!strncmp("sub", kv->k->value.string.p, kv->k->value.string.len)) { + if (kv->k->value.string.len == (sizeof("sub") - 1) + && !strncmp("sub", kv->k->value.string.p, kv->k->value.string.len)) { if (kv->v->type == APR_JSON_STRING) { sub = apr_pstrndup(r->pool, kv->v->value.string.p, kv->v->value.string.len); } } - if (!strncmp("exp", kv->k->value.string.p, kv->k->value.string.len)) { + if (kv->k->value.string.len == (sizeof("exp") - 1) + && !strncmp("exp", kv->k->value.string.p, kv->k->value.string.len)) { if (kv->v->type == APR_JSON_LONG) { exp = kv->v->value.lnumber; exp_set = 1; } } - if (!strncmp("nbf", kv->k->value.string.p, kv->k->value.string.len)) { + if (kv->k->value.string.len == (sizeof("nbf") - 1) + && !strncmp("nbf", kv->k->value.string.p, kv->k->value.string.len)) { if (kv->v->type == APR_JSON_LONG) { nbf = kv->v->value.lnumber; nbf_set = 1; diff --git a/test/pyhttpd/classic_errorlog.py b/test/pyhttpd/classic_errorlog.py index 8e9c88c01d3..9c8b6e6f7a2 100644 --- a/test/pyhttpd/classic_errorlog.py +++ b/test/pyhttpd/classic_errorlog.py @@ -79,6 +79,7 @@ r".*\(20023\)The given path was above the root path:.*unable to include file", r".*\[include:error\].* unable to include \""]), "test_ldap.py": (["AH01617", "AH01631"], []), + "test_mod_autht_jwt.py": (["AH10428"], []), "test_mergeslashes.py": (["AH01630"], []), "test_negotiation.py": (["AH00690"], []), "test_ocsp.py": (["AH03239", "AH10373"], []), diff --git a/test/pytest_suite/.gitignore b/test/pytest_suite/.gitignore index d81d4779ab7..e13e9b1b778 100644 --- a/test/pytest_suite/.gitignore +++ b/test/pytest_suite/.gitignore @@ -31,6 +31,7 @@ t/conf/ssl/ca t/conf/cacheroot/ t/logs/ t/state/ +t/autht_jwt.secret # Generated htpasswd / htdigest files t/basic1 diff --git a/test/pytest_suite/apache_pytest/config.py b/test/pytest_suite/apache_pytest/config.py index d6b4afed71c..74426e7ea8a 100644 --- a/test/pytest_suite/apache_pytest/config.py +++ b/test/pytest_suite/apache_pytest/config.py @@ -670,6 +670,38 @@ def _maybe_generate_sslca(self) -> None: SSLCA(Path(self.vars["sslca"]), self.vars["servername"]).generate() + # Shared with test/pytest_suite/tests/t/modules/test_mod_autht_jwt.py, + # which signs its JWTs with this same value. + AUTHT_JWT_SECRET = b"bz69946-test-secret-do-not-use-in-prod" + + def _maybe_generate_autht_jwt_secret(self) -> None: + """Write the HS256 secret AuthtJwtVerify reads while parsing its + config (modules/aaa/mod_autht_jwt.c:set_jwt_verify), so the file is + already in place before httpd starts -- a missing file is a fatal + config error, and writing it from a test function runs too late. + + Also sets the @jwt_crypto_driver@ token: AuthtJwtVerify's HS256 path + needs an apr_crypto driver, which mod_autht_jwt only initializes when + AuthtJwtDriver names one explicitly (modules/aaa/mod_autht_jwt.c: + auth_bearer_init() skips apr_crypto_get_driver() entirely unless + library_set is true). Without it every token fails HMAC verification + before the 'aud' claim is even inspected. The driver name mirrors + apr_crypto.h's APU_CRYPTO_RECOMMENDED_DRIVER per platform. + + The token is set unconditionally, before the has_module() check below: + process_conf_in() expands every @token@ in autht_jwt.conf.in textually, + regardless of the guard around its use (that + guard is only meaningful to httpd at runtime, not to this expansion + pass) -- on a build without mod_autht_jwt, returning early here would + leave the token unset and crash config generation with a KeyError. + """ + self.vars["jwt_crypto_driver"] = "commoncrypto" if sys.platform == "darwin" else "openssl" + if not self.info.has_module("mod_autht_jwt"): + return + secret = Path(self.vars["serverroot"]) / "autht_jwt.secret" + if not secret.exists(): + secret.write_bytes(self.AUTHT_JWT_SECRET) + def _emit_inherited_loadmodules(self) -> None: """Re-emit inherited (active) LoadModule lines (inherit_load_module). @@ -829,6 +861,10 @@ def generate(self, cmodule_loads: list[tuple[str, Path]] | None = None) -> Path: # dir exists (mirrors sslca_can/sslca_generate, TestConfig.pm:1524-1554). self._maybe_generate_sslca() + # Same reasoning as the SSL CA: AuthtJwtVerify reads its secret file + # while parsing the config, so it must exist before server.start(). + self._maybe_generate_autht_jwt_secret() + # Re-emit inherited LoadModule directives so the test server actually # loads modules (the generated conf does not Include the install conf). # Mirrors inherit_load_module (TestConfigParse.pm:213): wrap each in diff --git a/test/pytest_suite/t/conf/autht_jwt.conf.in b/test/pytest_suite/t/conf/autht_jwt.conf.in new file mode 100644 index 00000000000..6110dd74778 --- /dev/null +++ b/test/pytest_suite/t/conf/autht_jwt.conf.in @@ -0,0 +1,31 @@ +# +# Test config for mod_autht_jwt (see BZ 69946) +# +# mod_autht_jwt.c may be present as a loadable module (satisfying +# ) but still be a non-functional stub that registers neither +# the AuthtJwtVerify directive nor the "jwt" Autht provider, on any build +# whose apr-util lacks apr_jose support (BZ 70276/70277). Guard on the +# directive itself, not just the module, so this fragment never breaks +# httpd startup -- and the whole pytest_suite session -- on such a build. + + + + + # AuthtJwtVerify's HS256 path needs an apr_crypto driver, which + # mod_autht_jwt only initializes when a library is explicitly named + # (its own default library field is never marked "set" on its own). + # Without this, every token fails HMAC verification before the + # 'aud' claim is even inspected, masking the BZ 69946 fix either way. + AuthtJwtDriver @jwt_crypto_driver@ + + + AuthType bearer + AuthName autht-jwt-test + AuthBearerProvider jwt + AuthtJwtVerify hs256 file "@SERVERROOT@/autht_jwt.secret" + Require valid-user + + + + + diff --git a/test/pytest_suite/t/htdocs/autht-jwt-test/index.html b/test/pytest_suite/t/htdocs/autht-jwt-test/index.html new file mode 100644 index 00000000000..9a4b59d9b01 --- /dev/null +++ b/test/pytest_suite/t/htdocs/autht-jwt-test/index.html @@ -0,0 +1 @@ +jwt-ok diff --git a/test/pytest_suite/tests/t/modules/test_mod_autht_jwt.py b/test/pytest_suite/tests/t/modules/test_mod_autht_jwt.py new file mode 100644 index 00000000000..d87c2b046d2 --- /dev/null +++ b/test/pytest_suite/tests/t/modules/test_mod_autht_jwt.py @@ -0,0 +1,117 @@ +r"""Regression test for BZ 69946 -- mod_autht_jwt authentication bypass via +claim-name prefix matching. + +``check_token()`` in modules/aaa/mod_autht_jwt.c identified the ``aud``, +``sub``, ``exp`` and ``nbf`` claims with: + + strncmp("aud", kv->k->value.string.p, kv->k->value.string.len) + +``strncmp(a, b, n)`` only compares the first ``n`` bytes. Since ``n`` here is +the *actual* claim-name length (not the length of the literal), a claim +whose name is a proper prefix of "aud" (e.g. "a" or "au") satisfies this +check and is treated as if it were the real "aud" claim. A JWT that omits +"aud" entirely but carries such a short claim with the expected AuthName +value then passes the audience check that is supposed to confirm the token +was issued for this specific location. + +This test builds real HS256 JWTs (no external `jwt` package -- the standard +library's hmac/hashlib/base64 are enough) and checks that a token exploiting +the prefix match is rejected, while a token using the real "aud" claim name +still works. +""" + +import base64 +import hashlib +import hmac +import json + +import pytest + +from apache_pytest import need_module + +# Kept in sync with apache_pytest.config.TestConfig.AUTHT_JWT_SECRET, which +# writes this value to the file AuthtJwtVerify loads before httpd starts. +SECRET = b"bz69946-test-secret-do-not-use-in-prod" + + +def _b64url(data: bytes) -> str: + return base64.urlsafe_b64encode(data).rstrip(b"=").decode("ascii") + + +def _make_jwt(claims: dict) -> str: + header = _b64url(json.dumps({"alg": "HS256", "typ": "JWT"}).encode()) + payload = _b64url(json.dumps(claims).encode()) + signing_input = f"{header}.{payload}".encode() + sig = hmac.new(SECRET, signing_input, hashlib.sha256).digest() + return f"{header}.{payload}.{_b64url(sig)}" + + +def _skip_unless_functional(http): + """need_module() only checks that mod_autht_jwt.so is loaded, not that it + was built with apr_jose/JWT support (BZ 70276/70277): a stub build + registers the same module name but neither the AuthtJwtVerify directive + nor the "jwt" Autht provider, so in + autht_jwt.conf.in omits the /autht-jwt-test/ Location entirely. On such a + build this request hits a plain, unauthenticated static file (200); on a + functional build AuthType bearer with no Authorization header is + rejected (401). Skip explicitly rather than failing on the stub build's + 404/200, and never silently pass tests that never actually exercised the + module.""" + r = http.GET("/autht-jwt-test/") + if r.status_code != 401: + pytest.skip( + "mod_autht_jwt built without JOSE/JWT support " + f"(stub build, BZ 70276/70277; got {r.status_code} with no Authorization header)" + ) + + +@need_module("autht_jwt", "auth_bearer", "autht_core") +def test_jwt_aud_exact_match_is_accepted(http): + """Sanity check: a token with the real 'aud' claim is accepted.""" + _skip_unless_functional(http) + token = _make_jwt({"aud": "autht-jwt-test", "sub": "alice"}) + r = http.GET("/autht-jwt-test/", headers={"Authorization": f"Bearer {token}"}) + assert r.status_code == 200, ( + f"expected 200 for a valid 'aud' claim, got {r.status_code}" + ) + + +@need_module("autht_jwt", "auth_bearer", "autht_core") +def test_jwt_missing_aud_is_rejected(http): + """Sanity check: a token with no audience-like claim at all is rejected.""" + _skip_unless_functional(http) + token = _make_jwt({"foo": "bar", "sub": "alice"}) + r = http.GET("/autht-jwt-test/", headers={"Authorization": f"Bearer {token}"}) + assert r.status_code == 401, ( + f"expected 401 with no 'aud' claim present, got {r.status_code}" + ) + + +@need_module("autht_jwt", "auth_bearer", "autht_core") +def test_jwt_aud_prefix_bypass_is_rejected(http): + """BZ 69946: a claim whose name is a prefix of 'aud' ('au') must NOT be + treated as the 'aud' claim. Before the fix this token is wrongly + AUTHT_GRANTED (200); after the fix it must be AUTHT_MISMATCH (401). + + 'sub' is required here: Require valid-user needs r->user, which + check_token() only sets when 'sub' is present. Without it this test + would get 401 regardless of the aud bug and prove nothing.""" + _skip_unless_functional(http) + token = _make_jwt({"au": "autht-jwt-test", "sub": "alice"}) + r = http.GET("/autht-jwt-test/", headers={"Authorization": f"Bearer {token}"}) + assert r.status_code == 401, ( + "authentication bypass: a claim named 'au' (a prefix of 'aud') with " + f"the correct AuthName value was accepted as 'aud' (got {r.status_code})" + ) + + +@need_module("autht_jwt", "auth_bearer", "autht_core") +def test_jwt_aud_single_char_prefix_bypass_is_rejected(http): + """Same bypass with the shortest possible prefix, 'a'.""" + _skip_unless_functional(http) + token = _make_jwt({"a": "autht-jwt-test", "sub": "alice"}) + r = http.GET("/autht-jwt-test/", headers={"Authorization": f"Bearer {token}"}) + assert r.status_code == 401, ( + "authentication bypass: a claim named 'a' (a prefix of 'aud') with " + f"the correct AuthName value was accepted as 'aud' (got {r.status_code})" + )