diff --git a/sdk/src/error.rs b/sdk/src/error.rs index 7f87cdd09..5cfdf4a01 100644 --- a/sdk/src/error.rs +++ b/sdk/src/error.rs @@ -262,6 +262,9 @@ pub enum Error { #[error("hash verification( {0} )")] HashMismatch(String), + #[error("cyclic ingredient found in path: {claim_label_path:?}")] + CyclicIngredients { claim_label_path: Vec }, + #[error("claim verification failure: {0}")] ClaimVerification(String), diff --git a/sdk/src/reader.rs b/sdk/src/reader.rs index cafe8d3e9..856f07e5e 100644 --- a/sdk/src/reader.rs +++ b/sdk/src/reader.rs @@ -17,7 +17,7 @@ #[cfg(feature = "file_io")] use std::fs::{read, File}; use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, io::{Read, Seek, Write}, }; @@ -876,8 +876,11 @@ impl Reader { ) -> Result> { let mut assertion_values = HashMap::new(); let mut stack: Vec<(String, Option)> = vec![(manifest_label.to_string(), None)]; + let mut seen = HashSet::new(); while let Some((current_label, parent_uri)) = stack.pop() { + seen.insert(current_label.clone()); + // If we're processing an ingredient, push its URI to the validation log if let Some(uri) = &parent_uri { validation_log.push_ingredient_uri(uri.clone()); @@ -931,11 +934,14 @@ impl Reader { // Add ingredients to stack for processing for ingredient in manifest.ingredients().iter() { if let Some(label) = ingredient.active_manifest() { - let ingredient_uri = crate::jumbf::labels::to_assertion_uri( - ¤t_label, - ingredient.label().unwrap_or("unknown"), - ); - stack.push((label.to_string(), Some(ingredient_uri))); + // REVIEW-NOTE: if we already saw this manifest, do we need to compute it again here? + if !seen.contains(label) { + let ingredient_uri = crate::jumbf::labels::to_assertion_uri( + ¤t_label, + ingredient.label().unwrap_or("unknown"), + ); + stack.push((label.to_string(), Some(ingredient_uri))); + } } } diff --git a/sdk/src/store.rs b/sdk/src/store.rs index 3f72dfe0e..9a5fca575 100644 --- a/sdk/src/store.rs +++ b/sdk/src/store.rs @@ -4020,16 +4020,41 @@ impl Store { recurse: bool, validation_log: &mut StatusTracker, ) -> Result<()> { + Self::get_claim_referenced_manifests_impl( + claim, + store, + svi, + recurse, + validation_log, + &mut Vec::new(), + ) + } + + fn get_claim_referenced_manifests_impl<'a>( + claim: &'a Claim, + store: &'a Store, + svi: &mut StoreValidationInfo<'a>, + recurse: bool, + validation_log: &mut StatusTracker, + claim_label_path: &mut Vec<&'a str>, + ) -> Result<()> { + let claim_label = claim.label(); + + // REVIEW-NOTE: added this so we don't compute the same ingredients more than once. + if svi.manifest_map.contains_key(claim_label) { + return Ok(()); + } + + claim_label_path.push(claim_label); + // add in current redactions if let Some(c_redactions) = claim.redactions() { svi.redactions .append(&mut c_redactions.clone().into_iter().collect::>()); } - let claim_label = claim.label().to_owned(); - // save the addressible claims for quicker lookup - svi.manifest_map.insert(claim_label.clone(), claim); + svi.manifest_map.insert(claim_label.to_owned(), claim); for i in claim.ingredient_assertions() { let ingredient_assertion = Ingredient::from_assertion(i.assertion())?; @@ -4044,20 +4069,39 @@ impl Store { let ingredient_label = Store::manifest_label_from_path(&c2pa_manifest.url()); if let Some(ingredient) = store.get_claim(&ingredient_label) { + if claim_label_path.contains(&ingredient.label()) { + return Err(log_item!( + jumbf::labels::to_assertion_uri(claim_label, &i.label()), + "ingredient cannot be cyclic", + "ingredient_checks" + ) + .validation_status(validation_status::ASSERTION_INGREDIENT_MALFORMED) + .failure_as_err( + validation_log, + Error::CyclicIngredients { + claim_label_path: claim_label_path + .iter() + .map(|&label| label.to_owned()) + .collect(), + }, + )); + } + // build mapping of ingredients and those claims that reference it svi.ingredient_references - .entry(ingredient_label) - .or_insert(HashSet::from_iter(vec![claim_label.clone()].into_iter())) - .insert(claim_label.clone()); + .entry(ingredient_label.clone()) + .or_insert(HashSet::from_iter(vec![claim_label.to_owned()].into_iter())) + .insert(claim_label.to_owned()); // recurse nested ingredients if recurse { - Store::get_claim_referenced_manifests( + Store::get_claim_referenced_manifests_impl( ingredient, store, svi, recurse, validation_log, + claim_label_path, )?; } } else { @@ -4076,6 +4120,8 @@ impl Store { } } + claim_label_path.pop(); + Ok(()) } diff --git a/sdk/tests/test_builder.rs b/sdk/tests/test_builder.rs index 4bfa0a588..ca1d2dcc6 100644 --- a/sdk/tests/test_builder.rs +++ b/sdk/tests/test_builder.rs @@ -11,11 +11,11 @@ // specific language governing permissions and limitations under // each license. -use std::io::{self, Cursor}; +use std::io::{self, Cursor, Seek}; use c2pa::{ - settings::Settings, validation_status, Builder, BuilderIntent, ManifestAssertionKind, Reader, - Result, ValidationState, + identity::validator::CawgValidator, settings::Settings, validation_status, Builder, + BuilderIntent, Error, ManifestAssertionKind, Reader, Result, ValidationState, }; mod common; @@ -79,6 +79,94 @@ fn test_builder_riff() -> Result<()> { Ok(()) } +// Constructs a C2PA asset that has an ingredient that references the main asset's active +// manifest as the ingredients active manifest. +// +// Source: https://github.com/contentauth/c2pa-rs/issues/1554 +#[test] +fn test_builder_cyclic_ingredient() -> Result<()> { + Settings::from_toml(include_str!("../tests/fixtures/test_settings.toml"))?; + + let mut source = Cursor::new(include_bytes!("fixtures/no_manifest.jpg")); + let format = "image/jpeg"; + + let mut ingredient = Cursor::new(Vec::new()); + + // Start by making a basic ingredient. + let mut builder = Builder::new(); + builder.set_intent(BuilderIntent::Edit); + builder.sign(&Settings::signer()?, format, &mut source, &mut ingredient)?; + + source.rewind()?; + ingredient.rewind()?; + + let mut dest = Cursor::new(Vec::new()); + + // Then create an asset with the basic ingredient. + let mut builder = Builder::new(); + builder.set_intent(BuilderIntent::Edit); + builder.add_ingredient_from_stream( + serde_json::json!({}).to_string(), + format, + &mut ingredient, + )?; + builder.sign(&Settings::signer()?, format, &mut source, &mut dest)?; + + dest.rewind()?; + ingredient.rewind()?; + + let active_manifest_uri = Reader::from_stream(format, &mut dest)? + .active_label() + .unwrap() + .to_owned(); + let ingredient_uri = Reader::from_stream(format, ingredient)? + .active_label() + .unwrap() + .to_owned(); + + // If they aren't the same number of bytes then we can't reliably substitute the URI. + assert_eq!(active_manifest_uri.len(), ingredient_uri.len()); + + // Replace the ingredient active manifest with the main active manifest. + let mut bytes = dest.into_inner(); + let old = ingredient_uri.as_bytes(); + let new = active_manifest_uri.as_bytes(); + + let mut i = 0; + while i + old.len() <= bytes.len() { + if &bytes[i..i + old.len()] == old { + bytes[i..i + old.len()].copy_from_slice(new); + i += old.len(); + } else { + i += 1; + } + } + + // Attempt to read the manifest with a cyclical ingredient. + let mut cyclic_ingredient = Cursor::new(bytes); + assert!(matches!( + Reader::from_stream(format, &mut cyclic_ingredient), + Err(Error::CyclicIngredients { .. }) + )); + + cyclic_ingredient.rewind()?; + + // Read the manifest without validating so we can test with post-validating the CAWG. + Settings::from_toml( + &toml::toml! { + [verify] + verify_after_reading = false + } + .to_string(), + )?; + let mut reader = Reader::from_stream(format, cyclic_ingredient)?; + // Ideally we'd use a sync path for this. There are limitations for tokio on WASM. + #[cfg(not(target_arch = "wasm32"))] + tokio::runtime::Runtime::new()?.block_on(reader.post_validate_async(&CawgValidator {}))?; + + Ok(()) +} + #[test] fn test_builder_sidecar_only() -> Result<()> { Settings::from_toml(include_str!("../tests/fixtures/test_settings.toml"))?;