From 182307ac3bbdd0bd4ec1050de62b69f647d9fece Mon Sep 17 00:00:00 2001 From: Rajendra kadam Date: Fri, 15 May 2026 00:13:57 +0530 Subject: [PATCH] Add unit tests for Windows MDM WSTEP CSR parser helpers (#45457) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds unit tests for previously-uncovered parser helpers in `server/mdm/microsoft/wstep_csr.go`: - `parseBase128Int` — single/multi-byte values, non-minimal encoding (leading `0x80`), truncation, 5-byte cap, `MaxInt32` overflow - `parseTagAndLength` — short/long-form length, indefinite-length rejection, non-minimal length and tag, shift overflow - `parseSANExtension` — DNS, email, URI, IPv4, IPv6, plus malformed IP length and non-IA5 DNS - `domainToReverseLabels` — trailing dots, empty middle labels, single labels ASN.1 fixtures are built with `cryptobyte.Builder` to match the source file's existing toolchain — no new dependencies. Pure-Go tests, run in the `fast` bundle without Docker. **Related issue:** Refs #33373 # Checklist for submitter - [x] Added/updated automated tests ## Summary by CodeRabbit * **Tests** * Added comprehensive test coverage for certificate Subject Alternative Name (SAN) parsing: domain label reversal, base‑128 integer decoding with error classification, ASN.1 tag/length validation and boundary cases, end‑to‑end parsing of multiple SAN name types (DNS, email, IPv4/IPv6, URI), and numerous malformed/input edge‑case checks. [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/45457) --- server/mdm/microsoft/wstep_csr_test.go | 265 +++++++++++++++++++++++++ 1 file changed, 265 insertions(+) diff --git a/server/mdm/microsoft/wstep_csr_test.go b/server/mdm/microsoft/wstep_csr_test.go index eef70e4483..1da3886908 100644 --- a/server/mdm/microsoft/wstep_csr_test.go +++ b/server/mdm/microsoft/wstep_csr_test.go @@ -15,6 +15,8 @@ import ( "testing" "github.com/stretchr/testify/require" + "golang.org/x/crypto/cryptobyte" + cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1" ) func TestGetPublicKeyAlgorithmFromOID(t *testing.T) { @@ -199,3 +201,266 @@ var csrBase64Array = [...]string{ // Both [ v3_req ] and [ req_attributes ] "MIIDaTCCAlECAQAwfjELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDEUMBIGA1UEAwwLQ29tbW9uIE5hbWUxITAfBgkqhkiG9w0BCQEWEnRlc3RAZW1haWwuYWRkcmVzczCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAK1GY4YFx2ujlZEOJxQVYmsjUnLsd5nFVnNpLE4cV+77sgv9NPNlB8uhn3MXt5leD34rm/2BisCHOifPucYlSrszo2beuKhvwn4+2FxDmWtBEMu/QA16L5IvoOfYZm/gJTsPwKDqvaR0tTU67a9OtxwNTBMI56YKtmwd/o8d3hYv9cg+9ZGAZ/gKONcg/OWYx/XRh6bd0g8DMbCikpWgXKDsvvK1Nk+VtkDO1JxuBaj4Lz/p/MifTfnHoqHxWOWl4EaTs4Ychxsv34/rSj1KD1tJqorIv5Xv2aqv4sjxfbrYzX4kvS5SC1goIovLnhj5UjmQ3Qy8u65eow/LLWw+YFcCAwEAAaCBpTAgBgkqhkiG9w0BCQcxEwwRaWdub3JlZCBjaGFsbGVuZ2UwKAYJKoZIhvcNAQkCMRsMGWlnbm9yZWQgdW5zdHJ1Y3R1cmVkIG5hbWUwVwYJKoZIhvcNAQkOMUowSDAJBgNVHRMEAjAAMAsGA1UdDwQEAwIF4DAuBgNVHREEJzAlgRFnb3BoZXJAZ29sYW5nLm9yZ4IQdGVzdC5leGFtcGxlLmNvbTANBgkqhkiG9w0BAQsFAAOCAQEAgxe2N5O48EMsYE7o0rZBB0wi3Ov5/yYfnmmVI22Y3sP6VXbLDW0+UWIeSccOhzUCcZ/G4qcrfhhx6gTZTeA01nP7TdTJURvWAH5iFqj9sQ0qnLq6nEcVHij3sG6M5+BxAIVClQBk6lTCzgphc835Fjj6qSLuJ20XHdL5UfUbiJxx299CHgyBRL+hBUIPfz8p+ZgamyAuDLfnj54zzcRVyLlrmMLNPZNll1Q70RxoU6uWvLH8wB8vQe3Q/guSGubLyLRTUQVPh+dw1L4t8MKFWfX/48jwRM4gIRHFHPeAAE9D9YAoqdIvj/iFm/eQ++7DP8MDwOZWsXeB6jjwHuLmkQ==", } + +// TestDomainToReverseLabels covers the domain reverse-labels helper used by +// SAN URI validation. The function rejects domains with trailing dots or +// empty middle labels. +func TestDomainToReverseLabels(t *testing.T) { + for _, tc := range []struct { + name string + domain string + want []string + ok bool + }{ + {"three-label domain", "foo.example.com", []string{"com", "example", "foo"}, true}, + {"two-label domain", "example.com", []string{"com", "example"}, true}, + {"single label", "example", []string{"example"}, true}, + {"empty string yields no labels", "", nil, true}, + {"trailing dot is rejected", "foo.example.com.", nil, false}, + {"consecutive dots produce empty middle label", "foo..bar", nil, false}, + } { + t.Run(tc.name, func(t *testing.T) { + got, ok := domainToReverseLabels(tc.domain) + require.Equal(t, tc.ok, ok) + require.Equal(t, tc.want, got) + }) + } +} + +// TestParseBase128Int pins the boundary and error cases of the base-128 +// integer parser used inside parseTagAndLength. ASN.1 length-and-tag parsing +// bugs are a long-standing CVE class, so this function's guards deserve +// explicit coverage. +func TestParseBase128Int(t *testing.T) { + for _, tc := range []struct { + name string + bytes []byte + offset int + wantRet int + wantOffset int + errType string // "" = no error, "syntax", "structural" + }{ + { + name: "single-byte minimum", + bytes: []byte{0x05}, + wantRet: 5, + wantOffset: 1, + }, + { + name: "single-byte maximum without continuation", + bytes: []byte{0x7f}, + wantRet: 127, + wantOffset: 1, + }, + { + name: "multi-byte minimum above single-byte range", + bytes: []byte{0x81, 0x00}, + wantRet: 128, + wantOffset: 2, + }, + { + name: "non-minimal encoding rejected when leading byte is 0x80", + bytes: []byte{0x80, 0x01}, + errType: "syntax", + }, + { + name: "truncated when continuation bit set but no more bytes", + bytes: []byte{0x81}, + errType: "syntax", + }, + { + name: "rejected when integer exceeds five-byte limit", + bytes: []byte{0x81, 0x81, 0x81, 0x81, 0x81, 0x01}, + errType: "structural", + }, + { + name: "rejected when value exceeds MaxInt32", + bytes: []byte{0xff, 0xff, 0xff, 0xff, 0x7f}, + errType: "structural", + }, + } { + t.Run(tc.name, func(t *testing.T) { + ret, offset, err := parseBase128Int(tc.bytes, tc.offset) + switch tc.errType { + case "syntax": + var se asn1.SyntaxError + require.ErrorAs(t, err, &se, "expected asn1.SyntaxError") + case "structural": + var se asn1.StructuralError + require.ErrorAs(t, err, &se, "expected asn1.StructuralError") + default: + require.NoError(t, err) + require.Equal(t, tc.wantRet, ret) + require.Equal(t, tc.wantOffset, offset) + } + }) + } +} + +// TestParseTagAndLength pins the boundary and error cases of the ASN.1 tag +// and length parser. The DER-encoding rules around long-form length, minimal +// encoding, and indefinite-length rejection have all historically been +// sources of parser bugs. +func TestParseTagAndLength(t *testing.T) { + for _, tc := range []struct { + name string + bytes []byte + offset int + wantTag int + wantClass int + wantLen int + wantOffset int + errType string + }{ + { + name: "short tag and short length", + bytes: []byte{0x30, 0x05}, // SEQUENCE (tag 16), length 5 + wantTag: 0x10, + wantClass: 0, // universal + wantLen: 5, + wantOffset: 2, + }, + { + name: "long-form length encodes 256", + bytes: []byte{0x04, 0x82, 0x01, 0x00}, // OCTET STRING, length 256 in two bytes + wantTag: 0x04, + wantClass: 0, + wantLen: 256, + wantOffset: 4, + }, + { + name: "indefinite length rejected", + bytes: []byte{0x30, 0x80}, // SEQUENCE with indefinite length marker + errType: "syntax", + }, + { + name: "non-minimal length rejected (short value in long form)", + bytes: []byte{0x04, 0x81, 0x05}, // long-form encoding of 5 + errType: "structural", + }, + { + name: "length too large to shift safely", + bytes: []byte{0x04, 0x84, 0x80, 0x00, 0x00, 0x00}, // 4 bytes of length, overflows after shift + errType: "structural", + }, + { + name: "truncated length bytes", + bytes: []byte{0x04, 0x82, 0x01}, // long-form says 2 bytes follow, only 1 present + errType: "syntax", + }, + { + name: "non-minimal tag rejected", + bytes: []byte{0x1f, 0x05, 0x00}, // long-form tag header but value < 31 + errType: "syntax", + }, + } { + t.Run(tc.name, func(t *testing.T) { + ret, offset, err := parseTagAndLength(tc.bytes, tc.offset) + switch tc.errType { + case "syntax": + var se asn1.SyntaxError + require.ErrorAs(t, err, &se, "expected asn1.SyntaxError") + case "structural": + var se asn1.StructuralError + require.ErrorAs(t, err, &se, "expected asn1.StructuralError") + default: + require.NoError(t, err) + require.Equal(t, tc.wantTag, ret.tag) + require.Equal(t, tc.wantClass, ret.class) + require.Equal(t, tc.wantLen, ret.length) + require.Equal(t, tc.wantOffset, offset) + } + }) + } +} + +// TestParseSANExtension covers Subject Alternative Name parsing for each +// name type that wstep_csr supports (DNS, email, URI, IP) plus the error +// path for malformed IP length. SAN parser bugs are a notable CVE class. +func TestParseSANExtension(t *testing.T) { + // buildSAN encodes a single SAN extension SEQUENCE containing one entry + // with the given context-specific tag and bytes. + buildSAN := func(t *testing.T, contextTag byte, payload []byte) []byte { + t.Helper() + var b cryptobyte.Builder + b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) { + b.AddASN1(cryptobyte_asn1.Tag(contextTag).ContextSpecific(), func(b *cryptobyte.Builder) { + b.AddBytes(payload) + }) + }) + out, err := b.Bytes() + require.NoError(t, err) + return out + } + + t.Run("DNS name", func(t *testing.T) { + der := buildSAN(t, nameTypeDNS, []byte("test.example.com")) + dns, emails, ips, uris, err := parseSANExtension(der) + require.NoError(t, err) + require.Equal(t, []string{"test.example.com"}, dns) + require.Empty(t, emails) + require.Empty(t, ips) + require.Empty(t, uris) + }) + + t.Run("email address", func(t *testing.T) { + der := buildSAN(t, nameTypeEmail, []byte("gopher@golang.org")) + dns, emails, ips, uris, err := parseSANExtension(der) + require.NoError(t, err) + require.Empty(t, dns) + require.Equal(t, []string{"gopher@golang.org"}, emails) + require.Empty(t, ips) + require.Empty(t, uris) + }) + + t.Run("IPv4 address", func(t *testing.T) { + ipv4 := net.IPv4(127, 0, 0, 1).To4() + der := buildSAN(t, nameTypeIP, ipv4) + dns, emails, ips, uris, err := parseSANExtension(der) + require.NoError(t, err) + require.Empty(t, dns) + require.Empty(t, emails) + require.Len(t, ips, 1) + require.True(t, ips[0].Equal(ipv4)) + require.Empty(t, uris) + }) + + t.Run("IPv6 address", func(t *testing.T) { + ipv6 := net.ParseIP("2001:db8::1") + der := buildSAN(t, nameTypeIP, ipv6) + dns, emails, ips, uris, err := parseSANExtension(der) + require.NoError(t, err) + require.Empty(t, dns) + require.Empty(t, emails) + require.Len(t, ips, 1) + require.True(t, ips[0].Equal(ipv6)) + require.Empty(t, uris) + }) + + t.Run("URI", func(t *testing.T) { + der := buildSAN(t, nameTypeURI, []byte("https://example.com/path")) + dns, emails, ips, uris, err := parseSANExtension(der) + require.NoError(t, err) + require.Empty(t, dns) + require.Empty(t, emails) + require.Empty(t, ips) + require.Len(t, uris, 1) + require.Equal(t, "https://example.com/path", uris[0].String()) + }) + + t.Run("invalid IP length is rejected", func(t *testing.T) { + // 5 bytes is neither IPv4 (4) nor IPv6 (16). + der := buildSAN(t, nameTypeIP, []byte{1, 2, 3, 4, 5}) + _, _, _, _, err := parseSANExtension(der) + require.Error(t, err) + require.Contains(t, err.Error(), "cannot parse IP address") + }) + + t.Run("non-IA5 DNS name is rejected", func(t *testing.T) { + // 0x80 is outside the ASCII range that IA5String allows. + der := buildSAN(t, nameTypeDNS, []byte{'a', 0x80, 'b'}) + _, _, _, _, err := parseSANExtension(der) + require.Error(t, err) + require.Contains(t, err.Error(), "malformed") + }) +}