Skip to content

aws_sigv4: Fix ordering for headers with same prefix in the canonical request #14370

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.

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

Closed
wants to merge 1 commit into from

Conversation

austinmoore-
Copy link
Contributor

@austinmoore- austinmoore- commented Aug 4, 2024

Problem

AWS SigV4 signing requires headers to be lexicographically ordered by name. The current implementation uses strcmp on the full header, leading to incorrect ordering when header names have identical prefixes.

I found two scenarios where curl users could be affected:

  1. S3 object uploads with user-defined metadata (prefixed by x-amz-meta-).
  2. SSE-C operations using x-amz-server-side-encryption-customer-key and x-amz-server-side-encryption-customer-key-md5 headers.

Example of the issue:

curl -X PUT --verbose --silent \
    --aws-sigv4 "aws:amz:us-east-1:s3" --user "$AWS_ACCESS_KEY_ID:$AWS_SECRET_ACCESS_KEY" \
    -H "x-amz-meta-test-two: Test2" \
    -H "x-amz-meta-test: Test" \
    --data-binary @testfile \
    "https://examplebucket.s3.amazonaws.com/testfile"

This results in x-amz-meta-test-two incorrectly preceding x-amz-meta-test in the canonical request, causing S3 to return a SignatureDoesNotMatch error.

Fix

This PR resolves the issue by:

  1. Using strncmp with the length of the shorter header name for comparison for the canonical request (actual request headers are left in the specified order).
  2. If the compared substrings match, the shorter header name is compared as "less than" the longer one.

The fix has been verified with a few real S3 requests, along with the added libcurl test.

@github-actions github-actions bot added the tests label Aug 4, 2024
@austinmoore- austinmoore- changed the title aws_sigv4: Fix canon order for headers with same prefix aws_sigv4: Fix order for headers with same prefix in the canonical request Aug 4, 2024
@austinmoore- austinmoore- changed the title aws_sigv4: Fix order for headers with same prefix in the canonical request aws_sigv4: Fix ordering for headers with same prefix in the canonical request Aug 4, 2024
@bagder
Copy link
Member

bagder commented Aug 4, 2024

/cc @outscale-mgo

@austinmoore- austinmoore- force-pushed the master branch 5 times, most recently from 8f344e6 to 18fe179 Compare August 4, 2024 18:54
If a request containing two headers that have equivalent prefixes
(ex. "x-amz-meta-test:test" and "x-amz-meta-test-two:test2") AWS
expects the header with the shorter name to come first. The previous
implementation used `strcmp` on the full header. Using the example,
this would result in a comparison between the ':' and '-' chars and
sort "x-amz-meta-test-two" before "x-amz-meta-test", which produces
a different "StringToSign" than the one calculated by AWS.
@bagder bagder closed this in cf3e3d9 Aug 5, 2024
@bagder
Copy link
Member

bagder commented Aug 5, 2024

Thanks!

@jdelvare
Copy link

I took a look at the test case and I don't think it actually tests what the commit fixed.
In the example given in the commit message, the 2 headers are in lowercase, so one is indeed a prefix of the other, which is the case which needs special care. However in the test case, one header uses uppercase letters, the other doesn't, so the shorter one isn't considered to be a prefix of the longer one (sort order is case-sensitive according to a comment in the code).
So what the test case actually tests is whether the ordering is still case-sensitive as it is supposed to be, and already was before the fix. In order to also test whether the fix works, you would have to add a third header, of which one of the other two would be a prefix. For example:
x-amz-meta-test-three: test3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants