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/x509: Certs with odd RDN layouts not handled, cause confusing errors #16836

Closed
tv42 opened this issue Aug 22, 2016 · 5 comments
Closed

crypto/x509: Certs with odd RDN layouts not handled, cause confusing errors #16836

tv42 opened this issue Aug 22, 2016 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.8Maybe

Comments

@tv42
Copy link

tv42 commented Aug 22, 2016

  1. What version of Go are you using (go version)?
go version go1.7 linux/amd64
  1. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/tv/go"
GORACE=""
GOROOT="/home/tv/src/go-1.7"
GOTOOLDIR="/home/tv/src/go-1.7/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/home/tv/tmp/go-build877219418=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
  1. What did you do?

The program in in https://play.golang.org/p/I8S80_1P1p gave a confusing error (#16834 created about the message itself). This issue digs into why he ended up with such a parsed certificate, and what should have happened instead.

The cert in that play snippet is from https://github.com/wbond/badtls.io specifically https://github.com/wbond/badtls.io/blob/master/certs/domain-match.crt

What seems to have happened is that the cert there is structured like seq(set(a=b, c=d, e=f)) instead of what I seem to be getting from elsewhere, seq(set(a=b), set(c=d), set(e=f)). I may be very wrong about this, my asn.1 is 10 years rusty.

  1. What did you expect to see?

Hopefully, either

or

  • Go crypto/tls behaves more like openssl 1.0.2d and refuses that connection (this part is fine, and probably wiser, though I can't see anything in openssl changelog that would be the reason)
  • BUT ALSO do something reasonable with RDN sequences where the set contains multiple values. Give an error or parse all the key-value pairs in every set. Right now, entries after the first are silently ignored:

    go/src/crypto/x509/pkix/pkix.go

    Line 67 in 7c5f33b

    atv := rdn[0]
    -- of course, I have no idea how much real world usage this would break. Ain't it great to have such a flexible and underspecified security mechanism?
  1. What did you see instead?

Most of the subject RDN content discarded silently. Confusing error message. Behavior that differs from curl on OS X and Debian Jessie.
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 22, 2016
@bradfitz bradfitz added this to the Go1.8Maybe milestone Aug 22, 2016
This was referenced Aug 22, 2016
Merged
Open
@wbond
Copy link

wbond commented Aug 28, 2016

Just for reference, the incorrectly structured cert is located at https://github.com/wbond/badtls.io/blob/e46322e240ca97c7316e0f6cbf0b31bba610e707/certs/domain-match.crt.

Since this issue was created, I have regenerated the individual certs for badtls.io with a version of the ASN.1 library that generates the standard RDN structure.

@joneskoo
Copy link
Contributor

Three choices how to resolve this (as I see):

  1. Do nothing (and add a comment to

    go/src/crypto/x509/pkix/pkix.go

    Line 67 in 7c5f33b

    atv := rdn[0]
    why it's done like this)

  2. Fail fast – Reject certificate with bad RDN structure (len(rdn) > 1)

    Potential for breaking currently working cases (I guess if CN is first, it would work
    even if followed by entries ignored by go)

    Would it only be an error for the certificate being validated or also would a bad subject
    for a CA break parsing the CA certificate?

  3. Accept the bad structure

    I think this is a potentially dangerous option; different parsers could interpret the
    subject differently and well, if there's a way to make different parsers read the
    CN differently, that's obviously a security concern. 👎

    Although old OpenSSL and SecureTransport seem to do this?

@agl thoughts?

@agl
Copy link
Contributor

agl commented Oct 6, 2016

I ran a scan of the Pilot CT log for any unexpired chains that contained a certificate with more than one element in an RDN of either the Subject or Issuer.

There are only 360 of them, the vast majority appearing to be certificates from the government of Singapore.

I think the code should either reject them outright, or else scan them for known values and only reject cases where there are multiple common names or serial numbers etc.

Since I suspect that this practice is more widespread outside the Web PKI, I'm leaning towards the latter. Any modern certificate should be including SANs and disabling the processing of common names anyway. (Indeed, the CA/B Forum requires this now.)

@wbond
Copy link

wbond commented Oct 6, 2016

A note on compatibility: in my testing none of Secure Transport, SChannel, OpenSSL 1.0.1 or LibreSSL complained about the abnormal RDNs in the certs that had been generated by certbuilder for badtls.io.

I'm not suggesting that means go should take a particular stance, just figured I'd provide that data point.

@gopherbot
Copy link

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

@agl agl mentioned this issue Oct 20, 2016
Closed
@golang golang locked and limited conversation to collaborators Oct 11, 2017
@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.8Maybe
Development

No branches or pull requests
6 participants
@bradfitz @agl @wbond @joneskoo @tv42 @gopherbot