search

aws / aws-sdk-go

AWS SDK for the Go programming language.
http://aws.amazon.com/sdk-for-go/
Apache License 2.0
8.57k stars 2.06k forks source link

Add method to presign request without hoisting headers to query string. #458

Closed rfielding closed 8 years ago

rfielding commented 8 years ago

In order to securely perform direct (from browser) upload/download with S3, Presign() needs to deal with X-Amz-SignedHeaders. Example: I want a pre signed url: https://s3.aws.amazon.com/area51/deadAlien.jpg .... But I want the S3 GET to require a header "Nonce: a9d9395" in order to retrieve it, so that if the URL is leaked or bookmarked, it should be useless without the nonce. I did not see a way to do this "the right way", so I went into v4.go and changed buildCanonicalHeaders to require a header called "nonce" and plugged the nonce value in. That made it work. i.e.: hit the presigned url with a browser and it fails. Then use wget with --header="nonce: a9d9395" and it succeeds. This means that as far as the interface to Presign goes, I need to set the values of pre signed headers before calling Presign()

jasdel commented 8 years ago

@rfielding thanks for creating this issue. The SDK will use any header added to the request when calculating the signature. You can add the headers to Request.HTTPRequest.Header and they should be used by the SDK when calculating the signature. The only headers that should be ignored by the SDK are those defined in v4.go#ignoreHeaders. (Authorization,Content-Type,Content-Length,User-Agent).

svc := s3.New(sess)
req, out  := svc.GetOjbectRequest(&s3.GetObjectInput{...})

req.HTTPRequest.Header.Set("Nonce", "a9d9395")
err := req.Send()

If you're looking to add a nonce automatically to each request instead of in code, you can use the request.Handlers to inject the nouce automatically.

rfielding commented 8 years ago

Thanks @jasdel for taking a look at this, and getting to it so quickly. I apologize for my post being too vague. It's a problem with Presign(), not Send(). The problem is that when I do this the download works without the nonce (a security hole -- the URL alone is sufficient to get it, even though the nonce is supposedly required). The point of the nonce is that if a user scrapes the URL out of the browser and sends it to somebody else, he may not realize that the other person isn't actually authorized for it. So part of the required info must be in a header, since headers and cookies are not so easily leaked. This is the client code being suggested:

func main() {
    account := "default"
    bucketName := aws.String("decipherers")
    svc := generateSession(account)

    picture := aws.String("Grumpy-Cat-6.jpg")
    expiresWithin := 60 * time.Minute
    req, _ := svc.GetObjectRequest(
        &s3.GetObjectInput{
            Bucket: bucketName,
            Key:    picture,
        },
    )

    req.HTTPRequest.Header.Set("nonce", "9999")
    urlStr, _ := req.Presign(expiresWithin)
    log.Println(urlStr)
}

The problem is that the pre-signed GET url works without any headers being set (security hole):

Robs-MacBook-Pro:uploadtest rfielding$ go run main.go 
2015/12/08 10:32:12 https://decipherers.s3.amazonaws.com/Grumpy-Cat-6.jpg?Nonce=9999&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T153212Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host&X-Amz-Signature=40fd74c4ac5c127d735b0a0599464cdd7ad517a41e9891cbc0264a0aa3452e7a
Robs-MacBook-Pro:uploadtest rfielding$ wget 'https://decipherers.s3.amazonaws.com/Grumpy-Cat-6.jpg?Nonce=9999&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T153212Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host&X-Amz-Signature=40fd74c4ac5c127d735b0a0599464cdd7ad517a41e9891cbc0264a0aa3452e7a'

The get should have failed, when called like this, then succeeded when wget has the parameter --header="nonce: 9999" in it.

The nonce appearing in the GET is itself a security problem, as that's supposed to be the missing information required to ensure that the unexpired url is not leaked. So I did a Proof Of Concept fix in my code to do this (awful hack). First, I just removed the Header.Set from my code. I then hardcoded the header set at the earliest point I can (with passing the nonce value in somehow TBD):

        //Comment out my explicit Header.Set in main for now....
    //req.HTTPRequest.Header.Set("nonce", "9999")

So I am at github.com/aws/aws-sdk-go/private/signer/v4/v4.go:

func (v4 *signer) buildCanonicalHeaders() {
    var headers []string
    headers = append(headers, "host")
    headers = append(headers, "nonce")
    for k := range v4.Request.Header {
        if _, ok := ignoredHeaders[http.CanonicalHeaderKey(k)]; ok {
            continue // ignored header
        }
        headers = append(headers, strings.ToLower(k))
    }
    sort.Strings(headers)

    v4.signedHeaders = strings.Join(headers, ";")

    if v4.isPresign {
        v4.Query.Set("X-Amz-SignedHeaders", v4.signedHeaders)
    }

    headerValues := make([]string, len(headers))
    for i, k := range headers {
        if k == "host" {
            headerValues[i] = "host:" + v4.Request.URL.Host
        } else {
            if k == "nonce" {
                headerValues[i] = "nonce:9999"
            } else {
                headerValues[i] = k + ":" +
                    strings.Join(v4.Request.Header[http.CanonicalHeaderKey(k)], ",")
            }
        }
    }

    v4.canonicalHeaders = strings.Join(headerValues, "\n")
}

So, now it behaves correctly. The download FAILS without the header as it should now. The nonce is not leaked into the GET url:

Robs-MacBook-Pro:uploadtest rfielding$ go run main.go 
2015/12/08 10:56:39 https://decipherers.s3.amazonaws.com/Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec82652d93210d94abc4d431612c444255354341a281fddd3225f41e80
Robs-MacBook-Pro:uploadtest rfielding$

And it fails when we try to use the URL alone:

Robs-MacBook-Pro:uploadtest rfielding$ wget 'https://decipherers.s3.amazonaws.com/Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec82652d93210d94abc4d431612c444255354341a281fddd3225f41e80'
The name is too long, 291 chars total.
Trying to shorten...
New name is Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec8.
The name is too long, 291 chars total.
Trying to shorten...
New name is Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec8.
--2015-12-08 10:57:48--  https://decipherers.s3.amazonaws.com/Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec82652d93210d94abc4d431612c444255354341a281fddd3225f41e80
Resolving decipherers.s3.amazonaws.com... 54.231.10.233
Connecting to decipherers.s3.amazonaws.com|54.231.10.233|:443... connected.
HTTP request sent, awaiting response... 403 Forbidden
2015-12-08 10:57:48 ERROR 403: Forbidden.

Robs-MacBook-Pro:uploadtest rfielding$

And if we redo the request with --header="nonce: 9999", it succeeds.

Robs-MacBook-Pro:uploadtest rfielding$ wget --header="nonce: 9999" 'https://decipherers.s3.amazonaws.com/Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec82652d93210d94abc4d431612c444255354341a281fddd3225f41e80'
The name is too long, 291 chars total.
Trying to shorten...
New name is Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec8.
The name is too long, 291 chars total.
Trying to shorten...
New name is Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec8.
--2015-12-08 10:59:27--  https://decipherers.s3.amazonaws.com/Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec82652d93210d94abc4d431612c444255354341a281fddd3225f41e80
Resolving decipherers.s3.amazonaws.com... 54.231.11.57
Connecting to decipherers.s3.amazonaws.com|54.231.11.57|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 1643642 (1.6M) [image/jpeg]
Saving to: 'Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec8'

Grumpy-Cat-6.jpg?X- 100%[=====================>]   1.57M  1.32MB/s   in 1.2s   

2015-12-08 10:59:29 (1.32 MB/s) - 'Grumpy-Cat-6.jpg?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAJIOAGQ2S42PSW6QQ%2F20151208%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20151208T155639Z&X-Amz-Expires=3600&X-Amz-SignedHeaders=host%3Bnonce&X-Amz-Signature=c326c0ec8' saved [1643642/1643642]

Robs-MacBook-Pro:uploadtest rfielding$

Only now does it work. It does appear that X-Amz-SignedHeaders should just work for this purpose, though there are some signed headers that cannot appear in the GET url that gets returned.

jasdel commented 8 years ago

Thanks for the clarification, that was very helpful. It sounds like the best path to add this feature would be to add a method of registering which headers should be required, and not leaked to the URL. This would allow you to set which headers will stay in the signature, but not be hoisted to the URL.

Since this issue is limited to Presigning, we could explore the idea of creating a new Presigning method, or consider adding a variadic optional argument of additional options.

Tagged this as an enhancement so we can get this functionality added. We're also more than glad to take a look at PRs if you'd like to prototype something.

rfielding commented 8 years ago

I was trying to figure out how to write a clean fix (how do I at least properly pass in the header value and keep it out of the URL?) so that I could make a pull request. I will gladly make a pull request if I manage to figure it out first. Thanks!

On Tue, Dec 8, 2015 at 11:45 AM, Jason Del Ponte notifications@github.com wrote:

Thanks for the clarification, that was very helpful. It sounds like the best path to add this feature would be to add a method of registering which headers should be required, and not leaked to the URL. This would allow you to set which headers will stay in the signature, but not be hoisted to the URL.

Since this issue is limited to Presigning, we could explore the idea of creating a new Presigning method, or consider adding a variadic optional argument of additional options.

Tagged this as an enhancement so we can get this functionality added. We're also more than glad to take a look at PRs if you'd like to prototype something.

— Reply to this email directly or view it on GitHub https://github.com/aws/aws-sdk-go/issues/458#issuecomment-162941360.

http://rfieldin.appspot.com http://rrr00bb.blogspot.com

rfielding commented 8 years ago

I have a more general question, because I think this is possibly a general security bug. Why is any signed header value ever in the GET URL? (host isn't in there, so why would nonce be?).

If the client knows the correct values, then they go into the headers. If they are both in the GET parameters and the headers, and the values contradictory, why should I be allowed access if they are contradictory (between header and URL), and if the URL tells me what they are, then why does S3 want them plugged into a header?. I am very new to AWS (a few days), but I can't think of a case where the header value should be spelled out in the URL. It's like there's a difference between what the REST APIs mean, versus what the SDK means.

jasdel commented 8 years ago

@rfielding we'd be more than glad to look at your PR if you would like to put one together.

Hoisting headers to the URL during pre-signing is helpful for some services. For S3 though policies only inspect headers and not query string parameters. Because of this, I think we need to create an additional way of generating a presigned url which does not hoist any headers to the URL. In addition changing the functionality of Request.Presign would be a breaking change for those that relied on it.

I think support for this idea could be added as a new method off of aws/request#Request, PresignHTTPRequest which would return a net/http#Request instance. The benefit of returning a http.Request is the URL and map of headers are easily retrieved. With the http.Request you could share the presigned URL and the header map as desired.

What do you think of this pattern? It is similar to how the AWS SDK for Ruby exposes both presign with hoisting and generic presign http request without hoisting.

rfielding commented 8 years ago

This particular experiment to go directly to S3 to (rather dramatically) speed things up got shot down by other issues (related to 2way tls, gatekeeper, client-side encryption, etc) - so I will end up intercepting big file transfers with a Go program for now.

If I do go back to using pre-signed urls for some reason, I at least know how to plug the security hole in my own copy of the sdk.

jasdel commented 8 years ago

Thanks for the feedback @rfielding I've updated this issue to be a feature request so we can add a way to generate presigned requests without hoisting headers to the query string.

jasdel commented 8 years ago

Hi we didn't update this issue, but this is now fixed. Release v1.1.0 fixes how headers are hoisted to the query string, in addition adds a pre-sign request that does minimal and returns a list of headers that were signed.