Skip to content
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

AWS4SigningResult.ForQueryParameters does not escape semicolons #1953

Closed
1 task
rittneje opened this issue Dec 14, 2021 · 7 comments
Closed
1 task

AWS4SigningResult.ForQueryParameters does not escape semicolons #1953

rittneje opened this issue Dec 14, 2021 · 7 comments
Labels
breaking-change This issue requires a breaking change to remediate. bug This issue is a bug. module/sdk-core needs-major-version Can only be considered for the next major release p2 This is a standard priority issue queued

Comments

@rittneje
Copy link

rittneje commented Dec 14, 2021

Description

The ForQueryParameters method does not escape special characters. In particular, if there are multiple signed headers, then it includes them delimited by a literal semicolon instead of %3B. This is a violation of the RFC spec, and also causes issues with parsers in other languages. For example, Go treats the semicolon either as a delimiter between query parameters or as an invalid character, depending on the language version.

public string ForQueryParameters
{
get
{
var authParams = new StringBuilder()
.AppendFormat("{0}={1}", HeaderKeys.XAmzAlgorithm, AWS4Signer.AWS4AlgorithmTag)
.AppendFormat("&{0}={1}", HeaderKeys.XAmzCredential, string.Format(CultureInfo.InvariantCulture, "{0}/{1}", AccessKeyId, Scope))
.AppendFormat("&{0}={1}", HeaderKeys.XAmzDateHeader, ISO8601DateTime)
.AppendFormat("&{0}={1}", HeaderKeys.XAmzSignedHeadersHeader, SignedHeaders)
.AppendFormat("&{0}={1}", HeaderKeys.XAmzSignature, Signature);
return authParams.ToString();
}
}

Reproduction Steps

using System;
using Amazon.Runtime;
using Amazon.Runtime.Internal;
using Amazon.Runtime.Internal.Auth;
using Amazon.Runtime.Internal.Util;
using Amazon.SecurityToken;
using Amazon.SecurityToken.Model;
using Amazon.SecurityToken.Model.Internal.MarshallTransformations;
...

            IClientConfig config = new AmazonSecurityTokenServiceConfig();
  
            ImmutableCredentials creds = FallbackCredentialsFactory.GetCredentials().GetCredentials();
            IRequest req = GetCallerIdentityRequestMarshaller.Instance.Marshall(new GetCallerIdentityRequest());
            req.Endpoint = EndpointResolver.DetermineEndpoint(config, req);
            req.UseQueryString = true;
            req.OverrideSigningServiceName = "sts";
            req.HttpMethod = "GET";
  
            if (creds.UseToken)
            {
                req.Parameters.Add("X-Amz-Security-Token", creds.Token);
            }
            req.Parameters.Add("X-Amz-Expires", "900");
            req.Headers.Add("X-Custom-Header", "some-value")
  
            AWS4PreSignedUrlSigner aws4Signer = new AWS4PreSignedUrlSigner();
            AWS4SigningResult signingResult = aws4Signer.SignRequest(req, config, new RequestMetrics(), creds.AccessKey, creds.SecretKey);
  
            Uri url = AmazonServiceClient.ComposeUrl(req);
            string result = url.AbsoluteUri + "&" + signingResult.ForQueryParameters;

The resulting URL will have X-Amz-SignedHeaders=host;x-custom-header instead of X-Amz-SignedHeaders=host%3Bx-custom-header.

(Yes, we are referencing some internal packages here, until #1905 is addressed. Regardless, this bug still needs to be fixed.)

Resolution

  • 👋 I can/would-like-to implement a fix for this problem myself

The ForQueryParameters method must escape SignedHeaders while constructing the query string. In particular, semicolons must be replaced with %3B.

Alternatively, use AWSSDKUtils.UrlEncode, as is done internally while generating the signature.

var credential = string.Format(CultureInfo.InvariantCulture, "{0}/{1}", AccessKeyId, Scope);

var authParams = new StringBuilder()
                    .AppendFormat("{0}={1}", HeaderKeys.XAmzAlgorithm, AWS4Signer.AWS4AlgorithmTag)
                    .AppendFormat("&{0}={1}", HeaderKeys.XAmzCredential, AWSSDKUtils.UrlEncode(credential, false))
                    .AppendFormat("&{0}={1}", HeaderKeys.XAmzDateHeader, ISO8601DateTime)
                    .AppendFormat("&{0}={1}", HeaderKeys.XAmzSignedHeadersHeader, AWSSDKUtils.UrlEncode(SignedHeaders, false))
                    .AppendFormat("&{0}={1}", HeaderKeys.XAmzSignature, Signature);

This is a 🐛 bug-report

@rittneje rittneje added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 14, 2021
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Jan 14, 2022

Appears to be reproducible using customer's code. It generates URL something like:

https://sts.amazonaws.com/?Action=GetCallerIdentity&Version=2011-06-15&X-Amz-Expires=900&X-Amz-Security-Token=IQoJb3JpZ2luX2VjEGIaCXVzLWVhc3QtMSJHMEUCIQDPXj7VXUKT8pA6ls3OWWENVEy3gxNh%2F6H%2BhQq7ppLB0wIgXbNRf4JdpRo3nmy7u7pFlz%2F79YpZDr%2FGRipawXUYeqAqnQIIexABGgwxMzk0ODA2MDI5ODMiDFCbXLGtlEd%2BeLGA4Sr6AUtgpoAyJCPOlHMEeCl3bllWIyL6k8vojAIWersogHBPH4lYI1ZX8BZiZFP7VY89sNRFJ2%2BHLntrlgJOpYVjLfZ%2BsCbvC4%2BUQ5T%2BSNvgBXceg8DmR%2Bk7mT2jlh62AH7qN%2B0nIARsCN%2FIaJTnreTrlR2bYOMweZVXFrRvTQdSc0ssgXbnPLoM%2FASBAcJ575hk9Cnw6s9%2Fx42a6CZu0XoA%2FsvwU52E0mednZKCWJ2PkZFmum3mH%2BdJ7WA0eWUO5gdYCdXIiiM6FyQuz9MjbK7GMWRsi2V9BMYjo3NhmpVSpqyLjmwp6ketgWPLv7RfQMdGwY0jgz%2BIEZyIwBQwuPOGjwY6nQF%2FEOaKqcCavJAYNqMM4Xx2S3nEsmWqsskGwBY9lWbqwVq9m2Vb2h4uEGDpkekIb5cWyEsyK1xQIgZRMed9ulTbdRAawzdvELu4O4yTEQ9rQnfHISjTwA%2By64oivD5tZ7DTcGsb4aHuOKvSPR7CZrQPr8Y%2BvGZERFtzZr30roXEN%2B%2BmsKP%2BP3978C3XD7xHe%2B3C%2BsDaXUbMUOH%2FUU3%2B&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ASIASA6NRDFTU36XQ4FP/20220114/us-east-1/sts/aws4_request&X-Amz-Date=20220114T175853Z&X-Amz-SignedHeaders=host;x-custom-header&X-Amz-Signature=6eab1f8161a8289cf361ee945af11e39ab86030dff0b23a3ce8761fd5d98a6d8

Not sure if semicolons should be escaped to %3B. However, also refer https://www.w3.org/TR/1999/REC-html401-19991224/appendix/notes.html#h-B.2.2, which states We recommend that HTTP server implementors, and in particular, CGI implementors support the use of ";" in place of "&" to save authors the trouble of escaping "&" characters in this manner.).

Semicolon is not special in URLs themselves, but they are special in the parameter list in the query component (sometimes, they are used in place of &). Then they need to be escaped.

Also refer https://skorks.com/2010/05/what-every-developer-should-know-about-urls/.

Needs discussion with team since we also need to make sure there are no breaking changes.

@ashishdhingra ashishdhingra added needs-review and removed needs-triage This issue or PR still needs to be triaged. labels Jan 14, 2022
@rittneje
Copy link
Author

@ashishdhingra One thing I will point out is that the semicolon is being escaped to %3B while generating the signature.

var canonicalQueryParams = CanonicalizeQueryParameters(parametersToCanonicalize);

protected static string CanonicalizeQueryParameters(IEnumerable<KeyValuePair<string, string>> parameters)
{
return CanonicalizeQueryParameters(parameters, true);
}

if (uriEncodeParameters)
{
if (string.IsNullOrEmpty(value))
canonicalQueryString.AppendFormat("{0}=", AWSSDKUtils.UrlEncode(key, false));
else
canonicalQueryString.AppendFormat("{0}={1}", AWSSDKUtils.UrlEncode(key, false), AWSSDKUtils.UrlEncode(value, false));
}

Consequently, it seems like the AWS server must convert semicolon to %3B on the way in, in order for the signature to match, which is quite surprising.

@ashishdhingra
Copy link
Contributor

This appears to be a breaking change and should be opt-in behavior.

@ashishdhingra ashishdhingra added B breaking-change This issue requires a breaking change to remediate. labels Jan 21, 2022
@marfenij
Copy link

hello @ashishdhingra
not sure if correct issue to write here, but symptoms with wrong encoding are similar (for my opinion)
found difference how URL is composed when try to get object from AWS S3 services
and this cause Amazon.S3.AmazonS3Exception: The Signature you specified is invalid

I what this might caused by not identical code which use for compose path and signature

var canonicalForSign = AWSSDKUtils.CanonicalizeResourcePath(new Uri("https://endpoint.com", UriKind.RelativeOrAbsolute), "prefix.${previousTxid}");
            
var composeUrl = AWSSDKUtils.ResolveResourcePath("/prefix.${previousTxid}", null);

Assert.Equal(canonicalForSign, composeUrl);
/*
Xunit.Sdk.EqualException: Assert.Equal() Failure
                  ↓ (pos 8)
Expected: /prefix.%24%7BpreviousTxid%7D
Actual:   /prefix.$%7BpreviousTxid%7D
                  ↑ (pos 8)
*/

and if go little bit deeper
we have such code

var escaped = Uri.EscapeUriString(c.ToString());

which deprecated in .net 6.0 might create issues https://docs.microsoft.com/en-us/dotnet/api/system.uri.escapeuristring?view=net-6.0

P.S AWS CLI compose correct URL - encode $ -> %24

@rittneje
Copy link
Author

This appears to be a breaking change and should be opt-in behavior.

@ashishdhingra By that logic all bug fixes would have to be opt-in on the off chance that someone was relying on the incorrect behavior.

@ik130 ik130 added the vNext label Oct 28, 2022
@ashishdhingra ashishdhingra added p2 This is a standard priority issue queued and removed B labels Nov 2, 2022
@ik130 ik130 added needs-major-version Can only be considered for the next major release and removed vNext labels Dec 1, 2022
@ashishdhingra
Copy link
Contributor

This is fixed in latest version, refer

.

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This issue requires a breaking change to remediate. bug This issue is a bug. module/sdk-core needs-major-version Can only be considered for the next major release p2 This is a standard priority issue queued
Projects
None yet
4 participants