web-payments / web-payments.org

Web Payments website and core specifications
https://web-payments.org/
29 stars 13 forks source link

Review by James Manger #5

Open msporny opened 11 years ago

msporny commented 11 years ago

Review by James Manger uncovered the following issues:

  1. Content-Length in the example in section 2.1.2. "Signature String Construction" should be 14, not 15. The content-MD5 value is calculated over 14 bytes.
  2. The string to be signed (section 2.1.2. "Signature String Construction") looks like lines that could be sent in the HTTP request, but they donât quite match. Lines in an HTTP header end with \r\n, whereas lines in the string to be signed end in \n. Why bother omitting \n from the last line? It is simpler to end all the lines the same way. It is annoyingly awkward to write files with a text editor that omit a final \n.
  3. The signature is wrong in section 4.1. "Default Test". Unpacking the signature reveals a SHA-256 hash of EBF002A95EC4DDB7A50CCAC009BAA2FE5D1B2604DA7CB871B870CEE21BE85EB9. The SHA-256 has of "date: Thu, 05 Jan 2012 21:31:40 GMT" is 1958B656C09E29824A2BCF03197923BCD9B55370F2FF2118FF4C47AD77513B3F.
  4. The signature is wrong in section 4.2. "All Headers Test". Unpacking the signature reveals a SHA-256 hash of 4E7A038BC9EFAB95AC829AC176659DCE0A7B6DC94F9FAA54375390A5EA8A9B37. The SHA-256 has of all the headers is 1015528C71F9BC3D445B457CA41D8BC884E89275EDDD0F0435BBB13A65B4550D.
  5. The fields that are signed are absolutely crucial in determining the security properties achieved. It is not sufficient for the spec to merely say the choice of fields is "out of scope" in a clause of the "Security Considerations" (section 3.5 "Required Headers to Sign").
  6. The introduction needs to state that: the mechanism signs a subset of an HTTP request; recipients need to be pre-configured with the subset they require (and MUST confirm that "headers" includes at least that subset); the body is only protected if a Content-MD5/Digest is signed (and the body hash is checked). Signing just "date" show the originator committed to one HTTP request at about the time of the date value, but gives no protection as to the nature of that request (the query params, path, method, body, or even host etc). One strategy for recipients would be to only pass signed fields on to the next stage of request processing.
davidlehn commented 11 years ago

As I was updating the original spec doc, code, and tests in the node implementation, I synced up the spec tests and code tests so it would be easier to keep them correct as changes were made. The tests can dump some output including the signature values. It seems the input and signatures changed as it moved to an IETF doc. Consider updating or adding to the node tests to reflect the new test values. https://github.com/joyent/node-http-signature