DNSSECImpl.verifySignature compares strings incorrectly, allowing malicious zones to forge DNSSEC trust chain

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L186-L190

Vulnerability details

Impact

DNSSEC allows parent zones to sign for its child zones. To check validity of a signature, RFC4034 3.1.7 requires the Signer's Name in any RRSIG RDATA to contain the zone of covered RRset. This requirement is reasonable since any child zone should be covered by its parent zone.

ENS tries to implement the concept of name coverage in DNSSECImpl.verifySignature, but unfortuantely does it wrong, resulting in possibiliy of coverage between two unrelated domains. In the worst case, an attacker can utilize this bug to forge malicious trust chains and authenticate invalid domains.

Proof of Concept

In DNSSECImpl.verifySignature, ENS tries to verify the name of RRSet zone (name) is contained by Signer's Name (rrset.signerName).

    if(rrset.signerName.length > name.length
            || !rrset.signerName.equals(0, name, name.length - rrset.signerName.length))    //## This allows matches such as name="SubString.com" signerName="String.com", which is clearly incorrect, use label counts instead
        {
            revert InvalidSignerName(name, rrset.signerName);
        }

In DNS, for a parent zone to contain another child zone, we generally require the child zone to be a subdomain of the parent. For instance, example.eth. in considered to cover sub.example.eth., while xample.eth. should not be cover example.eth..

Unfortunately in the implementation shown above, both cases will path the check, and ample.eth. will be considered appropriate to sign for example.eth.. This is against the original design of DNS, and would result in breach of zone hierarchy.

In practice, the requirement to exploit this is a bit more complex. Since names are stored as a sequence of packed labels, example.eth. should be stored as \x06example\x03eth\x00, while xample.eth. is stored as \x05xample\x03eth\x00. Thus to successfully pull off the attack ,we have to make sure that the packed signer's name is actually a substring of child zone.

A simple (yet unrealistic) example can be like this xample.eth. can sign for e\x05xample.eth., since packed format of those two names are \x05xample\x03eth\x00 and \x07e\x05ample\x03eth\x00.

In general, it would require some effort for an attacker to find attackable zones, nevertheless, this should still be considered as a potential threat to the integrity of ENS.

Tools Used

None

Recommended Mitigation Steps

Check label by label instead of comparing the entire name.
To actually meet all requirements specified in RFC4034 and RFC4035, there are still a lot to do, but we will discuss that in a seperate issue for clarity.

[csanuragjain]

#61 seems to be similar and duplicate of this issue

[Arachnid]

This is a valid issue, but unexploitable in the wild; RFC 1035 specifies labels are limited to 63 octets or less, and the ASCII code of lowercase 'a' is 97. As a result, no vulnerable names should exist.

Recommend that this be triaged as severity 2 as a result.

[dmvt]

I agree with @Arachnid on this. The lack of real world likelihood makes this a Medium severity.