Bugfixes in X509 decoder (#1927)

seladb · web-flow · commit 2e891934ced1 · 2025-08-19T02:13:13.000-07:00
This PR fixes a few bugs:

- Support cases where the "version" field doesn't exist (the version in this case should be v1)
- Support cases where neither "extensions", "subject unique ID" and "issuer unique ID" exists
- Add tests for these cases
diff --git a/Packet++/header/X509Decoder.h b/Packet++/header/X509Decoder.h
@@ -596,10 +596,6 @@ namespace pcpp
 			int m_ExtensionsOffset = -1;
 
 			X509TBSCertificate(Asn1SequenceRecord* root);
-			int getIndex(int offset) const
-			{
-				return m_VersionOffset + offset;
-			}
 		};
 
 		/// @class X509Certificate
diff --git a/Packet++/src/X509Decoder.cpp b/Packet++/src/X509Decoder.cpp
@@ -749,23 +749,26 @@ namespace pcpp
 				m_SubjectOffset = currIndex++;
 				m_SubjectPublicKeyInfoOffset = currIndex++;
 
-				record = root->getSubRecords().at(currIndex);
-
-				if (record->getTagClass() == Asn1TagClass::ContextSpecific && record->getTagType() == 1)
+				if (root->getSubRecords().size() > static_cast<size_t>(currIndex))
 				{
-					m_IssuerUniqueID = currIndex++;
 					record = root->getSubRecords().at(currIndex);
-				}
 
-				if (record->getTagClass() == Asn1TagClass::ContextSpecific && record->getTagType() == 2)
-				{
-					m_SubjectUniqueID = currIndex++;
-					record = root->getSubRecords().at(currIndex);
-				}
-
-				if (X509Extensions::isValidExtensionsRecord(record))
-				{
-					m_ExtensionsOffset = currIndex++;
+					if (record->getTagClass() == Asn1TagClass::ContextSpecific && record->getTagType() == 1)
+					{
+						m_IssuerUniqueID = currIndex++;
+						record = root->getSubRecords().at(currIndex);
+					}
+
+					if (record->getTagClass() == Asn1TagClass::ContextSpecific && record->getTagType() == 2)
+					{
+						m_SubjectUniqueID = currIndex++;
+						record = root->getSubRecords().at(currIndex);
+					}
+
+					if (X509Extensions::isValidExtensionsRecord(record))
+					{
+						m_ExtensionsOffset = currIndex++;
+					}
 				}
 			}
 			catch (const std::out_of_range&)
@@ -788,25 +791,25 @@ namespace pcpp
 
 		X509Name X509TBSCertificate::getIssuer() const
 		{
-			auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, getIndex(m_IssuerOffset), "Issuer");
+			auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, m_IssuerOffset, "Issuer");
 			return X509Name(root);
 		}
 
 		X509Validity X509TBSCertificate::getValidity() const
 		{
-			auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, getIndex(m_ValidityOffset), "Validity");
+			auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, m_ValidityOffset, "Validity");
 			return X509Validity(root);
 		}
 
 		X509Name X509TBSCertificate::getSubject() const
 		{
-			auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, getIndex(m_SubjectOffset), "Subject");
+			auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, m_SubjectOffset, "Subject");
 			return X509Name(root);
 		}
 
 		X509SubjectPublicKeyInfo X509TBSCertificate::getSubjectPublicKeyInfo() const
 		{
-			auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, getIndex(m_SubjectPublicKeyInfoOffset),
+			auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, m_SubjectPublicKeyInfoOffset,
 			                                                    "Subject Public Key Info");
 			return X509SubjectPublicKeyInfo(root);
 		}
@@ -818,7 +821,7 @@ namespace pcpp
 				return nullptr;
 			}
 
-			auto root = getSubRecordAndCast<Asn1ConstructedRecord>(m_Root, getIndex(m_ExtensionsOffset), "Extensions");
+			auto root = getSubRecordAndCast<Asn1ConstructedRecord>(m_Root, m_ExtensionsOffset, "Extensions");
 			return std::unique_ptr<X509Extensions>(new X509Extensions(root));
 		}
 
@@ -1015,9 +1018,13 @@ namespace pcpp
 	{
 		if (!m_ExtensionsParsed)
 		{
-			for (const auto& extension : m_TBSCertificate.getExtensions()->getExtensions())
+			auto extensions = m_TBSCertificate.getExtensions();
+			if (extensions != nullptr)
 			{
-				m_Extensions.emplace_back(X509Extension(extension));
+				for (const auto& extension : extensions->getExtensions())
+				{
+					m_Extensions.emplace_back(X509Extension(extension));
+				}
 			}
 			m_ExtensionsParsed = true;
 		}
diff --git a/Tests/Packet++Test/PacketExamples/x509_cert_no_version_extension_fields.pem b/Tests/Packet++Test/PacketExamples/x509_cert_no_version_extension_fields.pem
@@ -0,0 +1,21 @@
+-----BEGIN CERTIFICATE-----
+MIIDdzCCAl8CFGJm3LFNJOTEx7xjXH9glmfasIw3MA0GCSqGSIb3DQEBCwUAMHcx
+CzAJBgNVBAYTAlVTMQswCQYDVQQIDAJOWTERMA8GA1UEBwwITmV3IFlvcmsxITAf
+BgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDElMCMGCSqGSIb3DQEJARYW
+cGNhcHBsdXNwbHVzQGdtYWlsLmNvbTAgFw0yNTA4MTcwODI3MDdaGA8yMTI1MDcy
+NDA4MjcwN1owdzELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAk5ZMREwDwYDVQQHDAhO
+ZXcgWW9yazEhMB8GA1UECgwYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMSUwIwYJ
+KoZIhvcNAQkBFhZwY2FwcGx1c3BsdXNAZ21haWwuY29tMIIBIjANBgkqhkiG9w0B
+AQEFAAOCAQ8AMIIBCgKCAQEAqDIKWbgU655OiCKNmrjU/yhiFMsx79ROgwn3s4XL
+XQzx0T1RkA4gA9JBLs4vqEvjg28eeTCGA9K360JjnjQztggWEU4/18UtM2U5ogNj
+BGmKLFAaz92wnzW2Ra7rUowMXl97SylhqhSBL4BsNZ4EntqL1Y4IcV29UVxOzCHY
+K2km0uwgedwJ7p0b7rQIeiZS1mSI1f1HQ/M9aeeV1gdT8OJRltVCvFkhY9p9L52Z
+0t6szCFARadqiZPWtoessQjK2p0N26wiW5rXPisqFt2lyDkXWQ16sk+xENukfb6h
+SiY5h20MPyNT3vOpkWq0jwqwIhittD18kcfqXJBul5714QIDAQABMA0GCSqGSIb3
+DQEBCwUAA4IBAQBe+gxZahT2J++/8ddn7MXqW4qKgcOeT1KWxvMj8Iwt6Ozmpsjs
+7c2e3iARS9JY0//7gXSKDJfrc1wowNab/eJ273oXZtneDahNpRqQ/fD8x1hW+oFW
+UTUPHNFIr1CrpfhtwT28hFxcPUNeEeiUpLnWuSLyTMm1Lb5+0xkovInBUKh/2p5t
+QO0E+sbAnRcFjpQd7pNsNz9kmcybxWHGjA8b7vRNNk3+2tLiMz/TGSnvQYU4gAOW
+ClA3U7E4zKVTR1WBEFQzfdMCHKpQPGtrqlnJRYhmSRm8cAHsKKI9OgceV7NWE2ZE
+trVp0+8pBZ9DExV5n2wtBHeaujDolG/TCGBr
+-----END CERTIFICATE-----
diff --git a/Tests/Packet++Test/Tests/X509Tests.cpp b/Tests/Packet++Test/Tests/X509Tests.cpp
@@ -329,6 +329,14 @@ PTF_TEST_CASE(X509VariantsParsingTest)
 		auto x509Cert = pcpp::X509Certificate::fromDERFile("PacketExamples/x509_cert_serial_lead_zeros.der");
 		PTF_ASSERT_EQUAL(x509Cert->getSerialNumber().toString(), "80");
 	}
+
+	// No version and extensions fields
+	{
+		auto x509Cert = pcpp::X509Certificate::fromPEMFile("PacketExamples/x509_cert_no_version_extension_fields.pem");
+		PTF_ASSERT_EQUAL(x509Cert->getVersion(), pcpp::X509Version::V1, enumclass);
+		PTF_ASSERT_EQUAL(x509Cert->getNotBefore().toString(), "2025-08-17 08:27:07");
+		PTF_ASSERT_EQUAL(x509Cert->getExtensions().size(), 0);
+	}
 }
 
 PTF_TEST_CASE(X509InvalidDataTest)

Original file line number	Diff line number	Diff line change
`@@ -749,23 +749,26 @@ namespace pcpp`
`749`	`749`	`m_SubjectOffset = currIndex++;`
`750`	`750`	`m_SubjectPublicKeyInfoOffset = currIndex++;`
`751`	`751`
`752`		`- record = root->getSubRecords().at(currIndex);`
`753`		`-`
`754`		`- if (record->getTagClass() == Asn1TagClass::ContextSpecific && record->getTagType() == 1)`
	`752`	`+ if (root->getSubRecords().size() > static_cast<size_t>(currIndex))`
`755`	`753`	`{`
`756`		`- m_IssuerUniqueID = currIndex++;`
`757`	`754`	`record = root->getSubRecords().at(currIndex);`
`758`		`- }`
`759`	`755`
`760`		`- if (record->getTagClass() == Asn1TagClass::ContextSpecific && record->getTagType() == 2)`
`761`		`- {`
`762`		`- m_SubjectUniqueID = currIndex++;`
`763`		`- record = root->getSubRecords().at(currIndex);`
`764`		`- }`
`765`		`-`
`766`		`- if (X509Extensions::isValidExtensionsRecord(record))`
`767`		`- {`
`768`		`- m_ExtensionsOffset = currIndex++;`
	`756`	`+ if (record->getTagClass() == Asn1TagClass::ContextSpecific && record->getTagType() == 1)`
	`757`	`+ {`
	`758`	`+ m_IssuerUniqueID = currIndex++;`
	`759`	`+ record = root->getSubRecords().at(currIndex);`
	`760`	`+ }`
	`761`	`+`
	`762`	`+ if (record->getTagClass() == Asn1TagClass::ContextSpecific && record->getTagType() == 2)`
	`763`	`+ {`
	`764`	`+ m_SubjectUniqueID = currIndex++;`
	`765`	`+ record = root->getSubRecords().at(currIndex);`
	`766`	`+ }`
	`767`	`+`
	`768`	`+ if (X509Extensions::isValidExtensionsRecord(record))`
	`769`	`+ {`
	`770`	`+ m_ExtensionsOffset = currIndex++;`
	`771`	`+ }`
`769`	`772`	`}`
`770`	`773`	`}`
`771`	`774`	`catch (const std::out_of_range&)`
`@@ -788,25 +791,25 @@ namespace pcpp`
`788`	`791`
`789`	`792`	`X509Name X509TBSCertificate::getIssuer() const`
`790`	`793`	`{`
`791`		`- auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, getIndex(m_IssuerOffset), "Issuer");`
	`794`	`+ auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, m_IssuerOffset, "Issuer");`
`792`	`795`	`return X509Name(root);`
`793`	`796`	`}`
`794`	`797`
`795`	`798`	`X509Validity X509TBSCertificate::getValidity() const`
`796`	`799`	`{`
`797`		`- auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, getIndex(m_ValidityOffset), "Validity");`
	`800`	`+ auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, m_ValidityOffset, "Validity");`
`798`	`801`	`return X509Validity(root);`
`799`	`802`	`}`
`800`	`803`
`801`	`804`	`X509Name X509TBSCertificate::getSubject() const`
`802`	`805`	`{`
`803`		`- auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, getIndex(m_SubjectOffset), "Subject");`
	`806`	`+ auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, m_SubjectOffset, "Subject");`
`804`	`807`	`return X509Name(root);`
`805`	`808`	`}`
`806`	`809`
`807`	`810`	`X509SubjectPublicKeyInfo X509TBSCertificate::getSubjectPublicKeyInfo() const`
`808`	`811`	`{`
`809`		`- auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, getIndex(m_SubjectPublicKeyInfoOffset),`
	`812`	`+ auto root = getSubRecordAndCast<Asn1SequenceRecord>(m_Root, m_SubjectPublicKeyInfoOffset,`
`810`	`813`	`"Subject Public Key Info");`
`811`	`814`	`return X509SubjectPublicKeyInfo(root);`
`812`	`815`	`}`
`@@ -818,7 +821,7 @@ namespace pcpp`
`818`	`821`	`return nullptr;`
`819`	`822`	`}`
`820`	`823`
`821`		`- auto root = getSubRecordAndCast<Asn1ConstructedRecord>(m_Root, getIndex(m_ExtensionsOffset), "Extensions");`
	`824`	`+ auto root = getSubRecordAndCast<Asn1ConstructedRecord>(m_Root, m_ExtensionsOffset, "Extensions");`
`822`	`825`	`return std::unique_ptr<X509Extensions>(new X509Extensions(root));`
`823`	`826`	`}`
`824`	`827`
`@@ -1015,9 +1018,13 @@ namespace pcpp`
`1015`	`1018`	`{`
`1016`	`1019`	`if (!m_ExtensionsParsed)`
`1017`	`1020`	`{`
`1018`		`- for (const auto& extension : m_TBSCertificate.getExtensions()->getExtensions())`
	`1021`	`+ auto extensions = m_TBSCertificate.getExtensions();`
	`1022`	`+ if (extensions != nullptr)`
`1019`	`1023`	`{`
`1020`		`- m_Extensions.emplace_back(X509Extension(extension));`
	`1024`	`+ for (const auto& extension : extensions->getExtensions())`
	`1025`	`+ {`
	`1026`	`+ m_Extensions.emplace_back(X509Extension(extension));`
	`1027`	`+ }`
`1021`	`1028`	`}`
`1022`	`1029`	`m_ExtensionsParsed = true;`
`1023`	`1030`	`}`
Original file line number	Diff line number	Diff line change
`@@ -329,6 +329,14 @@ PTF_TEST_CASE(X509VariantsParsingTest)`
`329`	`329`	`auto x509Cert = pcpp::X509Certificate::fromDERFile("PacketExamples/x509_cert_serial_lead_zeros.der");`
`330`	`330`	`PTF_ASSERT_EQUAL(x509Cert->getSerialNumber().toString(), "80");`
`331`	`331`	`}`
	`332`	`+`
	`333`	`+ // No version and extensions fields`
	`334`	`+ {`
	`335`	`+ auto x509Cert = pcpp::X509Certificate::fromPEMFile("PacketExamples/x509_cert_no_version_extension_fields.pem");`
	`336`	`+ PTF_ASSERT_EQUAL(x509Cert->getVersion(), pcpp::X509Version::V1, enumclass);`
	`337`	`+ PTF_ASSERT_EQUAL(x509Cert->getNotBefore().toString(), "2025-08-17 08:27:07");`
	`338`	`+ PTF_ASSERT_EQUAL(x509Cert->getExtensions().size(), 0);`
	`339`	`+ }`
`332`	`340`	`}`
`333`	`341`
`334`	`342`	`PTF_TEST_CASE(X509InvalidDataTest)`