Closed ConradIrwin closed 4 years ago
AIA is arguably a misfeature of X.509: it causes unexpected network requests during chain validation, which should be a self-contained process, introduces latency, system calls, and potentially intermittent failures in a process that should be deterministic, and multiplies the attack surface of a security critical operation.
Note how Go would have been safe from the catastrophic Windows chain verification vulnerability because it rejects custom curves, if it weren't that Windows implements AIA so was willing to fetch such a poisoned certificate behind Go's back.
crypto/x509 manages the complexity of X.509 by focusing on a strict useful subset, and I don't think AIA fits. Note that applications have access to the extensions, so you could write a thrid-party module that parses AIA with golang.org/x/crypto/cryptobyte, fetches the intermediate, and repeats a failed validation with it in the pool. This would provide you with all the control you need over these requests without growing the crypto/tls API.
Thanks for the detailed reply. I understand the principle of reducing the attack surface here.
It's certainly not very common to see AIA on the internet (as evidenced by the fact that most images load just fine), but it's common enough that we've had multiple independent reports of this not working. Do you know how common it would have to be for this to ship with go?
(I'm assuming that some things are included (despite being bad ideas) because they're necessary for interoperability, and can probably get some numbers on the % of domains we see with this feature enabled (if Google doesn't already have that in an internal dashboard somewhere :D)).
Also, I'd love your help with a little more direction on "write a thrid-party module that parses AIA with golang.org/x/crypto/cryptobyte, fetches the intermediate, and repeats a failed validation with it in the pool."
Would this require a replacement to crypto/x509
and implementing an http.RoundTripper that uses that, or are there hooks I could use to add this functionality to the existing network stack?
Thanks for the detailed reply. I understand the principle of reducing the attack surface here.
It's certainly not very common to see AIA on the internet (as evidenced by the fact that most images load just fine), but it's common enough that we've had multiple independent reports of this not working. Do you know how common it would have to be for this to ship with go?
(I'm assuming that some things are included (despite being bad ideas) because they're necessary for interoperability, and can probably get some numbers on the % of domains we see with this feature enabled (if Google doesn't already have that in an internal dashboard somewhere :D)).
I don't have an explicit bar to commit to, also because it moves depending on the complexity and attack surface cost, as well as an ecosystem benefit rationale. For AIA, the bar would be very high, also because I believe it's good for the ecosystem to leave AIA behind, rather than adopt it further.
Also, I'd love your help with a little more direction on "write a thrid-party module that parses AIA with golang.org/x/crypto/cryptobyte, fetches the intermediate, and repeats a failed validation with it in the pool."
Would this require a replacement to
crypto/x509
and implementing an http.RoundTripper that uses that, or are there hooks I could use to add this functionality to the existing network stack?
This would be logic in your custom tls.Config.VerifyPeerCertificate
, which you can set in http.Transport.TLSClientConfig
.
If the verification fails, you'd look at certs[0].Extensions
, find the one with the AIA OID, parse its value with golang.org/x/crypto/cryptobyte, add the CA to the x509.VerifyOptions.Intermediates
pool, and try Verify
again.
Thank you for the extra context. (I love the idea of pushing the ecosystem forward — easier to do as a large popular project, but harder when you're just getting started :D).
I'll play around and see if I can get something working — and if so put it on Github for others with this problem.
Feel free to close this if you think it's not something that go will implement ever.
Hey @ConradIrwin, playing around with this a little I've come up with the following POC, I haven't done much testing (so use at your own RISK :eyes:) but the brunt of it is there. Tomorrow when I have a chance I'll verify the code, add some test cases and role it into a little library on my Github. I will update you here when I do so.
package main
import (
"crypto/tls"
"crypto/x509"
"errors"
"fmt"
"io/ioutil"
"log"
"net"
"net/http"
"time"
)
func main() {
rootCAs, err := x509.SystemCertPool()
if err != nil {
log.Fatal(err)
}
client := http.Client{
Transport: &http.Transport{
DialTLS: func(network, addr string) (net.Conn, error) {
conn, err := tls.Dial(network, addr, &tls.Config{
InsecureSkipVerify: true,
ServerName: addr,
RootCAs: rootCAs,
VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
host, _, err := net.SplitHostPort(addr)
if err != nil {
return err
}
return verifyPeerCerts(rootCAs, host, rawCerts)
},
})
if err != nil {
return conn, err
}
return conn, nil
},
},
}
res, err := client.Get("https://incomplete-chain.badssl.com/")
if err != nil {
log.Fatal(err)
}
fmt.Println(res.Status)
}
func verifyPeerCerts(rootCAs *x509.CertPool, serverName string, rawCerts [][]byte) error {
certs := make([]*x509.Certificate, len(rawCerts))
for i, asn1Data := range rawCerts {
cert, err := x509.ParseCertificate(asn1Data)
if err != nil {
return errors.New("failed to parse certificate from server: " + err.Error())
}
certs[i] = cert
}
opts := x509.VerifyOptions{
Roots: rootCAs,
CurrentTime: time.Now(),
DNSName: serverName,
Intermediates: x509.NewCertPool(),
}
for _, cert := range certs[1:] {
opts.Intermediates.AddCert(cert)
}
_, err := certs[0].Verify(opts)
if err != nil {
if _, ok := err.(x509.UnknownAuthorityError); ok {
lastCert := certs[len(certs)-1]
if len(lastCert.IssuingCertificateURL) >= 1 && lastCert.IssuingCertificateURL[0] != "" {
resp, err := http.Get(lastCert.IssuingCertificateURL[0])
if resp != nil {
defer resp.Body.Close()
}
if err != nil {
return err
}
data, err := ioutil.ReadAll(resp.Body)
if err != nil {
return err
}
rawCerts = append(rawCerts, data)
return verifyPeerCerts(rootCAs, serverName, rawCerts)
}
}
return err
}
return nil
}
@fcjr this is awesome!
(Hah, I didn't know we already parse the AIA extension into Certificate.IssuingCertificateURL
. Much easier then! By the way I'd recommend using net.SplitHostPort
in of tlsHost
.)
Adding to proposal minutes but this seems headed for a likely decline.
Thanks @FiloSottile, I've updated my comment to reflect that. I've also set up a small library which provides a preconfigured transport which supports AIA for anyone who is interested https://github.com/fcjr/aia-transport-go.
Based on the discussion above (including the fact that it's so easily implemented outside the standard library), this seems like a likely decline.
No change in consensus, so declined.
I am using Go to proxy image requests from an email client (to mask the IP addresses of our users), but there are some images that load in Gmail, but fail to load in our email client.
Investigating this, it seems likely that the problem is that go does not support the "Authority Information Access" extension, which allows a certificate like the one below (in use by AT&T) to validate even though the server does not pass back the entire certificate chain.
I have verified that the intermediate does not appear in my Trust Store and that the image can be loaded in Chrome, Firefox and Safari. I can also see that the image does not load using curl (issue), or using the go standard library.
I would like to propose that go's tls stack support AIA. In particular the subset of AIA that enables loading id-ad-caIssuers over HTTP as defined in https://tools.ietf.org/html/rfc5280#section-4.2.2.2.
It could either be enabled by default, which would allow
http.Get
to work more like a web-browser; or if we're worried about unexpected extra requests, as an option totls.Config
that enables this behaviour.