Request validation hardening#153
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements strict canonical CBOR validation for certification requests to ensure deterministic encoding and prevent malleability. It introduces a new canonicalCBORValidator that enforces RFC 8949 Core Deterministic form by rejecting non-shortest integer encodings, trailing data, and out-of-order map keys. Additionally, the PR refactors hashing functions for StateID and CertificationData to use explicit structs for consistent marshaling and adds rigorous length validation for state and transaction hashes. Review feedback suggests optimizing integer parsing using the binary package and hardening simple value checks within the CBOR validator to reject reserved values.
| return nil | ||
| } | ||
|
|
||
| const maxCanonicalCBORDepth = 64 |
There was a problem hiding this comment.
the code below is canonical CBOR handling, should be extracted somewhere
There was a problem hiding this comment.
Extracted the generic canonical CBOR validation into pkg/cbor, so it is no longer certification-request specific
|
|
||
| // UnmarshalCanonicalCertificationRequestCBOR decodes data into out and rejects | ||
| // the input if it is not in canonical Core Deterministic CBOR form. | ||
| func UnmarshalCanonicalCertificationRequestCBOR(data []byte, out *CertificationRequest) error { |
There was a problem hiding this comment.
move this to pkg/api/certification_request.go. func name looks like an overkill. maybe just UnmarshalCertificationRequestCBOR (comment explains the expectation to be canonical)
There was a problem hiding this comment.
Moved the request-specific wrapper to certification_request.go and renamed it to UnmarshalCertificationRequestCBOR
| const maxCanonicalCBORDepth = 64 | ||
|
|
||
| const ( | ||
| cborMajorUnsignedInt byte = 0 |
There was a problem hiding this comment.
I haven't checked but shouldn't these consts and functions come from some existing cbor lib? or maybe we need to extract one and share it between bft and aggregator..
There was a problem hiding this comment.
The CBOR library has equivalent internal constants/parsing logic, but they are not exported for us to reuse directly. I moved the constants and validation code into a generic pkg/cbor package instead of keeping them in certification-request code, SDK handles this the same way
3 participants
Closes #150