Skip to content

fix: reject non-canonical ASN.1 lengths #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
leanthebean wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: reject non-canonical ASN.1 lengths #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Asn1Decode.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ library Asn1Decode {
}

function readNodeLength(bytes memory der, uint256 ix) private pure returns (Asn1Ptr) {
require(ix + 1 < der.length, "truncated ASN.1 header");
require(der[ix] & 0x1f != 0x1f, "ASN.1 tags longer than 1-byte are not supported");
uint256 length;
uint256 ixFirstContentByte;
Expand All @@ -239,6 +240,10 @@ library Asn1Decode {
ixFirstContentByte = ix + 2;
} else {
uint8 lengthbytesLength = uint8(der[ix + 1] & 0x7F);
require(lengthbytesLength != 0, "indefinite ASN.1 length");
require(lengthbytesLength <= 32, "ASN.1 length too long");
require(ix + 2 + lengthbytesLength <= der.length, "truncated ASN.1 length");
require(der[ix + 2] != 0, "non-canonical ASN.1 length");
if (lengthbytesLength == 1) {
length = uint8(der[ix + 2]);
} else if (lengthbytesLength == 2) {
Expand All @@ -247,8 +252,10 @@ library Asn1Decode {
length = uint256(readBytesN(der, ix + 2, lengthbytesLength) >> (32 - lengthbytesLength) * 8);
require(length <= 2 ** 64 - 1); // bound to max uint64 to be safe
}
require(length >= 128, "non-canonical ASN.1 length");
ixFirstContentByte = ix + 2 + lengthbytesLength;
}
require(ixFirstContentByte + length <= der.length, "ASN.1 node length exceeds input");
return LibAsn1Ptr.toAsn1Ptr(ix, ixFirstContentByte, length);
}

Expand Down
22 changes: 22 additions & 0 deletions test/Asn1Decode.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,28 @@ contract Asn1DecodeTest is Test {
h.rootLength(hex"0289ffffffffffffffffff"); // INTEGER, 9 length bytes all 0xff
}

function test_root_indefiniteLength_reverts() public {
vm.expectRevert("indefinite ASN.1 length");
h.rootLength(hex"0480"); // DER requires definite lengths
}

function test_root_longFormForShortLength_reverts() public {
vm.expectRevert("non-canonical ASN.1 length");
h.rootLength(hex"04810100"); // length 1 must use short form 0x01
}

function test_root_longFormLeadingZero_reverts() public {
vm.expectRevert("non-canonical ASN.1 length");
h.rootLength(hex"04820080"); // length 128 must be 0x81 0x80, not 0x82 0x00 0x80
}

function test_root_canonicalLongFormLength() public view {
bytes memory der = abi.encodePacked(bytes3(0x048180), new bytes(128));

assertEq(h.rootLength(der), 128);
assertEq(h.rootContent(der), 3);
}

// --- uintAt ---

function test_uintAt_value() public view {
Expand Down
28 changes: 28 additions & 0 deletions test/hinted/HintedNitroAttestation.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,19 @@ contract HintedNitroAttestationTest is Test {
certManager.verifyCACertWithHints(abi.encodePacked(caCert, bytes1(0x00)), parentHash, "");
}

function test_HintedCACertRejectsNonCanonicalOuterLength() public {
bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64()));
(bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation);
NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs);
(bytes memory caCert, bytes32 parentHash,) = _firstNonRootCA(attestationTbs, ptrs);
bytes memory nonCanonicalCert = _nonCanonicalOuterSequenceLength(caCert);
bytes32 nonCanonicalHash = keccak256(nonCanonicalCert);

vm.expectRevert("non-canonical ASN.1 length");
certManager.verifyCACertWithHints(nonCanonicalCert, parentHash, "");
assertEq(certManager.loadVerified(nonCanonicalHash).pubKey.length, 0, "non-canonical cert must not cache");
}

function test_HintedTrailingRootBytesCannotPoisonParentCache() public {
bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64()));
(bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation);
Expand Down Expand Up @@ -1255,6 +1268,21 @@ contract HintedNitroAttestationTest is Test {
output[3] = bytes1(uint8(length));
}

function _nonCanonicalOuterSequenceLength(bytes memory der) internal pure returns (bytes memory output) {
require(der.length >= 4 && der[0] == 0x30 && der[1] == 0x82, "test: expected long sequence");

output = new bytes(der.length + 1);
output[0] = der[0];
output[1] = 0x83;
output[2] = 0x00;
output[3] = der[2];
output[4] = der[3];

for (uint256 i = 4; i < der.length; ++i) {
output[i + 1] = der[i];
}
}

function _repairMissingPublicKeyBytes(bytes memory attestation) internal pure returns (bytes memory repaired) {
// The pasted Base64 sample is missing "ic_" in the CBOR key
// "public_key", but the key length and outer COSE payload length still
Expand Down
Loading