fix: Limit the maximum number of assertions allowed for C2PA Manifest#1951
fix: Limit the maximum number of assertions allowed for C2PA Manifest#1951
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
8932506 to
8ded3b2
Compare
tmathern
left a comment
There was a problem hiding this comment.
Some APIs used are deprecated - local thread settings were replaced by settings on context. So some checks need to be weaved through differently.
| /// of assertions. Calls to [`Builder::add_assertion`] or [`Builder::add_assertion_json`] | ||
| /// that would exceed this limit return [`Error::TooManyAssertions`]. | ||
| /// | ||
| /// The default value is 50. |
| S: Into<String>, | ||
| T: Serialize, | ||
| { | ||
| let max_assertions = self.context.settings().builder.max_assertions; |
There was a problem hiding this comment.
Optional: If doing twice exactly the same check, maybe it could be refactored?
|
|
||
| #[test] | ||
| fn test_add_assertion_limit() { | ||
| use crate::settings::Settings; |
There was a problem hiding this comment.
I prefer all exports on top at the test suite
There was a problem hiding this comment.
or at the top of the test module in this case
| assert!(matches!(err, Error::TooManyAssertions { max: 50 })); | ||
|
|
||
| // Verify the limit is configurable: set max_assertions=2 via settings. | ||
| let settings = Settings { |
There was a problem hiding this comment.
This should be another test, test_add_assertion_limit_is_configurable
| ) -> Result<C2PAAssertion> { | ||
| // Enforce the per-manifest assertion limit during signing to prevent | ||
| // resource exhaustion regardless of how the claim is constructed. | ||
| let max_assertions = get_thread_local_settings().builder.max_assertions; |
There was a problem hiding this comment.
I get the reason for this is that there is no context here or whatsoever to read the setting. But thread_local settings are another settings API that is deprecated. This likely needs to be weaved through differently.
There was a problem hiding this comment.
If we are using a setting it should only be one from the context. Perhaps the answer is we just set a large fixed constant for this. If we really need to configure it in claim, then we may want to consider adding a way to set a max assertions value in claim from the current settings. But I think a constant may be fine. The other approach is to add context or settings to Claim but that feels like overkill for this.
|
|
||
| // Reject manifests that embed more assertions than the configured limit to | ||
| // prevent unbounded memory and CPU consumption on untrusted input. | ||
| let max_assertions = get_thread_local_settings().verify.max_assertions; |
There was a problem hiding this comment.
I get the reason for this is that there is no context here or whatsoever to read the setting. But thread_local settings are another settings API that is deprecated. This likely needs to be weaved through differently.
|
|
||
| #[test] | ||
| fn test_add_assertion_limit() { | ||
| use crate::settings::Settings; |
There was a problem hiding this comment.
or at the top of the test module in this case
| ) -> Result<C2PAAssertion> { | ||
| // Enforce the per-manifest assertion limit during signing to prevent | ||
| // resource exhaustion regardless of how the claim is constructed. | ||
| let max_assertions = get_thread_local_settings().builder.max_assertions; |
There was a problem hiding this comment.
If we are using a setting it should only be one from the context. Perhaps the answer is we just set a large fixed constant for this. If we really need to configure it in claim, then we may want to consider adding a way to set a max assertions value in claim from the current settings. But I think a constant may be fine. The other approach is to add context or settings to Claim but that feels like overkill for this.
Changes in this pull request
Security Fix: Unbounded Assertion Count Allows Resource Exhaustion
Issue
No limit exists on the number of assertions that can be added to a manifest across three distinct code paths:
Builder::add_assertion/add_assertion_json(builder.rs) — the public API for constructing manifests programmatically.Claim::add_assertion(claim.rs) — the lower-level API callable directly, bypassingBuilder.Store::from_jumbf_impl(store.rs) — the parsing path that processes assertions from untrusted, existing C2PA files.An attacker can exploit paths 1 and 2 by constructing a manifest with an arbitrarily large number of assertions, and path 3 by supplying a crafted C2PA file with excessive assertions. All three cause uncontrolled memory and CPU consumption — a DoS vector for any application that creates or reads C2PA content.
Fix
New error variant (
sdk/src/error.rs)Returned by all three enforcement points when the configured limit is exceeded.
New settings fields
BuilderSettings.max_assertions: usize(sdk/src/settings/builder.rs) — default50. Controls assertion creation viaBuilderandClaim.Verify.max_assertions: usize(sdk/src/settings/mod.rs) — default50. Controls assertion parsing from untrusted files. Kept separate frombuilder.max_assertionsso operators can tune creation and parsing limits independently.Limit enforcement
sdk/src/builder.rsadd_assertion,add_assertion_jsoncontext.settings().builder.max_assertionssdk/src/claim.rsadd_assertion_implget_thread_local_settings().builder.max_assertionssdk/src/store.rsfrom_jumbf_impl, before the assertion parsing loopget_thread_local_settings().verify.max_assertionsEach point returns
Err(TooManyAssertions { max })before processing begins when the count meets or exceeds the limit.Tests
builder.rs—test_add_assertion_limitTooManyAssertions { max: 50 }.max_assertions: 2via settings; verifies the 3rd is rejected.claim.rs—test_add_assertion_default_limit_in_claimTooManyAssertions { max: 50 }.claim.rs—test_add_assertion_limit_in_claimbuilder.max_assertionsto 2 via thread-local settings.TooManyAssertions { max: 2 }.store.rs—test_verify_default_assertion_limit_in_storeverify.max_assertions(50) untouched.TooManyAssertions { max: 50 }.store.rs—test_verify_assertion_limit_in_storeverify.max_assertionsto 1.TooManyAssertions { max: 1 }.Checklist
TO DOitems (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.