fix(x509): handle hex.DecodeString error in FormatOID() instead of discarding

Malformed hex strings in certificate subject fields could previously
produce corrupt decoded output because the error from hex.DecodeString
was discarded with `decoded, _ := ...`. In a compliance ledger this
is a security concern: an attacker-controlled or accidentally truncated
certificate could yield bogus normalized OIDs in indexed fields.

After this fix, malformed entries are skipped and the original (unmodified)
subject value remains in the output, so consumers see the raw OID rather
than a silently-corrupted normalized one.

Adds regression tests to x/pki/x509/x509_test.go that exercise the bug:
- Test_FormatOID_HexDecodeError: 5 cases covering non-hex chars,
  odd-length hex, embedded whitespace, mixed valid/invalid OIDs,
  and the positive-path regression guard.
- Test_FormatOID_NoCorruptOutput: explicit assertion that the buggy
  output pattern ("vid=0x" with empty/null bytes) cannot occur.

These tests fail with the pre-fix code and pass with the fix applied.

Per reviewer feedback: removed unrelated x/audit module changes from
this PR — they will be submitted separately if there's interest.

Signed-off-by: Srikanth Patchava <srpatcha@users.noreply.github.com>

Author: srpatcha

x/pki/x509/x509.go

@@ -153,7 +153,10 @@ func FormatOID(header, oldKey, newKey string) string {
 			// get value from header
 			value = value[len(value)-hexStringIntegerLength:]
 
-			decoded, _ := hex.DecodeString(value)
+			decoded, err := hex.DecodeString(value)
+			if err != nil {
+				continue
+			}
 
 			subjectValues[index] = fmt.Sprintf("%s=0x%s", newKey, decoded)
 		}
@@ -234,7 +237,7 @@ func ParseAndValidateCertificate(pemCertificate string) (*Certificate, error) {
 		serial := cert.SerialNumber
 		// RFC 5280 requires serial numbers to be positive
 		if serial.Sign() <= 0 {
-			return pkitypes.NewErrInvalidCertificate("serial number must be a positive")
+			return pkitypes.NewErrInvalidCertificate("serial number must be positive")
 		}
 
 		// When crypto/x509 parses a certificate, it reads the DER integer, strips the sign byte if present,

x/pki/x509/x509_test.go

@@ -336,3 +336,85 @@ func Test_ParseAndValidateCertificate(t *testing.T) {
 		}
 	}
 }
+
+// Test_FormatOID_HexDecodeError reproduces the bug fixed in this PR: previously
+// FormatOID() discarded the error from hex.DecodeString() with `decoded, _ := ...`,
+// causing malformed hex values in certificate subject fields to silently produce
+// corrupt decoded output (e.g. "vid=0x" with empty bytes). In a compliance ledger
+// this is a security concern because an attacker-controlled or accidentally
+// truncated certificate could yield bogus normalized OIDs.
+//
+// After the fix, malformed entries are skipped: the original value remains in
+// the output unchanged.
+func Test_FormatOID_HexDecodeError(t *testing.T) {
+	tests := []struct {
+		name   string
+		header string
+		oldKey string
+		newKey string
+		// Expected output: original entry must be preserved unchanged when
+		// hex.DecodeString cannot parse the trailing 8 chars.
+		expected string
+	}{
+		{
+			name:     "non-hex characters in last 8 chars are skipped",
+			header:   "CN=Matter PAA 1,O=Google,C=US,1.3.6.1.4.1.37244.2.1=#1304ZZNOTHEX",
+			oldKey:   "1.3.6.1.4.1.37244.2.1",
+			newKey:   "vid",
+			expected: "CN=Matter PAA 1,O=Google,C=US,1.3.6.1.4.1.37244.2.1=#1304ZZNOTHEX",
+		},
+		{
+			name:     "odd-length hex (invalid) is skipped",
+			header:   "CN=Matter PAA 1,1.3.6.1.4.1.37244.2.1=#13043600060",
+			oldKey:   "1.3.6.1.4.1.37244.2.1",
+			newKey:   "vid",
+			expected: "CN=Matter PAA 1,1.3.6.1.4.1.37244.2.1=#13043600060",
+		},
+		{
+			name:     "value containing whitespace yields decode error",
+			header:   "CN=Matter PAA 1,1.3.6.1.4.1.37244.2.1=# 1304600",
+			oldKey:   "1.3.6.1.4.1.37244.2.1",
+			newKey:   "vid",
+			expected: "CN=Matter PAA 1,1.3.6.1.4.1.37244.2.1=# 1304600",
+		},
+		{
+			name:     "valid hex still works (regression guard for positive path)",
+			header:   "CN=Matter PAA 1,O=Google,C=US,1.3.6.1.4.1.37244.2.1=#130436303036",
+			oldKey:   "1.3.6.1.4.1.37244.2.1",
+			newKey:   "vid",
+			expected: "CN=Matter PAA 1,O=Google,C=US,vid=0x6006",
+		},
+		{
+			name:     "multiple OIDs - one valid, one invalid",
+			header:   "CN=Test,1.3.6.1.4.1.37244.2.1=#130436303036,1.3.6.1.4.1.37244.2.2=#1304ZZZZZZ",
+			oldKey:   "1.3.6.1.4.1.37244.2.2",
+			newKey:   "pid",
+			expected: "CN=Test,1.3.6.1.4.1.37244.2.1=#130436303036,1.3.6.1.4.1.37244.2.2=#1304ZZZZZZ",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := FormatOID(tt.header, tt.oldKey, tt.newKey)
+			require.Equal(t, tt.expected, result,
+				"FormatOID must NOT silently emit corrupted output when hex.DecodeString fails")
+		})
+	}
+}
+
+// Test_FormatOID_NoCorruptOutput is an explicit assertion that the post-fix
+// behaviour cannot produce the buggy output pattern "newKey=0x" (empty hex)
+// which the pre-fix code could emit when hex.DecodeString returned an error.
+func Test_FormatOID_NoCorruptOutput(t *testing.T) {
+	// Crafted input where the last 8 chars contain a non-hex sentinel.
+	header := "CN=Test,1.3.6.1.4.1.37244.2.1=#1304NOTHEXX"
+	result := FormatOID(header, "1.3.6.1.4.1.37244.2.1", "vid")
+
+	require.NotContains(t, result, "vid=0x ",
+		"output must not contain 'vid=0x' followed by empty/whitespace bytes")
+	require.NotContains(t, result, "vid=0x\x00",
+		"output must not contain a null-byte after the hex prefix")
+	// Original entry must survive untouched.
+	require.Contains(t, result, "1.3.6.1.4.1.37244.2.1=#1304NOTHEXX",
+		"malformed entry must be preserved verbatim, not silently rewritten")
+}