test: gate the JSON depth bound on the two grant entry points 0.10.0 added - #29
Merged
Merged
Conversation
Work in progress, committed as it stands so the review below has a baseline to move from. Four cases, all against the exported API: - `json_bounds`: answering a deep grant through `new_member_vmc_for` / `new_delegate_vdc_for`, and `verify_grant_with_public_key` on a deep grant, both on the 256 KiB stack the rest of the file uses. - `membership_edge`: a grant its own subject wrote satisfies the member check, and the same grant does not verify under the community's key. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
… is signed `a_grant_its_own_subject_wrote_does_not_verify` reached `DTGCredentialError::NotSigned` through the same two lines as `an_unsigned_grant_does_not_verify` directly above it: the depth check, then the absence of a top-level `proof`. Neither the document's provenance nor its subject is read before that error, so the two cases exercised identical code. The case that remains cross-references the one that was already there. `verifying_a_deep_grant_is_refused_without_exhausting_the_stack` signs the grant before attaching the deep member, which is load-bearing and was not explained: an unsigned grant is refused for having no proof before the verifier reaches the clone that strips it, so with the bound removed it would return an error rather than overflow, and the case would prove nothing. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Regression gates for the two SEC-4045 findings in this crate — H13 (a membership
acknowledgement built from an unverified grant) and M2 (unbounded JSON depth). Both were
fixed in #28; this adds the cases that hold the fixes in place, after checking what #27 and
#28 already gate.
Tests only. No library source changes, and no fix was weakened to suit a test:
git diff origin/main -- src/is empty.161 tests → 164 with
--all-features.Gap analysis
Most of what the PoC demonstrated was already gated by the suites that landed with the
fixes. Two entry points were not, both of them introduced by #28 itself.
H13 — acknowledgement from an unverified grant
new_member_vmcmints an ack from a forged, unsigned grant; the ack's issuer is the grant's subject, its subject the grant's issuer, and the digest binds the exact bytesmembership_edge::the_deprecated_constructor_checks_neither_the_member_nor_the_grant_expiryfor the deprecated path;binding_does_not_establish_that_the_grant_was_signedfor the_forpathacknowledges()accepts the forged pair, so the binding layer does not catch itmembership_edge::binding_does_not_establish_that_the_grant_was_signed;delegation_chain::an_acceptance_from_the_wrong_party_binds_nothingfor the VDC halfmembership_edge::verifying_the_grant::an_unsigned_grant_does_not_verifyanddelegation_chain::an_unsigned_grant_does_not_verify, both against the realverify_grant_with_public_keyrather than the PoC'ssigned()probe_forthe scenario is refused outright:membership_edge::a_grant_naming_someone_else_is_refused,delegation_chain::a_grant_appointing_someone_else_is_refused. Under the deprecated constructor the PoC's property is pinned bythe_deprecated_constructor_checks_neither_the_member_nor_the_grant_expirymembership_edge::a_grant_its_own_subject_wrote_satisfies_the_member_check. Not a gate; see Not a gate belowM2 — unbounded JSON depth
medium_unbounded_json_is_embedded_verbatim—new_vecembeds an unvalidated blob verbatim, with no check of shape, keys or sizejson_bounds::open_members_are_carried_verbatim_within_the_bound. Deliberate:endorsementis open vocabulary, so only its depth is boundedmedium_deep_json_serialization_stack_overflow_dos— a deependorsementaborts the processa_deep_endorsement_is_refused_without_exhausting_the_stack,an_endorsement_beyond_the_bound_is_refused,signing::sign_refuses_a_deep_endorsement,signing::verification_refuses_a_deep_member,a_deep_document_is_refused_without_exhausting_the_stack,deriving_from_a_deep_parent_is_refused_without_exhausting_the_stacknew_member_vmc_forandnew_delegate_vdc_forreach the digest only after the member and the grant's expiry have been read, so the bound has to hold at the end of that sequencejson_bounds::answering_a_deep_grant_is_refused_without_exhausting_the_stackverify_grant_with_public_key, also added by #28 — the one entry point handed a whole counterparty document before anything about it is establishedjson_bounds::signing::verifying_a_deep_grant_is_refused_without_exhausting_the_stackThe PoC's temporal finding
Not among the two findings this branch is for, but it appears in the same PoC file and is
fixed in #28, so it was checked:
new_vacaccepting an inverted window is gated byauthority_chain::an_inverted_window_is_refused_at_issueanda_window_narrower_than_a_second_is_refused, with the same shape indelegation_chain::an_inverted_window_is_refused_at_issueandmembership_edge::an_inverted_window_is_refused. The PoC's far-pastvalidFromis accepteddeliberately, and
authority_chain::a_backdated_window_is_accepted_at_issuepins that itstays accepted. Already covered, nothing added.
Dropped as redundant
a_grant_its_own_subject_wrote_does_not_verifywas written and then removed in 05aba97. Itreached
DTGCredentialError::NotSignedthrough the same two lines asan_unsigned_grant_does_not_verifydirectly above it — the depth check, then the absence ofa top-level
proof. Neither the document's provenance nor its subject is read before thaterror, so the two cases exercised identical code. The case that remains cross-references the
one that was already there.
Not a gate
a_grant_its_own_subject_wrote_satisfies_the_member_checkasserts that the constructorbuilds, so no revert makes it red. It is a characterization pin on the residual risk the
PoC's PART 1 showed, kept so that a later change cannot quietly start describing
new_member_vmc_foras verifying a grant. Demonstrated by removing thefound != membercheck from
new_member_vmc_for: the new case stayed green while the pre-existing gate wentred.
That is the answer to "is H13 gated": it is, by
a_grant_naming_someone_else_is_refusedfrom #28, not by anything here.
Red, then green
Both new depth cases run on a
std::thread::Builderthread with a 256 KiB stack, as therest of
tests/json_bounds.rsdoes, so a missing guard aborts the binary rather thanpassing quietly. Deep values are built programmatically — serde_json's parser caps at 128
levels, so a deep document cannot be parsed to make the case. Each revert below was
restored immediately, confirmed with
git diff origin/main -- src/lib.rsprinting nothing,and nothing was committed while a revert was in place.
answering_a_deep_grant_is_refused_without_exhausting_the_stackRevert:
check_json_depth(doc)?;removed fromdigest_multibase_json, which is whatnew_member_vmc_forreaches throughassemble_member_vmc.That is the PoC's
cargo-test-dos-output.txtsignature — stack overflow, SIGABRT — reachedthrough a constructor rather than through
serde_json::to_value. Restored:signing::verifying_a_deep_grant_is_refused_without_exhausting_the_stackRevert:
check_json_depth(grant)?;removed fromverify_grant_with_public_key. The grant issigned before the deep member is attached, which is load-bearing: without a proof the
verifier refuses the grant before it reaches the clone that strips
proof, and the casewould prove nothing.
Restored:
What cannot be gated
The PoC's abort was triggered by
serde_json::to_value(&credential)on a credential thecaller had built with a 50,000-level value — plain serialization, not a digest, a signature
or a verification.
MAX_JSON_DEPTHcannot cover that, and its own "What it cannot do"section says so: a
serde_json::Valueis dropped recursively too, so a caller alreadyholding such a value overflows when it goes out of scope whatever this library returns.
serde_json's parser limit is the real boundary, and
serde_json_still_limits_parsing_depthpins it. There is no way to assert the uncovered case in a test, because the only way to
exercise it is to overflow, which aborts the binary — which is exactly why the guarded
entry points are the ones worth gating, and why these cases run on a small stack.
Checks
cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings,cargo test --all-features(164 passed) andcargo test --no-default-features(148 passed)all clean.
Refs: SEC-4045