Security hole: Header names must be included in the signature string

[dlongley]

There's a security hole with this scheme. It looks like only the header values are signed, not the actual headers (their names) those values are associated with. That means that an attacker can change the meaning of a request by swapping various header values.

For example:

Original request:

POST /pay HTTP/1.1
Host: example.com
Date: Thu, 05 Jan 2012 21:31:40 GMT
X-Payment-Source: src@money.com
X-Payment-Destination: dst@money.com
Authorization: Signature keyId="Test",algorithm="rsa-sha256",headers="x-payment-source x-payment-destination" MDyO5tSvin5...

Attacker captures the request and changes it to:

POST /pay HTTP/1.1
Host: example.com
Date: Thu, 05 Jan 2012 21:31:40 GMT
X-Payment-Source: dst@money.com
X-Payment-Destination: src@money.com
Authorization: Signature keyId="Test",algorithm="rsa-sha256",headers="x-payment-destination x-payment-source" MDyO5tSvin5...

Note that the "X-Payment-Source" and "X-Payment-Destination" header values have been swapped and their order in the Authorization header have been swapped.

The resulting signature string in the original request will be:

src@money.com\n
dst@money.com\n

The resulting signature string in the attacker's request will be:

src@money.com\n
dst@money.com\n

So the signature will still be valid in the attacker's request, however, the attacker will have successfully reversed a financial transaction (in this contrived example).

A solution to this problem would be to include the header names in the signature. They should be normalized to lowercase, etc. If this change were made, the original request's signature string would be:

x-payment-source: src@money.com\n
x-payment-destination: dst@money.com\n

And in the attacker's request:

x-payment-destination: src@money.com\n
x-payment-source: dst@money.com\n

As you can see, the attack would fail here as the signature strings are different.

[mcavage]

Hi,

Thanks for the report -- yep, I agree that's definitely wrong for the non-TLS case (fortunately I know of no deployments using that form, but I'll reach out to the parties I know of that have picked this up to be sure). It's been a while, but I remember thinking about this and considering adding in a "meta header" that was an ordered list of signed headers to the signing string. I think your scheme though is more sensible, and is similar to signing a query string anyway (at least in things like OAuth et al).

Anyway - I got contacted about this being potentially folded into a W3C spec, so either we'll fix it there, or if it's not then I'll version this spec and bump it here. I'll leave this issue open for whomever stumbles across this in the meantime.

[dlongley]

@mcavage, I work with @msporny -- he updated me on your email exchange. We're working on a pull request that implements what I suggested above. We'll also add a couple of tweaks to handle corner-cases like multiple headers with the same name (they can just be appended to the signature string in the order they were received; order matters here per the HTTP spec).

[dlongley]

Just a heads-up that there's another security hole where an attacker could potentially reuse a request that was signed with its request line elsewhere by appending an http header that is called "Request-Line". We're going to change the behavior in this corner-case to instead always use the actual request line (METHOD PATH VERSION) regardless of the existence of a header called "Request-Line". If someone needs to sign a request that actually has that strange header, they can add a hack to include X-Request-Line and duplicate the data there (or do some similar work around).

[davidlehn]

I have patches to sign header names, use a "sig" param (should it be "signature"?), fix minor stuff, and some doc fixes. Please review. I can open a pull request if it's ok. Thanks.

https://github.com/digitalbazaar/node-http-signature

[mcavage]

Hi David,

Thanks for the reference -- I'll look at this in detail early this week (realistically tomorrow) and give you any comments.

m

[mcavage]

Hi David,

That was easy - everything looks good to me and is what I expected based on this thread. Pull request welcome.

m

[mcavage]

PR merged.

[mcavage]

I published the most recent changes (breaking) to npm as 0.10.0, and for those who use restify, that's published as 2.5.0.

[dwlf]

This looks to have been resolved 1.5 years ago. Closing.