Skip to content

fix(cose): reject undecodable CBOR values in the unprotected header - #411

Open
rajnisht7 wants to merge 2 commits into
agentrust-io:mainfrom
rajnisht7:fix-cose
Open

fix(cose): reject undecodable CBOR values in the unprotected header#411
rajnisht7 wants to merge 2 commits into
agentrust-io:mainfrom
rajnisht7:fix-cose

Conversation

@rajnisht7

Copy link
Copy Markdown
Contributor

What

Reject a malformed CBOR value (cbor2's internal break-marker sentinel) in the unprotected header before it can crash attach_receipt/attach_attestation/attach_approvals on re-encode.

Why

fuzz_cose found an input where the unprotected header decodes fine but contains a value cbor2 can't re-encode. attach_unprotected() copies that header and calls cbor2.dumps(), which raises CBOREncodeError an exception outside the documented CoseError contract, so except CoseError doesn't catch it.

Spec impact

None

Test plan

  • pytest -v passes
  • mypy src/agent_manifest passes
  • ruff check src/ tests/ passes
  • New or updated tests cover the change
  • If spec change: CHANGELOG.md updated

DCO

All commits in this PR are signed off (git commit -s). By submitting this PR I certify the Developer Certificate of Origin.

Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>

@imran-siddique imran-siddique left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The part that earns this is the discriminator, not the recursion.

Matching on type(value) is object rather than allow-listing the types a header may legitimately hold is the choice that makes the guard safe to put in front of every caller. cbor2's break marker is a bare object() and nothing a normal decode returns is, so the check cannot reject a manifest that decodes correctly, and it does not need revisiting when cbor2's tag support learns a new type. The docstring says exactly that, which is why I did not have to work it out from the code.

What I ran, in a clean venv on the fork head be71671:

  • pytest tests/: 1553 passed, 6 skipped.
  • mypy src/agent_manifest: no issues in 26 source files.
  • pytest tests/test_cose.py with src/agent_manifest/_cose.py reverted to main: 4 failed, test_unprotected_header_sentinel_is_checked_at_decode,
    test_attach_unprotected_rejects_a_caller_supplied_unencodable_value,
    test_reject_cbor_sentinels_recurses_into_cbortag_and_set,
    test_unprotected_header_sentinel_nested_in_tag_or_set_is_checked_at_decode. All four pass with the fix, so the tests are guarding the change rather than describing it.

That 1553 figure is the other half of the evidence: a new refusal added to a shared decode path is only safe if it fires on nothing that already worked, and it fires on nothing.

Repo CI: 14 distinct checks, all green including Fuzz changed code and Security scan. The only red entry is Policy: Awaiting maintainer review, which this approval clears.

One thing to expect, not a change request. This PR and #407 both add to CHANGELOG.md at line 17, so whichever lands first conflicts the other. I am merging this one now; #407 will need its changelog entry rebased and that is a mechanical fix, not a review round. #373's entry is further down the file and should be unaffected.

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.

2 participants