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

AWS LC fails against golang TLS server while ring works fine #1912

Closed
1 task done
robsdedude opened this issue Apr 22, 2024 · 6 comments · Fixed by #1924
Closed
1 task done

AWS LC fails against golang TLS server while ring works fine #1912

robsdedude opened this issue Apr 22, 2024 · 6 comments · Fixed by #1924

Comments

@robsdedude
Copy link
Contributor

Checklist

  • I've searched the issue tracker for similar bugs.

Describe the bug
I've stumbled over a very specific set-up where when using the AWS LC provider, a connection cannot be established while the ring implementation works fine. I'm not 100% sure this is an issue with this crate, but wan't able to trace it down any further.

The set-up: using a server written in golang enforcing TLS 1.2 (I couldn't reproduce the error with a server written in rust with rustls).

To Reproduce
The setup is quite large, even though I tried to reduce it as much as I could.

https://github.com/robsdedude/rusttls-ring-vs-aws-issue/tree/4f42dcf51beadce18dc2c44df610a5184bafa0f9

There's plenty more description about the output setup and the observations in the README of the linked repo.

The bottom line is that when using AWS LC, the client fails with

Custom { kind: InvalidData, error: InvalidCertificate(BadSignature) }

and the server complains

remote error: tls: error decrypting message

Whereas with ring everything works fine. Going through the trace logs, the only difference I could find is that LC offers ECDSA_NISTP521_SHA512 as SignatureAlgorithm while ring doesn't. So I cannot rule out that go's TLS implementation might be at fault here. Sadly I don't know enough TLS details to debug this any further.

@cpu
Copy link
Member

cpu commented Apr 25, 2024

Thanks for filing this issue. It sounds interesting and having a repro is a big help (even if its on the larger side). I won't have a chance to investigate for a little while but wanted to leave a comment so you knew it's on our radar.

@ctz
Copy link
Member

ctz commented Apr 25, 2024

Had a short look at this and it looks like there are no relevant TLS-level differences between the providers. Next step is to minimise the reproducer to just the signature verification, then we can send it upstream.

@robsdedude
Copy link
Contributor Author

Thank you, both. Your work is much appreciated ☺️

@skmcgrail
Copy link
Contributor

skmcgrail commented Apr 26, 2024

So just adding some information here from what I've dug into so far:

Signature algorithms offered by the ring-based client:
ring_client_hello

Signature algorithms offered by the aws-lc-rs-based client:
lc_client_hello

The increase in algorithms here makes sense due to aws-lc-rs offering a larger set of supported algoirthms in its signature module.

The corresponding Go server key exchange message for a ring client:
ring_server_hello

The corresponding Go server key exchange message for aws-lc-rs client:
lc_server_hello

What has me a bit puzzled though is why the Go server in both these instances is offering a signature algorithm to the client that can't be used with the certificate? My understanding is that the signature offered in the key exchange message is supposed to be a signed message of the key exchange parameters using ECDSA using p256r1 w/ SHA256 given that the private key is a p256r1 key and that scheme matches the certificate. So why is the Go server offering p384 and p521 in both instances if that doesn't match the servers private key? Interesting that it succeeds with the ring. Or is Wireshark just lying here and really this is just two bytes that specify the digest algorithm and digital signature scheme.

Attaching the debugger on the failure case confirms this is the failure case: https://github.com/rustls/rustls/blob/main/rustls/src/client/tls12.rs#L897. The actual error being returned is UnsupportedSignatureAlgorithmForPublicKey from here.

From rfc4492 section 2.2:

In ECDHE_ECDSA, the server's certificate MUST contain an ECDSA-
capable public key and be signed with ECDSA.

The server sends its ephemeral ECDH public key and a specification of
the corresponding curve in the ServerKeyExchange message. These
parameters MUST be signed with ECDSA using the private key
corresponding to the public key in the server's Certificate.

The client generates an ECDH key pair on the same curve as the
server's ephemeral ECDH key and sends its public key in the
ClientKeyExchange message.

Both client and server perform an ECDH operation (Section 5.10) and
use the resultant shared secret as the premaster secret.

Of course it is possible I have a gap in my knowledge here, but figured I'd drop some information of what I had seen so far.

Update: I believe this is likely the intended behavior for TLS 1.2, in that the digest algorithm isn't bound to a specific key size. See this comment from the Go TLS implementation: https://github.com/golang/go/blob/master/src/crypto/tls/auth.go#L182-L192

@ctz
Copy link
Member

ctz commented Apr 26, 2024

Ah, thanks for the detailed look!

I agree on your conclusion, and I think it means that offering ecdsa_secp521r1_sha512 for TLS1.2 & TLS1.3 implies support for SHA512 signatures for all supported curves on TLS1.2. That is very annoying!

That previously wasn't an issue, because the set of hash functions we were willing to do verifications for {SHA256, SHA384} were supported for each curve.

@skmcgrail
Copy link
Contributor

skmcgrail commented Apr 26, 2024

Ah I see since aws-lc-rs doesn't expose SHA512 for the other curves that is creating an issue. Let me revisit that and get back to you, I'm not seeing an immediate reason why we wouldn't allow SHA512 with the non-p521 curves.

Update

I think the direction you have chosen for the fix is likely the best path forward given that you only recently extended support for SHA512.

There is a few online discussions that are of interest around this topic of why you may not want to use larger digest sizes with the smaller keys in addition to reusing the same key across different digest algorithms:
https://groups.google.com/g/mozilla.dev.security.policy/c/mCKvUmYUMb0/m/VVW_eSNrCAAJ
https://groups.google.com/a/chromium.org/g/security-dev/c/SlfABuvvQas/m/qOil2X4UBQAJ

In Go's fixed preference order for signature algorithms in clients ECDSA SHA512 is actually ranked lower in priority then SHA256 and SHA384 similar to your change here. I imagine these concerns are largely why TLS 1.3 binds the digest used to a specific key size.

@ctz ctz closed this as completed in #1924 May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants