fix(assembly): validate procedure roots during library deserialization#2933
Conversation
249710f to
9ff84ee
Compare
|
@amathxbt Please cease spam comments. |
619f63f to
8d82445
Compare
|
@giwaov Can you rebase this and resolve the CHANGELOG conflict? I think we can get this merged once that is done |
Library::read_from accepts procedure exports whose MastNodeId is not a procedure root in the underlying MAST forest. This means a malicious library can export uncallable procedures, violating the invariant enforced by Library::new(). Add a validation loop to the Deserializable implementation for Library that checks each procedure export has a corresponding procedure root in the MastForest, matching the existing check in Library::new(). Add a regression test that constructs a tampered library with an export pointing to a non-root node and verifies deserialization rejects it. Closes 0xMiden#2831
8d82445 to
1e97ee9
Compare
|
Automated check (CONTRIBUTING.md) Findings:
Recommendations:
Next steps:
|
|
Rebased onto the latest I also reran the regression test locally:
That passes now, so this should be ready for another look. |
| exports.insert(path, export); | ||
| } | ||
|
|
||
| for export in exports.values() { |
There was a problem hiding this comment.
I wonder if this should call Library::new(mast_forest, exports) instead of re-checking roots here. The last bug came from deserialization drifting away from the constructor path, and keeping the invariant and digest logic in one place would make the next validation change harder to miss.
There was a problem hiding this comment.
Updated on the branch now. Library::read_from delegates to Self::new(mast_forest, exports) so the procedure-root invariant and digest computation stay on the constructor path instead of drifting here again.
I also re-ran cargo test -p miden-assembly library_deserialization_rejects_non_root_export locally.
There was a problem hiding this comment.
Haha, fair enough 😄 Thanks again for the review and for merging this one.
That makes 10 merged PRs on my side now, which I’m really happy about and would love to do more, I also wanted to ask: when Miden eventually goes mainnet, is there likely to be any kind of recognition or reward for contributors, or is it still too early to say? Thank you sir.
Description
This PR fixes an issue where \Library::read_from\ accepts procedure exports whose \MastNodeId\ is not a procedure root in the underlying MAST forest. This allows crafting a library with uncallable procedure exports, violating the invariant enforced by \Library::new().
This was identified as finding 19 in the audit report.
Changes
Fix
Test
Testing
Closes #2831