c4-bot-9 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/automata-attestation/AutomataDcapV3Attestation.sol#L263-L271 https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/automata-attestation/AutomataDcapV3Attestation.sol#L300 https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/automata-attestation/AutomataDcapV3Attestation.sol#L84

Vulnerability details

Impact

Invalid Verification of Certificate Chain as Revoked certificate would pass though verification unchecked when Certificate array length is 1 or when the last Certificate in the certs Array is a Revoked Certificate

Proof of Concept

   function addRevokedCertSerialNum(
        uint256 index,
        bytes[] calldata serialNumBatch
    )
        external
        onlyOwner
    {
        for (uint256 i; i < serialNumBatch.length; ++i) {
            if (_serialNumIsRevoked[index][serialNumBatch[i]]) {
                continue;
            }
>>>            _serialNumIsRevoked[index][serialNumBatch[i]] = true;
        }
    }

The function above from the AutomataDcapV3Attestation contract shows how certificate are revoked by owner using their serial number. While the pointers in the _verifyCertChain(...) function from code provided below from the same contract shows how this certificates are checked to ensure they are not revoked, this can be noted from the very first pointer i.e certRevoked. if any of the cert in the array is a revoked one, the verification returns false. From the implementation in the contract, under the circumstance that certRevoked is true the verification returns false i.e failed verification as noted from the implementation in the last pointer. The problem is that whenever the certs array is just 1 no check is done to see if this lone cert is a revoked cert, which means certRevoked will use the default value of false when it should actually be true. A deep analysis of the function also shows that when the certs array length is more than 1 and the last cert in the array is a revoked one, it would not go though validation which would allow verification when certificate is actually revoked

  function _verifyCertChain(IPEMCertChainLib.ECSha256Certificate[] memory certs)
        private
        view
        returns (bool)
    {
        uint256 n = certs.length;
>>>        bool certRevoked;
        bool certNotExpired;
        bool verified;
        bool certChainCanBeTrusted;

        for (uint256 i; i < n; ++i) {
            IPEMCertChainLib.ECSha256Certificate memory issuer;
            if (i == n - 1) {
                // rootCA
>>>                issuer = certs[i]; 
///@audit issuer is assigned a certificate, Revoked or not, 
///@audit Revoked cert not checked if certs array is one or if the revoked cert is the last cert in the certs array
            } else {
>>>                issuer = certs[i + 1];
                if (i == n - 2) {
                    // this cert is expected to be signed by the root
>>>                    certRevoked = _serialNumIsRevoked[uint256(IPEMCertChainLib.CRL.ROOT)][certs[i]
                        .serialNumber];
                } else if (certs[i].isPck) {
>>>                   certRevoked = _serialNumIsRevoked[uint256(IPEMCertChainLib.CRL.PCK)][certs[i]
                        .serialNumber];
                }
                if (certRevoked) {
                    break;
                }
            }

            certNotExpired =
                block.timestamp > certs[i].notBefore && block.timestamp < certs[i].notAfter;
            if (!certNotExpired) {
                break;
            }

            verified = sigVerifyLib.verifyES256Signature(
                certs[i].tbsCertificate, certs[i].signature, issuer.pubKey
            );
            if (!verified) {
                break;
            }

            bytes32 issuerPubKeyHash = keccak256(issuer.pubKey);

            if (issuerPubKeyHash == ROOTCA_PUBKEY_HASH) {
                certChainCanBeTrusted = true;
                break;
            }
        }

>>>        return !certRevoked && certNotExpired && verified && certChainCanBeTrusted;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

As adjusted in the code below Protocol should make sure a Certification validation is done for certs under every circumstances.

  function _verifyCertChain(IPEMCertChainLib.ECSha256Certificate[] memory certs)
        private
        view
        returns (bool)
    {
        uint256 n = certs.length;
        bool certRevoked;
        bool certNotExpired;
        bool verified;
        bool certChainCanBeTrusted;

        for (uint256 i; i < n; ++i) {
            IPEMCertChainLib.ECSha256Certificate memory issuer;
            if (i == n - 1) {
                // rootCA
                issuer = certs[i];
+++             certRevoked = _serialNumIsRevoked[uint256(IPEMCertChainLib.CRL.ROOT)][certs[i]
+++                        .serialNumber];
            } else {
                issuer = certs[i + 1];
                if (i == n - 2) {
                    // this cert is expected to be signed by the root
                    certRevoked = _serialNumIsRevoked[uint256(IPEMCertChainLib.CRL.ROOT)][certs[i]
                        .serialNumber];
                } else if (certs[i].isPck) {
                    certRevoked = _serialNumIsRevoked[uint256(IPEMCertChainLib.CRL.PCK)][certs[i]
                        .serialNumber];
                }
                if (certRevoked) {
                    break;
                }
            }

            ...
        }

        return !certRevoked && certNotExpired && verified && certChainCanBeTrusted;
    }

Assessed type

Invalid Validation

c4-pre-sort commented 5 months ago

minhquanym marked the issue as sufficient quality report

smtmfft commented 5 months ago

The Cert chain follows this architecture: (from: https://download.01.org/intel-sgx/sgx-dcap/1.9/linux/docs/Intel_SGX_PCK_Certificate_CRL_Spec-1.4.pdf)

so this n is forced to be 3 when enter this function which is pre-checked in input validation phase. Then the n-1(certChain[2]) which is the intel ROOT CA is never revoked, and we only need to check the n-2(certChain[1]) the ROOT signed revoked list & n-3(certChain[0]) the PCK signed revoded list.

c4-sponsor commented 5 months ago

dantaik (sponsor) disputed

0xean commented 5 months ago

Certificate array length is 1

Warden doesn't show how this expected state will be reached based on the design. I think QA is probably more appropriate based on this fact

c4-judge commented 5 months ago

0xean changed the severity to QA (Quality Assurance)