Skip to content

Signature: Avoid PHP warning when algorithm is not set #1803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 19, 2025
Merged

Signature: Avoid PHP warning when algorithm is not set #1803

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
429fd32
Signature: Avoid PHP warning when `algorithm` is not set
obenland Jun 11, 2025
0559052
Add changelog
matticbot Jun 11, 2025
68fd2d7
Merge branch 'trunk' into fix/signature-algorithm-warning
obenland Jun 16, 2025
df002b4
Don't accept empty algorithms
obenland Jun 19, 2025
1f45bb8
Merge branch 'trunk' into fix/signature-algorithm-warning
obenland Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions includes/class-signature.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ public static function verify_http_signature( $request ) {
$algorithm = 'sha512';
}

if ( \base64_encode( \hash( $algorithm, $body, true ) ) !== $digest[1] ) { // phpcs:ignore
if ( \base64_encode( \hash( $algorithm, $body, true ) ) !== $digest[1] ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable, WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode
return new WP_Error( 'activitypub_signature', __( 'Invalid Digest header', 'activitypub' ), array( 'status' => 401 ) );
}
}
Expand Down Expand Up @@ -354,7 +354,7 @@ public static function get_remote_key( $key_id ) {
* @return string The signature algorithm.
*/
public static function get_signature_algorithm( $signature_block ) {
if ( $signature_block['algorithm'] ) {
if ( isset( $signature_block['algorithm'] ) ) {
switch ( $signature_block['algorithm'] ) {
case 'rsa-sha-512':
return 'sha512'; // hs2019 https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures-12.
Copy link
Member Author

Choose a reason for hiding this comment

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

@mediaformat While looking at this function I opened the spec that's linked to in the rsa-sha-512 case.
Any example there mentioning SHA512 has hs2019 set as its algorithm, not rsa-sha-512:

The authorization header and signature would be generated as:

Authorization: Signature keyId="rsa-key-1",algorithm="hs2019",
headers="(request-target) (created) host digest content-length",
signature="Base64(RSA-SHA512(signing string))"

—RSA Example

The error that gets returned when there is no algorithm says "only rsa-sha256 and hs2019 are supported":

wordpress-activitypub/includes/class-signature.php

Lines 286 to 288 in 499bcad

if ( ! $algorithm ) {
return new WP_Error( 'activitypub_signature', __( 'Unsupported signature algorithm (only rsa-sha256 and hs2019 are supported)', 'activitypub' ), array( 'status' => 401 ) );
}

Should the case statement test for hs2019 instead of rsa-sha-512?

Copy link
Contributor

Choose a reason for hiding this comment

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

@obenland I think the error message is correct, in that the declared algorithm should be either rsa-256 or hs2019, because under hs2019 the key is meant to be derived from keyId.

Implementers SHOULD derive the
digital signature algorithm used by an implementation from the key
metadata identified by the keyId rather than from this field. If
algorithm is provided and differs from the key metadata identified
by the keyId, for example rsa-sha256 but an EdDSA key is
identified via keyId, then an implementation MUST produce an error.
Implementers should note that previous versions of the algorithm
parameter did not use the key information to derive the digital
signature type and thus could be utilized by attackers to expose
security vulnerabilities.
— algorithm

The implementation actually derives the key from the digest header rather than the keyId.

wordpress-activitypub/includes/class-signature.php

Lines 294 to 298 in 499bcad

$algorithm = 'sha256';
$digest = explode( '=', $headers['digest'], 2 );
if ( 'SHA-512' === $digest[0] ) {
$algorithm = 'sha512';
}

Copy link
Member Author

Choose a reason for hiding this comment

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

If algorithm is provided

@mediaformat Based on the doc you shared, I guess we shouldn't return an error if algorithm is not set?

Expand Down
Loading