Skip to content

LI-197450 - Pin JWE alg/enc headers before decryption - #137

Merged
grmeyer-hw-dev merged 4 commits into
masterfrom
bugfix/LI-197450_jwe-algorithm-pinning
Oct 1, 2026
Merged

grmeyer-hw-dev merged 4 commits into
masterfrom
bugfix/LI-197450_jwe-algorithm-pinning

Conversation

@pkedar-hw-dev

@pkedar-hw-dev pkedar-hw-dev commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes a JWE algorithm-confusion/downgrade vulnerability (CWE-347, CVSS 8.0) reported by Hawkeye-AI: HyperwalletEncryption::decrypt() called $jwe->decrypt($privateJweKey) without validating the untrusted alg/enc header fields embedded in the ciphertext, so an attacker who could intercept/tamper with an encrypted response could flip alg to legacy RSA1_5 and force the client's real private key to be used with PKCS#1 v1.5 padding — vulnerable to Bleichenbacher-style padding-oracle attacks.
  • Adds HyperwalletEncryption::checkJweHeaderAlgorithm($header), called immediately after JOSE_JWT::decode($body) and before $jwe->decrypt() is ever invoked, which rejects the token unless both alg and enc exactly match the algorithm/encryption method this client instance was configured with. This mirrors the algorithm pinning already applied on the JWS verification side ($this->signAlgorithm).
  • Since the check runs before any key material is used, a tampered header is rejected unconditionally, every time — the vulnerable RSA1_5 code path can never execute against real ciphertext, closing the padding-oracle attack surface entirely (not just masking it).

Testing

Local PHP/Composer installs were blocked in my environment (non-standard Homebrew prefix + corporate TLS-intercepting proxy blocking GitHub zip downloads), so I ran the full suite inside a Docker container with a fully installed vendor/:

docker run --rm -v "$(pwd)":/app -w /app composer:2 php ./vendor/bin/phpunit tests/Hyperwallet/Tests/Util/HyperwalletEncryptionTest.php -v
docker run --rm -v "$(pwd)":/app -w /app composer:2 php ./vendor/bin/phpunit

Target file (HyperwalletEncryptionTest.php): 11/11 passed, including 3 new regression tests added in this PR:

Test What it proves
testShouldThrowExceptionWhenJweAlgHeaderDoesNotMatchExpectedAlgorithm A header with alg: RSA1_5 (downgrade attempt) is rejected with 'While trying to decrypt JWE, unexpected [alg] header found'
testShouldThrowExceptionWhenJweEncHeaderDoesNotMatchExpectedEncryptionMethod A header with an unexpected enc value is rejected with 'While trying to decrypt JWE, unexpected [enc] header found'
testShouldRejectTamperedJweAlgHeaderBeforeDecryption End-to-end attack simulation: encrypts a real message, tampers only the alg header of the resulting JWE to RSA1_5 (leaving the ciphertext/key encryption untouched, as an attacker intercepting traffic would), and asserts decrypt() throws the header-rejection exception — proving the check fires before $jwe->decrypt() is reached

Full suite: 2150 tests, 7908 assertions — all passed. No regressions in existing encryption/decryption, signature verification, or any other test group.

Test plan

  • New regression tests added and passing
  • Full existing test suite passes with no regressions
  • Verified fix rejects tampered alg/enc headers before private key is used
  • Verified legitimate encrypt/decrypt round-trip (existing testShouldSuccessfullyEncryptAndDecryptTextMessage) still passes unchanged

pkedar-hw-dev and others added 3 commits September 29, 2026 15:27
Prevents an algorithm-confusion/downgrade attack (CWE-347): decrypt()
previously called $jwe->decrypt($privateJweKey) without validating the
untrusted alg/enc header fields, so an attacker intercepting a response
could flip alg to legacy RSA1_5 and force the private key to be used
with PKCS#1 v1.5 padding, vulnerable to Bleichenbacher-style attacks.
Mirrors the algorithm pinning already applied on the JWS side.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Composer's dependency resolver now excludes package versions with
known security advisories from the pool by default. phpunit/phpunit's
^5.7/^7.0.0 branches (needed for the PHP 5.6/7.1/7.2 CI jobs) carry
advisories, so the resolver was left only with phpunit 9.6.x, which
requires PHP >=7.3 and breaks composer install on those older PHP
versions in CI.

phpunit is a require-dev-only testing dependency; consumers of this
SDK never install it (composer install --no-dev skips it), so this
has no effect on production dependency security. Verified via
`composer audit --no-dev` that the production dependency tree has
zero advisories. Add --no-blocking to the CI composer install
commands so the resolver can still pick a PHP-compatible phpunit
version for each matrix entry.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The --no-blocking CLI flag doesn't exist in the older Composer version
bundled for the PHP 7.1 CI job, causing a hard
"option does not exist" error there. Composer also supports this via
the COMPOSER_NO_BLOCKING=1 environment variable, which older Composer
versions simply ignore rather than erroring on, so it works across
the entire PHP version matrix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@pkedar-hw-dev

pkedar-hw-dev commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator Author

Why this change

1. The core fix (HyperwalletEncryption.php + tests) — LI-197450

HyperwalletEncryption::decrypt() called $jwe->decrypt($privateJweKey) without checking the JWE's alg/enc headers, which are attacker-controlled (embedded in the ciphertext itself, not verified by anything at that point). An attacker who could tamper with an intercepted response could set alg to legacy RSA1_5, forcing our real private key to be used with PKCS#1 v1.5 padding — vulnerable to Bleichenbacher chosen-ciphertext attacks that can recover plaintext or forge messages.

The fix pins alg/enc to the values this client was explicitly configured with, and rejects anything else before decrypt() is ever called, mirroring the algorithm pinning already applied on the JWS side (signAlgorithm). Three regression tests cover: wrong alg, wrong enc, and a full end-to-end tamper simulation (testShouldRejectTamperedJweAlgHeaderBeforeDecryption).

2. The CI workflow change (.github/workflows/ci.yml)

Unrelated to the vulnerability itself. Composer's resolver now excludes package versions with known security advisories from consideration by default. phpunit/phpunit's older branches (needed for the PHP 5.6/7.1/7.2 test jobs) have advisories, so the resolver was left with only PHPUnit 9.x, which requires PHP ≥7.3 and broke composer install on the older PHP jobs in this repo's CI matrix.

phpunit is a require-dev-only testing dependency — it is never installed by consumers of this SDK. Setting COMPOSER_NO_BLOCKING=1 for CI's install step only affects dependency resolution for running our own test suite in CI, and has no effect on the SDK's production dependency tree. Verified with composer audit --no-dev that production dependencies have zero advisories.

Testing performed

  • Target file HyperwalletEncryptionTest.php: 11/11 passed, including the 3 new regression tests above
  • Full suite: 2150 tests, 7908 assertions, all passing, no regressions
  • composer audit --no-dev: no security vulnerability advisories found in production dependencies

@coveralls

coveralls commented Sep 29, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 97.816% (+0.007%) from 97.809% — bugfix/LI-197450_jwe-algorithm-pinning into master

Comment thread .github/workflows/ci.yml
#get dependencies
- name: Install dependencies
env:
COMPOSER_NO_BLOCKING: 1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this param ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Composer's resolver was blocking PR with PHP version. phpunit/phpunit's older branches (needed for the PHP 5.6/7.1/7.2 test jobs) have advisories, so the resolver was left with only PHPUnit 9.x, which requires PHP ≥7.3 and broke composer install on the older PHP jobs in this repo's CI matrix.

phpunit is a require-dev-only testing dependency - it is never installed by consumers of this SDK. Setting COMPOSER_NO_BLOCKING=1 for CI's install step only affects dependency resolution for running our own test suite in CI, and has no effect on the SDK's production dependency tree. Verified with composer audit --no-dev that production dependencies have zero advisories.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

below is error it throwing , Hence we need to add COMPOSER_NO_BLOCKING=1

https://github.com/hyperwallet/php-sdk/actions/runs/36620439369/job/109584156782?pr=137

Screenshot 2026-09-30 at 4 22 01 PM

Update CHANGELOG.md and ApiClient::VERSION so consumers can identify
and pull in the fix via the package manager.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@pkedar-hw-dev

pkedar-hw-dev commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator Author

Added it to this same PR rather than a follow-up, to keep the version bump tied directly to the fix it documents:

  • Bumped `ApiClient::VERSION` to `2.2.7` (it was stale at `2.2.3` even before this PR — the `CHANGELOG.md` had already moved to `2.2.6` without a matching version-const update, so this also corrects that drift).
  • Added a `2.2.7` entry to `CHANGELOG.md` describing the security fix.

Full suite re-verified passing (2150 tests, 7908 assertions) after the bump.

@akalichety-hw akalichety-hw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@grmeyer-hw-dev
grmeyer-hw-dev merged commit 699fbfb into master Oct 1, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants