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

Jump to bottom

crypto/tls: generate_cert.go sets KeyEncipherment KU for non-RSA keys [freeze exception] #36499

Closed
cpu opened this issue Jan 10, 2020 · 11 comments
Closed

crypto/tls: generate_cert.go sets KeyEncipherment KU for non-RSA keys [freeze exception] #36499

cpu opened this issue Jan 10, 2020 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@cpu
Copy link

cpu commented Jan 10, 2020 •
edited

What version of Go are you using (go version)?

go version go1.13.4 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

N/A

What did you do?

$ go run ./src/crypto/tls/generate_cert.go -host threeletter.agency -ecdsa-curve=P256

$ openssl x509 -in cert.pem -noout -text | grep -A1 "X509v3 Key Usage:"

What did you expect to see?

            X509v3 Key Usage: critical
                Digital Signature

What did you see instead?

            X509v3 Key Usage: critical
                Digital Signature, Key Encipherment

Summary:

The crypto/tls/generate_cert.go utility should only set the template x509.Certificate's KeyUsage field to a value with the x509.KeyUsageKeyEncipherment bits set when the certificate subject public key is an RSA public key, not an ECDSA or ED25519 public key.

Presently it sets the KeyUsage to KeyUsageKeyEncipherment and KeyUsageDigitalSignature no matter what type of public key is used:

go/src/crypto/tls/generate_cert.go

Line 110 in 79ccbe1

KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,

RFC 5480 describes the usage of ECDSA elliptic curve subject keys with X509. Unfortunately while Section 3 "Key Usages Bits" indicates which key usage bits MAY be used with a certificate that indicates id-ecPublicKey in the SubjectPublicKeyInfo field it doesn't provide guidance on which usages should not be included (e.g. the keyEncipherment bit, which is particular to RSA key exchange). The same problem is present in RFC 8410 Section 5 describing Key Usage Bits for ED25519 elliptic curve subject keys.

There's an update to RFC 5480 in last call stage within the IETF LAMPS WG, draft-ietf-lamps-5480-ku-clarifications-00. This update is meant to clarify the allowed Key Usages extension values for certificates with ECDSA subject public keys by adding:

If the keyUsage extension is present in a certificate that indicates id-ecPublicKey as algorithm of AlgorithmIdentifier [RFC2986] in SubjectPublicKeyInfo, then following values MUST NOT be present:

keyEncipherment; and
dataEncipherment.

I don't believe there is an update for RFC 8410 in the works but I suspect it will be clarified similarly in the future (I will follow up with the LAMPS WG).

The current behaviour of generate_cert.go won't comply with the updated RFC 5480 requirement.
This was referenced Jan 10, 2020
Closed
Closed
@toothrot toothrot changed the title tls: generate_cert.go sets KeyEncipherment KU for non-RSA keys. crypto/tls: generate_cert.go sets KeyEncipherment KU for non-RSA keys. Jan 10, 2020
@toothrot toothrot added this to the Backlog milestone Jan 10, 2020
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 10, 2020
@toothrot
Copy link
Contributor

/cc @FiloSottile @agl @katiehockman

@cpu
Copy link
Author

cpu commented Jan 29, 2020

👋 Hi folks,

Anything I can do to help move this forward?

@mholt mholt mentioned this issue Jul 2, 2020
Closed
@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 2, 2020
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.16 Jul 2, 2020
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 2, 2020
@FiloSottile FiloSottile self-assigned this Jul 2, 2020
@mholt
Copy link

mholt commented Jul 2, 2020

Subscribing to this issue, as certs generated like this with ECC keys (haven't used ED25519 yet) raises an error in Chrome 83 on Mac (and I have verified this as well): https://caddy.community/t/internal-ssl-certificate-is-not-standards-compliant-on-macos/8933

@FiloSottile FiloSottile changed the title crypto/tls: generate_cert.go sets KeyEncipherment KU for non-RSA keys. crypto/tls: generate_cert.go sets KeyEncipherment KU for non-RSA keys [freeze exception] Jul 2, 2020
@FiloSottile FiloSottile modified the milestones: Go1.16, Go1.15 Jul 2, 2020
@FiloSottile
Copy link
Contributor

@rsc @golang/osp-team Can I get a freeze exception on this? generate_cert.go is a +build ignore file, so it's basically documentation, but it gets copied into projects, and they end up subtly broken.

@dmitshur
Copy link
Contributor

dmitshur commented Jul 2, 2020 •
edited

Fixing this during freeze seems okay to me. Chrome 83 was released after the Go 1.15 freeze started, so some of the information here wasn't available earlier. A +build ignored file doesn't contribute to the stability of the final release, and won't compromise testing that has been done via Go 1.15 Beta 1.

Approved.

@sleevi
Copy link

sleevi commented Jul 2, 2020

Yeah, Chrome 83 is a misdirect. It's been part of BoringSSL since 2019-01-30 for TLS 1.2 and earlier, and always for TLS 1.3

https://boringssl.googlesource.com/boringssl/+/d7266ecc9bf92ffad277bc39653919da79c8f42b%5E%21

@davidben
Copy link
Contributor

davidben commented Jul 2, 2020

That change was for RSA keys, not EC keys. But nothing applicable changed in Chrome 83 as far as I know. I don't think this is coming from BoringSSL, however. We require for the presence of various bits, but not the absence. The error would also come out differently.

Given both the error and that it's reflected in the macOS certificate viewer UI, this probably isn't coming directly from Chrome at all, but macOS.

@sleevi
Copy link

sleevi commented Jul 2, 2020

@davidben No, that change affected EC keys.

    if (ssl->config->enforce_rsa_key_usage ||
        EVP_PKEY_id(hs->peer_pubkey.get()) != EVP_PKEY_RSA) {

If enforce_rsa_key_usage is false, but EVP_PKEY_ID is EC, it will run those checks. Because we always check KU for EC keys.

@davidben
Copy link
Contributor

davidben commented Jul 2, 2020 •
edited

Right, we check KU for EC keys. But we did that long before that CL. The new thing in that CL was support for checking on RSA in TLS 1.2.

But we only check for the presence of the bit we wanted. If there was some irrelevant bit, BoringSSL doesn't care. Sounds like this is about something rejecting an irrelevant bit.

@davidben
Copy link
Contributor

davidben commented Jul 2, 2020

(One way or another, the keyEncipherment bit doesn't make sense for EC keys and ought to be omitted.)

@maraino maraino mentioned this issue Jul 2, 2020
Merged
@gopherbot
Copy link

Change https://golang.org/cl/214337 mentions this issue: crypto/tls: create certs w/o KeyEncipherment KU for non-RSA keys in generate_cert.go

@golang golang locked and limited conversation to collaborators Jul 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.15
Development

Successfully merging a pull request may close this issue.

crypto/tls: create certs w/o KeyEncipherment KU for non-RSA keys in generate_cert.go cpu/go-1
8 participants
@toothrot @davidben @cpu @mholt @FiloSottile @sleevi @dmitshur @gopherbot