From 81ed1389d4e43517c8e52b02679acdb654bd4ee2 Mon Sep 17 00:00:00 2001 From: Lokesh Kumar Date: Thu, 5 Jun 2025 22:40:18 +0200 Subject: [PATCH] crypto/x509: Apply code review improvements --- src/crypto/x509/parser.go | 34 +++++++++---------------------- src/crypto/x509/parser_test.go | 37 +++++++++++++++++++++++++++------- 2 files changed, 40 insertions(+), 31 deletions(-) diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go index 455f4ed5f2..5fb0bd3754 100644 --- a/src/crypto/x509/parser.go +++ b/src/crypto/x509/parser.go @@ -150,26 +150,24 @@ func parseName(raw cryptobyte.String) (*pkix.RDNSequence, error) { for !raw.Empty() { var rdnSet pkix.RelativeDistinguishedNameSET var set cryptobyte.String - var rawSet cryptobyte.String - - if !raw.ReadASN1Element(&rawSet, cryptobyte_asn1.SET) { + if !raw.ReadASN1(&set, 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 - + var prevAttr cryptobyte.String for !set.Empty() { var atav cryptobyte.String var rawAttr cryptobyte.String - if !set.ReadASN1Element(&rawAttr, cryptobyte_asn1.SEQUENCE) { return nil, errors.New("x509: invalid RDNSequence: invalid attribute") } - rawAttrs = append(rawAttrs, rawAttr) + + // Compare each attribute with the previous one + // In DER, they must be in ascending order when compared as octet strings + if prevAttr != nil && bytes.Compare(prevAttr, rawAttr) > 0 { + return nil, errors.New("x509: invalid RDNSequence: SET values not in ascending order") + } + + prevAttr = rawAttr if !rawAttr.ReadASN1(&atav, cryptobyte_asn1.SEQUENCE) { return nil, errors.New("x509: invalid RDNSequence: invalid attribute") @@ -192,18 +190,6 @@ 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) } diff --git a/src/crypto/x509/parser_test.go b/src/crypto/x509/parser_test.go index 86d98ef343..0e2b4ae8d9 100644 --- a/src/crypto/x509/parser_test.go +++ b/src/crypto/x509/parser_test.go @@ -6,7 +6,6 @@ package x509 import ( "encoding/asn1" - "encoding/base64" "encoding/pem" "os" "testing" @@ -255,17 +254,41 @@ 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==" + certPEM := `-----BEGIN CERTIFICATE----- +MIIFFDCCAvygAwIBAgIUb6hhfTZ9YpBB9FUvC1IUFrL3KAgwDQYJKoZIhvcNAQEL +BQAwUjELMAkGA1UEBhMCQ04xCzAJBgNVBAgMAkJKMQ0wCwYDVQQKDARKZWZlMRUw +EwYDVQQDDAxKZWZlIFJvb3QgQ0ExEDAOBgNVBAcTB0JlaWppbmcwHhcNMjUwNTE2 +MjEwMjE2WhcNMjYwNTE2MjEwMjE2WjBSMQswCQYDVQQGEwJDTjELMAkGA1UECAwC +QkoxDTALBgNVBAoMBEplZmUxFTATBgNVBAMMDEplZmUgUm9vdCBDQTEQMA4GA1UE +BxMHQmVpamluZzCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAONdnqNc +vwTNTKLCJMQzfBW8CjfMRxZI96NU+AYvvwTaSlEXxGY93KD1HsrqXRb4lUhxXVSd +bdGGtCwF20zKSoJmcikMW21+9dW6hxkDJVp/E2BKgb1nBJj7d0FgVZyEcjgX2xbH +cUdvBJg5IB13MPxcfRfGdHJ8vbA3NFJGdxJgqGb1XQHuU5ql3UGK0UMYHoLAA8Ze +UZ7RgdCXAyM2XxF5lXDfzn5/DrlcFbMCLtA4JpbU87QnTIZxWQQ0LLz+FJ/M6sqk +TL+CsOWRKXH6TPcyXLCrjuDa7pM/8vVkCX/oeyqwMvYEYV/q+JPHQ34UdhX1g7/O +XZh+nGcgV4USOQECAwEAAaOCAQEwgf4wHQYDVR0OBBYEFA2Dg0Oa1UgW3qF3Q6cv +q6fvp5wlMIHBBgNVHSMEgbkwgbaAFA2Dg0Oa1UgW3qF3Q6cvq6fvp5wloYGApH4w +fDELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcMDVNh +biBGcmFuY2lzY28xDTALBgNVBAoMBEplZmUxGDAWBgNVBAMMD3d3dy5leGFtcGxl +LmNvbTEXMBUGCSqGSIb3DQEJARYIQUBCLkMuRE2CFG+oYX02fWKQQfRVLwtSFBay +9ygIMAwGA1UdEwQFMAMBAf8wCwYDVR0PBAQDAgEGMBEGCWCGSAGG+EIBAQQEAwIB +BjANBgkqhkiG9w0BAQsFAAOCAQEAZkWrFDnDN7aJYxgaLbTxvPQiUEw56GZfYaEH +/gHSfkUiWvW8/Ub6Gp0rb/UEwu/9pPvs6QnwqLwBHkBpZX6lF1f5ltBbNzPdFVgQ +N1GdvETofyqQOo3hRbZ3vfEP7Yro7qXWFmwJwM1lMgTWuPpwxeGOqKR0o8C0dEss +PJePAJRQHQHyldQ5Ie96KgLqRjxqx/7A4EQyZ3j3kWGnEY+QiHEEH9SgJ/iVkFuQ +f479VdMVLgcP9eEF+eKczcHINIGLvYL/9XYxKmfKLIKcZTYpxHdXJRIGLQ27IbXd +KeZG0l9+ztLNCkG5fqCDZosfYvN0CIIpkQDQxnPnV4MVOXUhZBVW5Q== +-----END CERTIFICATE-----` - der, err := base64.StdEncoding.DecodeString(certB64) - if err != nil { - t.Fatalf("Failed to decode certificate: %v", err) + block, _ := pem.Decode([]byte(certPEM)) + if block == nil { + t.Fatalf("Failed to decode PEM block") } - _, err = ParseCertificate(der) + _, err := ParseCertificate(block.Bytes) 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) } -} \ No newline at end of file +}