SECURITY-10974: stop notification exceptions disclosing the request body - #102
Draft
tomas-amaro wants to merge 2 commits into
Draft
tomas-amaro wants to merge 2 commits into
tomas-amaro wants to merge 2 commits into
Conversation
SECURITY-10974 / HackerOne 3688453. The reported HMAC disclosure was already fixed in d42cae6, which removed both HMACs from AuthorizationException and switched the comparison to hash_equals(). Two of the report's recommendations were never applied, and the alternate channel it describes is still open - it just no longer carries the signature. NotificationException::customMessage() still concatenated the raw request body. The body is attacker-controlled and exception messages reach HTTP responses and log aggregators, so it was a reflection and log-injection channel. The message now carries the body length; getBody()/getHeaders() expose the full values for deliberate server-side logging. Notification::test_authorization() now rejects a missing HMAC header explicitly. Previously the array access raised an undefined-key warning - disclosing a filesystem path under display_errors - before hash_equals() coerced null to '' and failed closed anyway. This matches the .NET reference, which guards the missing header with !string.IsNullOrEmpty before comparing. sample/callback.php now catches both exception types and answers with a bare 401 or 400. This is the change that makes the reported attack unexploitable regardless of what the exception contains, and was recommendation 3 of the report. Verified by reproducing the report's proof of concept: the forged request now gets an empty 401, there is nothing to extract and replay, and a correctly signed notification still returns 200. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found while reproducing the SECURITY-10974 proof of concept: every response carried a notice naming the SDK's absolute install path, because PHP 8 ignores spl_autoload_register()'s $do_throw argument and warns when it is passed false. Passing true silences it and is the behaviour you want on PHP 7 too - a failed autoloader registration should be loud. Unrelated to the reported vulnerability, kept as its own commit for that reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to SECURITY-10974 / HackerOne report 3688453.
The reported issue is already fixed
d42cae6e("bugfix: update exception logging", #94, 2026-05-05) landed all three of the report's cryptographic recommendations, and shipped before v1.13.0:AuthorizationExceptionno longer takes or emits the expected/received HMACstest_authorization()useshash_equals()instead of a loose!=headersString()masks the HMAC headerThis PR does not re-fix any of that. It closes recommendations 3 and 4, which were never applied.
What is still open
The alternate channel the reporter described is still there — it just no longer carries the signature.
NotificationException::customMessage()concatenated the raw request body. The body is attacker-controlled, and exception messages reach HTTP responses and log aggregators. Underdisplay_errors=1the sample echoed the attacker's own payload straight back. The message now reports the body length;getBody()/getHeaders()expose the full values for deliberate server-side logging, so debuggability is preserved.sample/callback.phphad no exception handling (recommendation 3). It now catchesAuthorizationException→ bare401andNotificationException→ bare400, with no detail in the response (recommendation 4). This is the change that makes the reported attack unexploitable regardless of what the exception contains.A missing HMAC header was not guarded. The array access raised an undefined-key warning — which discloses a filesystem path under
display_errors— beforehash_equals()coercednullto''and failed closed anyway. Now rejected explicitly, matching the .NET reference, which guards it with!string.IsNullOrEmptybefore comparing.The second commit is unrelated to the report and separated for that reason: on PHP 8 the autoloader emitted a notice naming the SDK's absolute install path on every response, because
spl_autoload_register()ignores$do_throwand warns when passedfalse.Verification
Reproduced the report's proof of concept against the patched sample,
display_errors=1:200 OK, 1989 bytes, echoes the attacker's body401, empty body401200200401, no warningvendor/bin/phpunit— 54 pass (was 49; 5 new regression tests). The one deprecation is pre-existing, in an unrelated transport test on PHP 8.5.composer check— phpcs 0 errors, phpstan clean.🤖 Generated with Claude Code