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/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

Comments

@tv42
Copy link

@tv42 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:
    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.

@wbond
Copy link

@wbond 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

@joneskoo joneskoo commented Aug 28, 2016

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

  1. Do nothing (and add a comment to

    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 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 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

@gopherbot gopherbot commented Oct 10, 2016

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

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.