crypto/x509: Validate certificates with unsorted SET values in RDNs

Author: kumarlokesh

src/crypto/x509/parser.go

@@ -150,14 +150,31 @@ func parseName(raw cryptobyte.String) (*pkix.RDNSequence, error) {
 	for !raw.Empty() {
 		var rdnSet pkix.RelativeDistinguishedNameSET
 		var set cryptobyte.String
-		if !raw.ReadASN1(&set, cryptobyte_asn1.SET) {
+		var rawSet cryptobyte.String
+
+		if !raw.ReadASN1Element(&rawSet, cryptobyte_asn1.SET) {
+			return nil, errors.New("x509: invalid RDNSequence")
+		}
+
+		if !rawSet.ReadASN1(&set, cryptobyte_asn1.SET) {
 			return nil, errors.New("x509: invalid RDNSequence")
 		}
+
+		var rawAttrs []cryptobyte.String
+
 		for !set.Empty() {
 			var atav cryptobyte.String
-			if !set.ReadASN1(&atav, cryptobyte_asn1.SEQUENCE) {
+			var rawAttr cryptobyte.String
+
+			if !set.ReadASN1Element(&rawAttr, cryptobyte_asn1.SEQUENCE) {
 				return nil, errors.New("x509: invalid RDNSequence: invalid attribute")
 			}
+			rawAttrs = append(rawAttrs, rawAttr)
+
+			if !rawAttr.ReadASN1(&atav, cryptobyte_asn1.SEQUENCE) {
+				return nil, errors.New("x509: invalid RDNSequence: invalid attribute")
+			}
+
 			var attr pkix.AttributeTypeAndValue
 			if !atav.ReadASN1ObjectIdentifier(&attr.Type) {
 				return nil, errors.New("x509: invalid RDNSequence: invalid attribute type")
@@ -175,6 +192,18 @@ func parseName(raw cryptobyte.String) (*pkix.RDNSequence, error) {
 			rdnSet = append(rdnSet, attr)
 		}
 
+		// Verify that the SET values are sorted according to DER encoding rules
+		// as required by X.690 section 11.6
+		if len(rawAttrs) > 1 {
+			for i := 1; i < len(rawAttrs); i++ {
+				// Compare each attribute with the previous one
+				// In DER, they must be in ascending order when compared as octet strings
+				if bytes.Compare(rawAttrs[i-1], rawAttrs[i]) > 0 {
+					return nil, errors.New("x509: invalid RDNSequence: SET values not in ascending order")
+				}
+			}
+		}
+
 		rdnSeq = append(rdnSeq, rdnSet)
 	}
 

src/crypto/x509/parser_test.go

@@ -6,6 +6,7 @@ package x509
 
 import (
 	"encoding/asn1"
+	"encoding/base64"
 	"encoding/pem"
 	"os"
 	"testing"
@@ -251,3 +252,20 @@ d5l1tRhScKu2NBgm74nYmJxJYgvuTA38wGhRrGU=
 		}
 	}
 }
+
+func TestUnsortedSETInRDN(t *testing.T) {
+	// This certificate has an unsorted SET in its RDN
+	certB64 := "MIIFFDCCAvygAwIBAgIUb6hhfTZ9YpBB9FUvC1IUFrL3KAgwDQYJKoZIhvcNAQELBQAwUjELMAkGA1UEBhMCQ04xCzAJBgNVBAgMAkJKMQ0wCwYDVQQKDARKZWZlMRUwEwYDVQQDDAxKZWZlIFJvb3QgQ0ExEDAOBgNVBAcTB0JlaWppbmcwHhcNMjUwNTE2MjEwMjE2WhcNMjYwNTE2MjEwMjE2WjBSMQswCQYDVQQGEwJDTjELMAkGA1UECAwCQkoxDTALBgNVBAoMBEplZmUxFTATBgNVBAMMDEplZmUgUm9vdCBDQTEQMA4GA1UEBxMHQmVpamluZzCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAONdnqNcvwTNTKLCJMQzfBW8CjfMRxZI96NU+AYvvwTaSlEXxGY93KD1HsrqXRb4lUhxXVSdbdGGtCwF20zKSoJmcikMW21+9dW6hxkDJVp/E2BKgb1nBJj7d0FgVZyEcjgX2xbHcUdvBJg5IB13MPxcfRfGdHJ8vbA3NFJGdxJgqGb1XQHuU5ql3UGK0UMYHoLAA8ZeUZ7RgdCXAyM2XxF5lXDfzn5/DrlcFbMCLtA4JpbU87QnTIZxWQQ0LLz+FJ/M6sqkTL+CsOWRKXH6TPcyXLCrjuDa7pM/8vVkCX/oeyqwMvYEYV/q+JPHQ34UdhX1g7/OXZh+nGcgV4USOQECAwEAAaOCAQEwgf4wHQYDVR0OBBYEFA2Dg0Oa1UgW3qF3Q6cvq6fvp5wlMIHBBgNVHSMEgbkwgbaAFA2Dg0Oa1UgW3qF3Q6cvq6fvp5wloYGApH4wfDELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDTALBgNVBAoMBEplZmUxGDAWBgNVBAMMD3d3dy5leGFtcGxlLmNvbTEXMBUGCSqGSIb3DQEJARYIQUBCLkMuRE2CFG+oYX02fWKQQfRVLwtSFBay9ygIMAwGA1UdEwQFMAMBAf8wCwYDVR0PBAQDAgEGMBEGCWCGSAGG+EIBAQQEAwIBBjANBgkqhkiG9w0BAQsFAAOCAQEAZkWrFDnDN7aJYxgaLbTxvPQiUEw56GZfYaEH/gHSfkUiWvW8/Ub6Gp0rb/UEwu/9pPvs6QnwqLwBHkBpZX6lF1f5ltBbNzPdFVgQN1GdvETofyqQOo3hRbZ3vfEP7Yro7qXWFmwJwM1lMgTWuPpwxeGOqKR0o8C0dEssPJePAJRQHQHyldQ5Ie96KgLqRjxqx/7A4EQyZ3j3kWGnEY+QiHEEH9SgJ/iVkFuQf479VdMVLgcP9eEF+eKczcHINIGLvYL/9XYxKmfKLIKcZTYpxHdXJRIGLQ27IbXdKeZG0l9+ztLNCkG5fqCDZosfYvN0CIIpkQDQxnPnV4MVOXUhZBVW5Q=="
+
+	der, err := base64.StdEncoding.DecodeString(certB64)
+	if err != nil {
+		t.Fatalf("Failed to decode certificate: %v", err)
+	}
+
+	_, err = ParseCertificate(der)
+	if err == nil {
+		t.Errorf("Expected ParseCertificate to fail due to unsorted SET values in RDN, but it succeeded")
+	} else if err.Error() != "x509: malformed certificate" {
+		t.Errorf("Expected error 'x509: malformed certificate', got: %v", err)
+	}
+}