oauth2: fix crash in decryptToken on spurious AES-CBC PKCS#7 success

[Copilot]

OAuth2Filter::decryptToken only guarded against EVP_DecryptFinal_ex returning failure. When AES-CBC decrypts ciphertext under the wrong key, PKCS#7 padding is coincidentally valid ~1/256 of the time, producing arbitrary binary bytes. Those bytes were StrJoin-ed back into the Cookie: header via setReferenceKey, tripping HeaderStringValidator::assertValid() and crashing Envoy. Observed flaking in oauth_integration_test during HMAC-secret rotation.

Changes

• source/extensions/filters/http/oauth2/filter.cc — After AES-CBC decryption, validate the plaintext with Http::HeaderUtility::headerValueIsValid (the same predicate HeaderStringValidator uses). Binary/invalid plaintext is treated identically to an explicit decryption failure: log the error and return the original ciphertext unchanged, letting HMAC validation naturally fail and re-redirect.

const bool decrypt_failed =
    decrypt_result.error.has_value() ||
    !Http::HeaderUtility::headerValueIsValid(decrypt_result.plaintext);

• test/extensions/filters/http/oauth2/filter_test.cc — Five new tests:

  • DecryptTokenEmpty / DecryptTokenSameSecret — baseline correctness
  • DecryptTokenDecryptionFails — legacy/unencrypted token (explicit PKCS#7 failure)
  • DecryptTokenSpuriousSuccessReturnsOriginalInput — decryption "succeeds" but plaintext contains bytes invalid as an HTTP header value → original ciphertext returned
  • GarbagePlaintextCookieDoesNotCrash — end-to-end via decodeHeaders; confirms no crash, error log emitted, and RefreshToken cookie value preserved

• changelogs/current.yaml — Bug fix entry under oauth2.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• dl.google.com
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)
• ftpmirror.gnu.org
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)
• mirror.bazel.build
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)
• pkgconfig.freedesktop.org
  • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

Original prompt

Background

The OAuth2 HTTP filter encrypts the access / id / refresh token cookies with AES-256-CBC (introduced in #39059). On every request OAuth2Filter::decryptAndUpdateOAuthTokenCookies decrypts those cookies and rewrites the Cookie: header with the plaintext values before forwarding upstream.

There is a latent bug in OAuth2Filter::decryptToken that can crash Envoy with a HeaderStringValidator assertion failure. It has been observed flaking CI on release/v1.37 in //test/extensions/filters/http/oauth2:oauth_integration_test:

[error][oauth2] failed to decrypt token: j5Vhtnz_…, error: failed to finalize decryption
[critical][assert] ./envoy/common/union_string.h:69 assert failure: valid().
  Details: HeaderStringValidator failed to validate string
  "RefreshToken=P\xE5E*\xC1\xD0\x90\nr/; OauthExpires=3554116502;
   OauthHMAC=…; BearerToken=j5Vhtnz_uyhDVTrSri3GzLoroprQYVoXsp61kIq_JC4;
   OauthNonce.8c18b8fcf575b593=…"
[error][envoy_bug] stacktrace for envoy bug
  #0 Envoy::UnionStringBase<>::assertValid()
  #1 Envoy::UnionStringBase<>::setCopy()
  …
  #3 Envoy::Http::HeaderMapImpl::addReferenceKey()
  #4 Envoy::Http::HeaderMapImpl::setReferenceKey()

The exact same code path is present on main and release/v1.38 — see source/extensions/filters/http/oauth2/filter.cc decryptToken (the function that returns either the decrypted plaintext or, on failure, the original encrypted input).

Root cause

decryptToken only handles the case where EVP_DecryptFinal_ex returns failure (PKCS#7 padding check fails). However when the wrong key is used to AES-CBC-decrypt arbitrary ciphertext, EVP_DecryptFinal_ex will spuriously succeed with probability ~1/256 — whenever the last byte of the random "plaintext" happens to be 0x01, plus smaller cases for 0x02 0x02, etc. In that case decrypt() returns success and the "plaintext" is arbitrary binary bytes (containing \n, \0, ;, control chars, …).

That binary string is then StrJoined back into a new Cookie: header value and pushed into the request via setReferenceKey(Http::Headers::get().Cookie, …), which trips HeaderString::assertValid() because the value is no longer a valid header field-value.

In the failing log, BearerToken is unchanged (decryption properly failed → returned as-is) but RefreshToken has been replaced by 13 bytes of garbage — exactly the asymmetry expected from this race.

The flakiness on oauth_integration_test AuthenticationFlow / RefreshTokenFlow is explained by the SDS hmac-secret reload step (hmac_secret → hmac_secret_1): after rotation, the test re-uses the previously-encrypted TEST_ENCRYPTED_* cookies, so they get AES-decrypted under a different key on subsequent requests — effectively rolling the 1/256 dice on every test run.

This affects two real production scenarios as well:

1. Legacy unencrypted tokens in cookies after a rolling upgrade onto an oauth2_encrypt_tokens-enabled build.
2. HMAC secret rotation while users still hold cookies encrypted under the old secret.

Both are already commented as supported in decryptToken; the comment is correct, the implementation is not.

Fix

Validate the decrypted plaintext using Http::HeaderUtility::headerValueIsValid before treating decryption as successful. Anything else means we have the wrong key (or unencrypted input), so fall through to the existing legacy / wrong-secret behavior — return the original encrypted input unchanged and let HMAC validation naturally fail and re-redirect.

The change is in source/extensions/filters/http/oauth2/filter.cc:

std::string OAuth2Filter::decryptToken(const std::string& encrypted_token) const {
  if (encrypted_token.empty()) {
    return EMPTY_STRING;
  }

  DecryptResult decrypt_result = decrypt(encrypted_token, config_->hmacSecret());

  // Decryption can spuriously succeed against a token that was either never
  // encrypted, or was encrypted under a different secret -- PKCS#7 padding
  // happens to be valid for arbitrary ciphertext with probability ~1/256,
  // leaving us with arbitrary binary bytes that would later fail HeaderString
  // validation when written back into the Cookie header. Treat any plaintext
  // that is not a valid header value as a decrypt failure and fall through to
  // the legacy / wrong-secret handling below.
  const bool decrypt_failed =
      decrypt_result.error.has_value() ||
      !Http::HeaderUtility::headerValueIsValid(decrypt_result.plaintext);

  if (decrypt_failed) {
    ENVOY_STREAM_LOG(error, "failed to decrypt token: {}, error: {}", *decoder_callbacks_,
                     encrypted_token,
                     decrypt_result.error.value_or("plaintext is not a valid header value"));
    // There are two cases:
    // 1. The token is a legacy unencrypted token. Return the toke...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #44677 was opened by Copilot.

see: more, trace.