Improve untrusted MAST overspec warning - #3418
Conversation
76870fe to
87c0e82
Compare
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
Looks good, I do have a few questions though (see comments)
| let package = MastPackage::read_from_bytes_trusted(&bytes) | ||
| .map_err(|error| Report::msg(error.to_string()))?; | ||
| let package = | ||
| MastPackage::read_from_bytes(&bytes).map_err(|error| Report::msg(error.to_string()))?; |
There was a problem hiding this comment.
I think this should use read_from_bytes_trusted - implicitly the dependency graph is operating on trusted packages (they are referenced either explicitly by the user, assembled on-demand and cached, or are loaded from a user-provided registry).
This is exactly the sort of issue I was running into - some code might defensively say "I'm not sure if I should assume trust, so I'm going to use the safer choice", but this would result in errors getting logged. Now that those errors are warnings, it's marginally better, but it's still essentially out of the control of the caller to say "this package is trusted", and I think it's fair to assume packages are trusted here.
There was a problem hiding this comment.
Updated to use a narrower reader than read_from_bytes_trusted: it validates MAST and manifest data, but preserves trusted local debug sections.
| log::warn!( | ||
| "Package read ignored debug sections from an untrusted artifact; use Package::read_from_trusted for local cache/debug reads" | ||
| ); | ||
| log::warn!("Package read ignored debug sections from an untrusted artifact"); |
There was a problem hiding this comment.
It's unclear to me why we treat debug info like this? It should just be another custom section, just one that various built-in facilities recognize - in other words, what is special about the debug info section that warrants stripping it out?
There was a problem hiding this comment.
Package::read_from stays the untrusted reader. It validates MAST and drops debug sections because as of today we do not know how to verify debug info adversarially (what sort of resource exhaustion could it create in any of its downstream processing, not limited to deserialization?). Keeping it would make unverified debug data look trusted.
There was a problem hiding this comment.
That feels to me like a job for Package::debug_info, not Package::read_from - we're not deserializing the debug section here, and at this point it is no different than any other section.
The trust boundary here is really about the package metadata and the MAST - the parts it actually has to process when this is called. By conflating the two we're stripping out one of the most valuable pieces of the package silently.
355d636 to
fe1b216
Compare
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
I think we should stop stripping debug info when deserializing Package via read_from (i.e. the untrusted path), and instead punt that responsibility to Package::debug_info when we actually deserialize that data.
Currently, we're unintentionally stripping debug info in various places, silently, and it will be super confusing.
|
Related: #3425 same diagnostic, reported from the miden client CLI side |
Demote the overspecified untrusted MAST diagnostic to a warning and include the tracked caller location. Clarify package trusted reader docs and add the local design note with origin/next permalinks.
d2fcca5 to
6064ff4
Compare
3 participants
On top of #3460. Closes #3425.
This PR changes the untrusted MAST forest overspec log fromerror!towarn!, adds caller location to the warning, and aligns package trusted readers with their names.This PR hardens package deserialization across trust boundaries.
Untrusted package reads now validate the embedded MAST forest, decode package
DebugInfowithin fixed bounds, validate it against the package, and retain it only after that succeeds.Trusted package reads now mean what the name says. They trust the embedded MAST and manifest links, preserve package debug sections, and skip the cross-checks meant for hostile input.
The MAST warning is narrower : if an untrusted MAST forest includes wire hashes, we log
warn!with the caller location. The input is still accepted and validated by recomputing hashes. The warning is about wasted untrusted bytes, not corruption.Package::read_fromremains the untrusted reader for packages where debug sections are not needed. It validates the MAST and drops package debug sections.Package::read_fromandPackage::read_from_bytesare the untrusted readers. They validate MAST and package debug info before returning the package.Package::read_from_untrustedandPackage::read_from_bytes_untrustedare new. They validate the MAST and package debug sections, then keep debug info.No separate untrusted reader was added. The existing
Package::read_fromandPackage::read_from_byteskeep that role.Package::read_from_uncheckedandPackage::read_from_bytes_uncheckedremain the way to trust bytes completely.Package::read_from_uncheckedandPackage::read_from_bytes_uncheckedwere removed. UsePackage::read_from_trustedandPackage::read_from_bytes_trustedfor same-domain trusted package bytes.The DebugInfo path now rejects oversized payloads, oversized string and type tables, oversized strings, invalid references, bad spans, invalid layouts, and unsafe lookup shapes. Fuzz targets and regression seeds cover those hostile inputs.