Fix PreIssuer detection (#554)

* Fix PreIssuer detection

y

* Use OIDs from rfc6962

* add IsCA check, and more thorough tests.

o

* better names

* add comment

Author: phbnf

internal/types/rfc6962/rfc6962.go

@@ -43,8 +43,10 @@ const (
 	TreeNodePrefix = byte(0x01)
 )
 
-// Defined in RFC 6962 s3.1.
+// Defined or referenced in RFC 6962 s3.1.
 var (
+	OIDExtAuthorityKeyId                  = asn1.ObjectIdentifier{2, 5, 29, 35}
+	OIDExtKeyUsage                        = asn1.ObjectIdentifier{2, 5, 29, 37}
 	OIDExtensionCTPoison                  = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 11129, 2, 4, 3}
 	OIDExtKeyUsageCertificateTransparency = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 11129, 2, 4, 4}
 )

internal/x509util/ct.go

@@ -25,13 +25,9 @@ import (
 
 	"github.com/transparency-dev/tessera/ctonly"
 	"github.com/transparency-dev/tesseract/internal/types/rfc6962"
-)
 
-var (
-	oidExtensionAuthorityKeyId = asn1.ObjectIdentifier{2, 5, 29, 35}
-	// These extensions are defined in RFC 6962 s3.1.
-	oidExtensionCTPoison                        = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 11129, 2, 4, 3}
-	oidExtensionKeyUsageCertificateTransparency = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 11129, 2, 4, 4}
+	"golang.org/x/crypto/cryptobyte"
+	cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1"
 )
 
 type tbsCertificate struct {
@@ -110,7 +106,7 @@ func removeExtension(tbsData []byte, oid asn1.ObjectIdentifier) ([]byte, error)
 //   - The precert's AuthorityKeyId is changed to the AuthorityKeyId of the
 //     intermediate.
 func BuildPrecertTBS(tbsData []byte, preIssuer *x509.Certificate) ([]byte, error) {
-	data, err := removeExtension(tbsData, oidExtensionCTPoison)
+	data, err := removeExtension(tbsData, rfc6962.OIDExtensionCTPoison)
 	if err != nil {
 		return nil, err
 	}
@@ -133,28 +129,20 @@ func BuildPrecertTBS(tbsData []byte, preIssuer *x509.Certificate) ([]byte, error
 		// to that of the preIssuer.
 		var issuerKeyID []byte
 		for _, ext := range preIssuer.Extensions {
-			if ext.Id.Equal(oidExtensionAuthorityKeyId) {
+			if ext.Id.Equal(rfc6962.OIDExtAuthorityKeyId) {
 				issuerKeyID = ext.Value
 				break
 			}
 		}
 
-		// The x509 package does not parse CT EKU, so look for it in
-		// extensions directly.
-		seenCTEKU := false
-		for _, ext := range preIssuer.Extensions {
-			if ext.Id.Equal(oidExtensionKeyUsageCertificateTransparency) {
-				seenCTEKU = true
-				break
-			}
-		}
-		if !seenCTEKU {
+		// TODO(phbnf): is this check really necessary?
+		if !isPreIssuer(preIssuer) {
 			return nil, fmt.Errorf("issuer does not have CertificateTransparency extended key usage")
 		}
 
 		keyAt := -1
 		for i, ext := range tbs.Extensions {
-			if ext.Id.Equal(oidExtensionAuthorityKeyId) {
+			if ext.Id.Equal(rfc6962.OIDExtAuthorityKeyId) {
 				keyAt = i
 				break
 			}
@@ -169,7 +157,7 @@ func BuildPrecertTBS(tbsData []byte, preIssuer *x509.Certificate) ([]byte, error
 		} else if issuerKeyID != nil {
 			// PreCert did not have an auth-key-id, but the preIssuer does, so add it at the end.
 			authKeyIDExt := pkix.Extension{
-				Id:       oidExtensionAuthorityKeyId,
+				Id:       rfc6962.OIDExtAuthorityKeyId,
 				Critical: false,
 				Value:    issuerKeyID,
 			}
@@ -198,7 +186,6 @@ func RemoveCTPoison(tbsData []byte) ([]byte, error) {
 
 // EntryFromChain generates an Entry from a chain and timestamp.
 // copied from certificate-transparency-go/serialization.go
-// TODO(phboneff): add tests
 func EntryFromChain(chain []*x509.Certificate, isPrecert bool, timestamp uint64) (*ctonly.Entry, error) {
 	leaf := ctonly.Entry{
 		IsPrecert: isPrecert,
@@ -256,14 +243,35 @@ func EntryFromChain(chain []*x509.Certificate, isPrecert bool, timestamp uint64)
 	return &leaf, nil
 }
 
-// isPreIssuer indicates whether a certificate is a pre-cert issuer with the specific
-// certificate transparency extended key usage.
+// isPreIssuer indicates if a certificate is a precertificate signing cert.
+//
+// From RFC6962 s3.1, these certs should contain:
+// (CA:true, Extended Key Usage: Certificate Transparency, OID 1.3.6.1.4.1.11129.2.4.4)
 func isPreIssuer(cert *x509.Certificate) bool {
-	// Look for the extension in the Extensions field and not ExtKeyUsage
-	// since crypto/x509 does not recognize this extension as an ExtKeyUsage.
+	if !cert.IsCA {
+		return false
+	}
+	// Look for the extension in the Extensions field and not in ExtKeyUsage
+	// since crypto/x509 does not recognize this extension as such.
+	// We cannot reliably check in UnknownExtKeyUsage either, since it might
+	// one day disappear from UnknownExtKeyUsage and make it to ExtKeyUsage.
+	// Given that ExtKeyUsage does not contain OIDs, we would not be able to
+	// detect such a move.
 	for _, ext := range cert.Extensions {
-		if rfc6962.OIDExtKeyUsageCertificateTransparency.Equal(ext.Id) {
-			return true
+		if rfc6962.OIDExtKeyUsage.Equal(ext.Id) {
+			der := cryptobyte.String(ext.Value)
+			if !der.ReadASN1(&der, cryptobyte_asn1.SEQUENCE) {
+				continue
+			}
+			for !der.Empty() {
+				var eku asn1.ObjectIdentifier
+				if !der.ReadASN1ObjectIdentifier(&eku) {
+					continue
+				}
+				if rfc6962.OIDExtKeyUsageCertificateTransparency.Equal(eku) {
+					return true
+				}
+			}
 		}
 	}
 	return false