Skip to content

fix _loadVerified assembly offsets in CertManager#37

Open
Sertug17 wants to merge 1 commit into
base:mainfrom
Sertug17:main
Open

fix _loadVerified assembly offsets in CertManager#37
Sertug17 wants to merge 1 commit into
base:mainfrom
Sertug17:main

Conversation

@Sertug17

Copy link
Copy Markdown

_loadVerified was reading fields from wrong byte offsets in the packed certificate metadata. abi.encodePacked lays out the fields as:

  ca(1) || notAfter(8) || maxPathLen(8) || subjectHash(32) || pubKey(48)

The assembly code was reading from:
  ca at offset 0x1 should be 0x0
  notAfter at offset 0x9 should be 0x1
  maxPathLen at offset 0x11 should be 0x9
  subjectHash at offset 0x31 should be 0x11

Only pubKey was correct since it uses slice() which starts at the right offset.

The warm cache path calls _loadVerified for every cert that has been previously verified. With these wrong offsets, every cached cert returns garbage values for ca, notAfter, maxPathLen and subjectHash. The notAfter being wrong means expiry checks in the warm path are comparing against random bytes rather than the actual certificate expiry date. The subjectHash being wrong means parent chain validation can fail or worse silently pass when it should not.

This also affects the NitroValidator flow since verifyCachedCertBundle relies on the warm cache path via verifyCACertWithHints and verifyClientCertWithHints.

_loadVerified was reading fields from wrong byte offsets in the packed
certificate metadata. abi.encodePacked lays out the fields as:

  ca(1) || notAfter(8) || maxPathLen(8) || subjectHash(32) || pubKey(48)

But the assembly code was reading from offset 0x1 for ca (should be
0x0), 0x9 for notAfter (should be 0x1), 0x11 for maxPathLen (should
be 0x9), and 0x31 for subjectHash (should be 0x11). Only pubKey was
correct since it uses slice().

The warm cache path reads cert metadata with these wrong offsets, so
every cached cert would get garbage values for ca, notAfter, maxPathLen
and subjectHash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant