diff --git a/src/CertManager.sol b/src/CertManager.sol index ef6be5a..29ecfd0 100644 --- a/src/CertManager.sol +++ b/src/CertManager.sol @@ -21,6 +21,8 @@ contract CertManager is ICertManager { error InvalidExtension(); error InvalidBasicConstraints(); error InvalidSubjectPublicKey(); + error InvalidCertAlgorithm(); + error InvalidCertVersion(); error UnsupportedCriticalExtension(); error NotOwner(); error NotRevoker(); @@ -306,7 +308,7 @@ contract CertManager is ICertManager { Asn1Ptr root = certificate.root(); _requireAsn1Tag(certificate, root, 0x30); - require(root.totalLength() == certificate.length, "invalid cert length"); + if (root.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length(); Asn1Ptr tbsCertPtr = certificate.firstChildOf(root); _requireAsn1Tag(certificate, tbsCertPtr, 0x30); return certificate.keccak(tbsCertPtr.header(), tbsCertPtr.totalLength()); @@ -329,7 +331,7 @@ contract CertManager is ICertManager { bytes memory signatureHints ) internal view returns (VerifiedCert memory cert, bytes32 identity) { Asn1Ptr root = certificate.root(); - require(root.totalLength() == certificate.length, "invalid cert length"); + if (root.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length(); Asn1Ptr tbsCertPtr = certificate.firstChildOf(root); (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey) = _parseTbs(certificate, tbsCertPtr, ca); @@ -373,20 +375,20 @@ contract CertManager is ICertManager { _requireAsn1Tag(certificate, ptr, 0x30); Asn1Ptr versionPtr = certificate.firstChildOf(ptr); _requireAsn1Tag(certificate, versionPtr, 0xa0); - Asn1Ptr vPtr = certificate.firstChildOf(versionPtr); - Asn1Ptr serialPtr = certificate.nextSiblingOf(versionPtr); - Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(serialPtr); + Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(certificate.nextSiblingOf(versionPtr)); _requireAsn1Tag(certificate, sigAlgoPtr, 0x30); - require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo"); - uint256 version = certificate.uintAt(vPtr); + if (certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) != CERT_ALGO_OID) { + revert InvalidCertAlgorithm(); + } // as extensions are used in cert, version should be 3 (value 2) as per https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.1 - require(version == 2, "version should be 3"); + if (certificate.uintAt(certificate.firstChildOf(versionPtr)) != 2) revert InvalidCertVersion(); - (notAfter, maxPathLen, issuerHash, subjectHash, pubKey) = _parseTbsInner(certificate, sigAlgoPtr, ca); + (notAfter, maxPathLen, issuerHash, subjectHash, pubKey) = + _parseTbsInner(certificate, sigAlgoPtr, ca, ptr.content() + ptr.length()); } - function _parseTbsInner(bytes memory certificate, Asn1Ptr sigAlgoPtr, bool ca) + function _parseTbsInner(bytes memory certificate, Asn1Ptr sigAlgoPtr, bool ca, uint256 tbsEnd) internal view returns (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey) @@ -411,6 +413,7 @@ contract CertManager is ICertManager { // skip optional subjectUniqueID extensionsPtr = certificate.nextSiblingOf(extensionsPtr); } + if (extensionsPtr.content() + extensionsPtr.length() != tbsEnd) revert Asn1Decode.InvalidAsn1Length(); notAfter = _verifyValidity(certificate, validityPtr); maxPathLen = _verifyExtensions(certificate, extensionsPtr, ca); @@ -465,40 +468,48 @@ contract CertManager is ICertManager { returns (int64 maxPathLen) { if (certificate[extensionsPtr.header()] != 0xa3) revert InvalidExtension(); + uint256 end = extensionsPtr.content() + extensionsPtr.length(); extensionsPtr = certificate.firstChildOf(extensionsPtr); _requireAsn1Tag(certificate, extensionsPtr, 0x30); + if (extensionsPtr.content() + extensionsPtr.length() != end) revert InvalidExtension(); Asn1Ptr extensionPtr = certificate.firstChildOf(extensionsPtr); - uint256 end = extensionsPtr.content() + extensionsPtr.length(); bool basicConstraintsFound = false; bool keyUsageFound = false; maxPathLen = -1; while (true) { + uint256 extensionEnd = extensionPtr.content() + extensionPtr.length(); + if (extensionEnd > end) revert InvalidExtension(); _requireAsn1Tag(certificate, extensionPtr, 0x30); Asn1Ptr oidPtr = certificate.firstChildOf(extensionPtr); bytes32 oid = certificate.keccak(oidPtr.content(), oidPtr.length()); - Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr); bool recognized = oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID; + Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr); + if (certificate[valuePtr.header()] == 0x01) { if (valuePtr.length() != 1) revert InvalidExtension(); if (!recognized && certificate[valuePtr.content()] != 0x00) revert UnsupportedCriticalExtension(); valuePtr = certificate.nextSiblingOf(valuePtr); } + if (valuePtr.content() + valuePtr.length() != extensionEnd) revert InvalidExtension(); + if (recognized) { valuePtr = certificate.octetString(valuePtr); if (oid == BASIC_CONSTRAINTS_OID) { + if (basicConstraintsFound) revert InvalidExtension(); basicConstraintsFound = true; maxPathLen = _verifyBasicConstraintsExtension(certificate, valuePtr, ca); } else { + if (keyUsageFound) revert InvalidExtension(); keyUsageFound = true; _verifyKeyUsageExtension(certificate, valuePtr, ca); } } - if (extensionPtr.content() + extensionPtr.length() == end) { + if (extensionEnd == end) { break; } extensionPtr = certificate.nextSiblingOf(extensionPtr); @@ -562,9 +573,9 @@ contract CertManager is ICertManager { // X.509 KeyUsage bits are MSB-first. bitstringUintAt keeps the first KeyUsage octet in the // low byte, so these masks continue to target the same bits for one- or multi-octet values. if (ca) { - require(value & 0x04 == 0x04, "CertSign must be present"); + if (value & 0x04 != 0x04) revert InvalidExtension(); } else { - require(value & 0x80 == 0x80, "DigitalSignature must be present"); + if (value & 0x80 != 0x80) revert InvalidExtension(); } } @@ -576,9 +587,11 @@ contract CertManager is ICertManager { ) internal view { Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(ptr); _requireAsn1Tag(certificate, sigAlgoPtr, 0x30); - require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo"); + if (certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) != CERT_ALGO_OID) { + revert InvalidCertAlgorithm(); + } Asn1Ptr sigPtr = certificate.nextSiblingOf(sigAlgoPtr); - require(sigPtr.header() + sigPtr.totalLength() == certificate.length, "trailing cert fields"); + if (sigPtr.header() + sigPtr.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length(); bytes memory hash = Sha2Ext.sha384(certificate, ptr.header(), ptr.totalLength()); bytes memory sigPacked = _certSignature(certificate, sigPtr); diff --git a/test/CertManager.t.sol b/test/CertManager.t.sol index eedc5c3..6db1293 100644 --- a/test/CertManager.t.sol +++ b/test/CertManager.t.sol @@ -40,14 +40,6 @@ contract CertManagerPubKeyHarness is CertManager { function parsePubKey(bytes memory subjectPublicKeyInfo) external pure returns (bytes memory) { return _parsePubKey(subjectPublicKeyInfo, subjectPublicKeyInfo.root()); } - - function parsePubKeyAt(bytes memory certificate, uint256 header, uint256 content, uint256 length) - external - pure - returns (bytes memory) - { - return _parsePubKey(certificate, LibAsn1Ptr.toAsn1Ptr(header, content, length)); - } } contract CertManagerExtensionsHarness is CertManager { @@ -127,6 +119,53 @@ contract CertManagerTest is Test { certManagerHarness.verifyBasicConstraints(hex"30020400", false); } + function test_ExtensionsRejectDuplicateBasicConstraints() public { + vm.expectRevert(CertManager.InvalidExtension.selector); + certManagerExtensionsHarness.verifyExtensions( + _extensions(bytes.concat(_basicConstraintsExtension(), _basicConstraintsExtension(), _keyUsageExtension())), + true + ); + } + + function test_ExtensionsRejectDuplicateKeyUsage() public { + vm.expectRevert(CertManager.InvalidExtension.selector); + certManagerExtensionsHarness.verifyExtensions( + _extensions(bytes.concat(_basicConstraintsExtension(), _keyUsageExtension(), _keyUsageExtension())), true + ); + } + + function test_ExtensionsRejectTrailingExtensionFields() public { + vm.expectRevert(CertManager.InvalidExtension.selector); + certManagerExtensionsHarness.verifyExtensions( + _extensions(bytes.concat(_basicConstraintsExtensionWithTrailingField(), _keyUsageExtension())), true + ); + } + + function test_ExtensionsRejectTrailingWrapperFields() public { + bytes memory inner = _derNode(0x30, bytes.concat(_basicConstraintsExtension(), _keyUsageExtension())); + + vm.expectRevert(CertManager.InvalidExtension.selector); + certManagerExtensionsHarness.verifyExtensions(_derNode(0xa3, bytes.concat(inner, hex"0500")), true); + } + + function test_ExtensionsRejectInnerSequencePastWrapper() public { + bytes memory inner = _derNode(0x30, bytes.concat(_basicConstraintsExtension(), _keyUsageExtension())); + bytes memory der = bytes.concat(bytes1(0xa3), bytes1(uint8(inner.length - 1)), inner); + + vm.expectRevert(CertManager.InvalidExtension.selector); + certManagerExtensionsHarness.verifyExtensions(der, true); + } + + function test_ParseTbsRejectsTrailingSignedFields() public { + vm.warp(1775145600); + CertManager cm = new CertManager(new P384Verifier()); + + bytes memory mutated = _appendTbsTrailingField(CB1); + + vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector); + cm.verifyCACertWithHints(mutated, keccak256(CB0), ""); + } + function test_ParsePubKeyAcceptsUncompressedP384Point() public view { bytes memory pubKey = _patternBytes(96); bytes memory spki = abi.encodePacked(hex"3076301006072a8648ce3d020106052b8104002203620004", pubKey); @@ -137,10 +176,9 @@ contract CertManagerTest is Test { function test_ParsePubKeyRejectsCompressedP384Point() public { bytes memory compressedKey = _patternBytes(48); bytes memory spki = abi.encodePacked(hex"3046301006072a8648ce3d020106052b8104002203320002", compressedKey); - bytes memory paddedCertificate = abi.encodePacked(new bytes(128), spki); vm.expectRevert(CertManager.InvalidSubjectPublicKey.selector); - certManagerPubKeyHarness.parsePubKeyAt(paddedCertificate, 128, 130, 0x46); + certManagerPubKeyHarness.parsePubKey(spki); } function test_ParsePubKeyRejectsOversizedP384Point() public { @@ -370,6 +408,17 @@ contract CertManagerTest is Test { _writeDerLength(result, sigRoot, _addDelta(sigRoot.length(), delta)); } + function _appendTbsTrailingField(bytes memory certificate) internal pure returns (bytes memory result) { + Asn1Ptr root = certificate.root(); + Asn1Ptr tbsPtr = certificate.firstChildOf(root); + bytes memory nullField = hex"0500"; + int256 delta = int256(nullField.length); + result = _insertBytes(certificate, tbsPtr.content() + tbsPtr.length(), nullField); + + _writeDerLength(result, root, _addDelta(root.length(), delta)); + _writeDerLength(result, tbsPtr, _addDelta(tbsPtr.length(), delta)); + } + function _appendSignatureTrailingField(bytes memory certificate) internal pure returns (bytes memory result) { (Asn1Ptr root, Asn1Ptr sigPtr, Asn1Ptr sigRoot,) = _certSignaturePtrs(certificate); bytes memory extraInteger = hex"020100"; @@ -398,6 +447,33 @@ contract CertManagerTest is Test { } } + function _extensions(bytes memory extensionList) internal pure returns (bytes memory) { + return _derNode(0xa3, _derNode(0x30, extensionList)); + } + + function _basicConstraintsExtension() internal pure returns (bytes memory) { + return _derNode(0x30, bytes.concat(hex"0603551d13", hex"0101ff", _derNode(0x04, hex"30060101ff020100"))); + } + + function _basicConstraintsExtensionWithTrailingField() internal pure returns (bytes memory) { + return + _derNode(0x30, bytes.concat(hex"0603551d13", hex"0101ff", _derNode(0x04, hex"30060101ff020100"), hex"0500")); + } + + function _keyUsageExtension() internal pure returns (bytes memory) { + return _derNode(0x30, bytes.concat(hex"0603551d0f", hex"0101ff", _derNode(0x04, hex"03020186"))); + } + + function _derNode(bytes1 tag, bytes memory content) internal pure returns (bytes memory der) { + require(content.length < 128, "test: long-form length not supported"); + der = new bytes(2 + content.length); + der[0] = tag; + der[1] = bytes1(uint8(content.length)); + for (uint256 i = 0; i < content.length; ++i) { + der[2 + i] = content[i]; + } + } + function _certSignaturePtrs(bytes memory certificate) internal pure diff --git a/test/hinted/HintedNitroAttestation.t.sol b/test/hinted/HintedNitroAttestation.t.sol index 938362f..cb6f8e2 100644 --- a/test/hinted/HintedNitroAttestation.t.sol +++ b/test/hinted/HintedNitroAttestation.t.sol @@ -479,7 +479,7 @@ contract HintedNitroAttestationTest is Test { NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); (bytes memory caCert, bytes32 parentHash,) = _firstNonRootCA(attestationTbs, ptrs); - vm.expectRevert("invalid cert length"); + vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector); certManager.verifyCACertWithHints(abi.encodePacked(caCert, bytes1(0x00)), parentHash, ""); } @@ -511,7 +511,7 @@ contract HintedNitroAttestationTest is Test { bytes memory shadowRootHints = hintCollector.collectCertSignatureHints(shadowRoot, certManager.loadVerified(rootHash).pubKey); - vm.expectRevert("invalid cert length"); + vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector); certManager.verifyCACertWithHints(shadowRoot, rootHash, shadowRootHints); assertEq(certManager.loadVerified(shadowRootHash).pubKey.length, 0, "shadow root must not cache"); @@ -525,7 +525,7 @@ contract HintedNitroAttestationTest is Test { NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); (bytes memory caCert, bytes32 parentHash,) = _firstNonRootCA(attestationTbs, ptrs); - vm.expectRevert("trailing cert fields"); + vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector); certManager.verifyCACertWithHints(_appendInsideOuterSequence(caCert, bytes1(0x00)), parentHash, ""); }