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

crypto/tls: advertise full SHA suite in SignatureHashAlgorithm #9757

Closed
medina opened this issue Feb 2, 2015 · 6 comments
Closed

crypto/tls: advertise full SHA suite in SignatureHashAlgorithm #9757

medina opened this issue Feb 2, 2015 · 6 comments
Assignees
Milestone

Comments

@medina
Copy link

@medina medina commented Feb 2, 2015

A simple Go client connecting to a server via TLS1.2 advertises supported algorithms as the equivalent of openssl s_client -sigalgs RSA+SHA256:ECDSA+SHA256:RSA+SHA1:ECDSA+SHA1 -tls1_2, while it should be capable of more. An excerpt of the ClientHello sent from the client is below:

screen shot 2015-02-02 at 10 28 59 am

The server (a web load balancing appliance), which has an RSA+SHA256 certificate and a bundled RSA+SHA384 intermediary perhaps reasonably responds with an alert(40) (handshake_failure), whilst internally logging "[h]andshake failure selecting certificate for foo.example.com: Certificate chain uses algorithms not supported by client". On the face of what's presented by the client, this is true.

I believe this is the same issue previously reported by Michael Daffin on the golang-nuts mailing list. In that case, connecting to the specified server using a simulated string of

# "OpenSSL 1.0.2 22 Jan 2015" for `-sigalgs` support
$ openssl s_client \
    -sigalgs RSA+SHA384:RSA+SHA256 \
    -cipher AES256-SHA \
    -tls1_2 \
    -servername foo.example.com \
    -connect foo.example.com:443 -showcerts

yields success.

I believe the GoLang TLS library should advertise the full SHA suite it supports in the SignatureHashAlgorithm.

@mikioh mikioh changed the title Advertise full SHA suite in SignatureHashAlgorithm crypto/tls: advertise full SHA suite in SignatureHashAlgorithm Feb 2, 2015
@mikioh

This comment has been minimized.

Copy link
Contributor

@mikioh mikioh commented Feb 3, 2015

Please take a look at https://github.com/golang/go/blob/master/CONTRIBUTING.md.

  1. What version of Go are you using (go version)?
  2. What operating system and processor architecture are you using?

Seems like this issue is similar to #8190.

@medina

This comment has been minimized.

Copy link
Author

@medina medina commented Feb 3, 2015

go version go1.4 darwin/amd64

on OS X 10.9.5. That issue points to the fix I'd suggest as well, however you can check the source to see that it didn't make it into go1.4.1.

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Feb 3, 2015

I think the reason sha512/384 arr not advised is that they are rarely used,
so crypto/TLS chooses not to import the sha512 package.

However, it makes sense that if the program does import crypto/sha512,
crypto/tls should detect that and advise those hash algorithms.

@medina

This comment has been minimized.

Copy link
Author

@medina medina commented Feb 3, 2015

crypto/tls may not import crypto/sha512, but crypto/x509 does, with the intent of supporting intermediate certificates such as this. Since the support is there to verify these certs, the support should be there to advertise the capability in TLS1.2 as part of the ClientHello.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 28, 2015

CL https://golang.org/cl/9415 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 29, 2015

CL https://golang.org/cl/9472 mentions this issue.

agl added a commit that referenced this issue Apr 30, 2015
Prior to TLS 1.2, the handshake had a pleasing property that one could
incrementally hash it and, from that, get the needed hashes for both
the CertificateVerify and Finished messages.

TLS 1.2 introduced negotiation for the signature and hash and it became
possible for the handshake hash to be, say, SHA-384, but for the
CertificateVerify to sign the handshake with SHA-1. The problem is that
one doesn't know in advance which hashes will be needed and thus the
handshake needs to be buffered.

Go ignored this, always kept a single handshake hash, and any signatures
over the handshake had to use that hash.

However, there are a set of servers that inspect the client's offered
signature hash functions and will abort the handshake if one of the
server's certificates is signed with a hash function outside of that
set. https://robertsspaceindustries.com/ is an example of such a server.

Clearly not a lot of thought happened when that server code was written,
but its out there and we have to deal with it.

This change decouples the handshake hash from the CertificateVerify
hash. This lays the groundwork for advertising support for SHA-384 but
doesn't actually make that change in the interests of reviewability.
Updating the advertised hash functions will cause changes in many of the
testdata/ files and some errors might get lost in the noise. This change
only needs to update four testdata/ files: one because a SHA-384-based
handshake is now being signed with SHA-256 and the others because the
TLS 1.2 CertificateRequest message now includes SHA-1.

This change also has the effect of adding support for
client-certificates in SSLv3 servers. However, SSLv3 is now disabled by
default so this should be moot.

It would be possible to avoid much of this change and just support
SHA-384 for the ServerKeyExchange as the SKX only signs over the nonces
and SKX params (a design mistake in TLS). However, that would leave Go
in the odd situation where it advertised support for SHA-384, but would
only use the handshake hash when signing client certificates. I fear
that'll just cause problems in the future.

Much of this code was written by davidben@ for the purposes of testing
BoringSSL.

Partly addresses #9757

Change-Id: I5137a472b6076812af387a5a69fc62c7373cd485
Reviewed-on: https://go-review.googlesource.com/9415
Run-TryBot: Adam Langley <agl@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
@agl agl closed this in 1c10598 Apr 30, 2015
@mikioh mikioh modified the milestones: Go1.5, Go1.5Maybe May 15, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
This is the second in a two-part change. See https://golang.org/cl/9415
for details of the overall change.

This change updates the supported signature algorithms to include
SHA-384 and updates all the testdata/ files accordingly. Even some of
the testdata/ files named “TLS1.0” and “TLS1.1” have been updated
because they have TLS 1.2 ClientHello's even though the server picks a
lower version.

Fixes golang#9757.

Change-Id: Ia76de2b548d3b39cd4aa3f71132b0da7c917debd
Reviewed-on: https://go-review.googlesource.com/9472
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
This is the second in a two-part change. See https://golang.org/cl/9415
for details of the overall change.

This change updates the supported signature algorithms to include
SHA-384 and updates all the testdata/ files accordingly. Even some of
the testdata/ files named “TLS1.0” and “TLS1.1” have been updated
because they have TLS 1.2 ClientHello's even though the server picks a
lower version.

Fixes golang#9757.

Change-Id: Ia76de2b548d3b39cd4aa3f71132b0da7c917debd
Reviewed-on: https://go-review.googlesource.com/9472
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.