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.

Sign up for GitHub

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

Jump to bottom

RSA signature does not appear to be correct #28

Open
Diggsey opened this issue Apr 16, 2020 · 13 comments
Open

RSA signature does not appear to be correct #28

Diggsey opened this issue Apr 16, 2020 · 13 comments

Comments

@Diggsey
Copy link
Contributor

Diggsey commented Apr 16, 2020 •
edited
Loading

It's difficult to validate because this repo doesn't contain the expected signature string. However, when I manually apply the algorithm it also seems to fail. Furthermore, I have tested my implementation against the RSA-signed test case in the spec itself, where validation works as expected.

I've been using the test case

MUST derive the digital signature algorithm from the metadata associated with the keyId.

To test this.

My implementation is called with this command:

verify --public-key <root>\http-signatures-test-suite\test\keys\rsa.pub --headers "host digest" --key-type rsa --keyId test

Stdin is:

GET /basic/request HTTP/1.1
Host: example.com
Content-Type: application/json
Digest: SHA-256=X48E9qOokqqrvdts8nOJRJN3OWDUoyWxBf7kbu9DBPE=
Content-Length: 18
Authorization: Signature keyId="test",headers="host digest",signature="G/q271XxN9vXgC4/PZj++CIA1tPRIejyrEv+Xji1JunhlVYgNVnMn13TFa47ierVyHZO+5XQV43J9m/n4gbuEwSj87x2fnp0kV82k1VaXmhpiBHaQ123jK9xToynmcIynrkj2MGCCfTotLf1Z4lCP3Eb2G3K76C+npgWNC0I44g="
Date: Thu, 16 Apr 2020 21:15:07 GMT

From this, I compute the signature string:

host: example.com
digest: SHA-256=X48E9qOokqqrvdts8nOJRJN3OWDUoyWxBf7kbu9DBPE=

Which I then try to validate using the supplied signature:

G/q271XxN9vXgC4/PZj++CIA1tPRIejyrEv+Xji1JunhlVYgNVnMn13TFa47ierVyHZO+5XQV43J9m/n4gbuEwSj87x2fnp0kV82k1VaXmhpiBHaQ123jK9xToynmcIynrkj2MGCCfTotLf1Z4lCP3Eb2G3K76C+npgWNC0I44g=

And the public key:

-----BEGIN PUBLIC KEY-----
MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDCFENGw33yGihy92pDjZQhl0C3
6rPJj+CvfSC8+q28hxA161QFNUd13wuCTUcq0Qd2qsBe/2hFyc2DCJJg0h1L78+6
Z4UMR7EOcpfdUE9Hf3m/hs+FUR45uBJeDK1HSFHD8bHKD6kv8FPGfJTotc+2xjJw
oYi+1hqp1fIekaxsyQIDAQAB
-----END PUBLIC KEY-----

In my implementation, this validation fails as it thinks the signature is invalid, so I test this on the command line to make sure, using the following command:

openssl dgst -verify rsa.pub -sha256 -signature sig.bin message.txt

sig.bin: binary signature having been base64-decoded
message.txt: contents of signature string (LF separated, no trailing newline)

My implementation is able to sign and verify its own signatures, and signatures generated from the command line, and the signature in the http-signatures spec, so either something very strange is going on, or the RSA signature in this repo is not valid.
@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 16, 2020

I also tested this using an online tool with the same result: https://8gwifi.org/RSAFunctionality?rsasignverifyfunctions=rsasignverifyfunctions

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 18, 2020

I would really appreciate a pointer in the right direction here, even just a link to a public implementation which passes this test suite would help immensely.

I find it hard to believe that these have the wrong signature and nobody has noticed, and I would fully expect to be wrong, but none of the implementations linked here: w3c-ccg/http-signatures#1 seem to incorporate this test suite, and there is no reference implementation linked anywhere?

@aljones15 you tested my PR recently with a nodejs implementation - are you able to validate these RSA signatures?

@aljones15
Copy link
Collaborator

aljones15 commented Apr 19, 2020 via email

@aljones15
Copy link
Collaborator

aljones15 commented Apr 19, 2020 via email

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 19, 2020

@aljones15 I'm sorry to say but your nodejs implementation is broken: it always returns success even if the signature doesn't match!

See here:

  const verified = crypto.verify(
    null, canonicalizedString, publicKeyFile, signature);
  if(verified) {
    return canonicalizedString.toString('utf8');
  }
  return 'Signature verification failed.';

The test suite harness expects the generator to return a non-zero exit code if validation fails, and in your program this only happens when the library throws an exception. However, when the signature doesn't match it just returns the string "Signature verification failed." which the test harness ignores.

Clearly there is a need for a test which ensures a program rejects bad signatures.

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 19, 2020

There are two additional bugs I spotted:

  1. The implementation always assumes SHA512 - the "key type" argument from the test suite should really include what digest algorithm to use.

  2. The implementation round-trips binary data through utf8 encoding which will cause problems for data which is not valid utf8:

    canonicalizedString = Buffer.from(
      httpSignatureAlgorithm.hash.digest('utf8'), 'utf8');

This should just be:

    canonicalizedString = httpSignatureAlgorithm.hash.digest();

@aljones15
Copy link
Collaborator

aljones15 commented Apr 19, 2020 via email

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 19, 2020

The utf-8 one might be intentional I'm not sure

In this case the data being encoded is the output of the hash function (ie. it is random binary data, not text) so definitely shouldn't be round-tripped through UTF-8.

thank you for the help so far.

I would be happy to contribute more to this code-base, but I don't want open a load of PRs that sit around for a long time, or never get merged. I will look at adding tests and update the RSA keys with valid ones once #26 is merged.

Is there anything else I can do to help drive progress on the RFC in general? Everything seems to be in a bit of a weird state - I know that lots of companies are using this signature scheme in production, and there is recent activity on the github repo for the RFC itself, but then the main link in the repo description is broken (https://w3c-dvcg.github.io/http-signatures/) and there's not even a README. The draft is 7 years old now, and while there have been continual improvements, it doesn't seems like it's getting any closer to the goal of leaving "draft" status. How is work on this being organised? Are there any timelines?

@aljones15
Copy link
Collaborator

aljones15 commented Apr 19, 2020 via email

@dlongley
Copy link
Contributor

@Diggsey,

The draft is 7 years old now, and while there have been continual improvements, it doesn't seems like it's getting any closer to the goal of leaving "draft" status. How is work on this being organised? Are there any timelines?

The spec has been picked up by the HTTP working group at IETF: w3c-ccg/http-signatures#96 (comment)

Further work on the spec will continue there.

@aljones15
Copy link
Collaborator

aljones15 commented Apr 20, 2020 •
edited
Loading

@Diggsey So i created this branch:

https://github.com/digitalbazaar/http-signature-header/blob/binary-throws-on-on-verified-signature/bin/util.js

What is strange is that with the utf8 round trip it does verify, without the utf8 roundtrip it does not verify and throws as you seem to have seen. Not entirely sure what is going on there perhaps the issue is I might have roundtriped the signing process that produced the signature being verified?

When I can make time again (probably this weekend) I will try verifying a random signature and see if something is causing verify to return true in all cases.

@Diggsey
Copy link
Contributor Author

Diggsey commented Apr 20, 2020

perhaps the issue is I might have roundtriped the signing process that produced the signature being verified?

Possibly. For me, when I test your code it fails to validate (and then passes the test anyway) with or without the utf-8 round-trip. It's possible there's some platform-specific behaviour regarding the utf-8 conversion (I am testing on windows), although that really shouldn't be the case as utf-8 should be the same everywhere...

@aljones15
Copy link
Collaborator

aljones15 commented Apr 20, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dlongley @aljones15 @Diggsey