Skip to content

Fix - Direct mapping CPI caller privilege escalation after ownership transfer#6709

Merged
Lichtso merged 3 commits intoanza-xyz:masterfrom
Lichtso:fix/direct_mapping_must_update_caller_when_ownership_changes
Jun 28, 2025
Merged

Fix - Direct mapping CPI caller privilege escalation after ownership transfer#6709
Lichtso merged 3 commits intoanza-xyz:masterfrom
Lichtso:fix/direct_mapping_must_update_caller_when_ownership_changes

Conversation

@Lichtso
Copy link
Copy Markdown

@Lichtso Lichtso commented Jun 23, 2025

Problem

In the original direct mapping implementation a CPI caller can transfer the ownership of an account to a callee before CPI, pass it as a read-only instruction account to a callee during CPI, and then after CPI returns, it is still possible for the caller to write to the account it no longer owns.

Credit for finding this goes to Felix Wilhelm and for testing it goes to Troy Sargent.

Summary of Changes

First demonstrates the issue by slightly adjusting TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_IN_CALLER. Then fixes the issue in update_callee_account() by setting must_update_caller if the owner changes on the CPI call edge.

@Lichtso Lichtso changed the title Fix - Direct mapping CPI caller previledge escalation after ownership transfer Fix - Direct mapping CPI caller privilege escalation after ownership transfer Jun 23, 2025
@Lichtso Lichtso requested a review from a team as a code owner June 24, 2025 08:04
@mergify
Copy link
Copy Markdown

mergify Bot commented Jun 24, 2025

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 24, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.3%. Comparing base (080febc) to head (8fe4954).
⚠️ Report is 1890 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #6709     +/-   ##
=========================================
- Coverage    83.3%    83.3%   -0.1%     
=========================================
  Files         852      852             
  Lines      377899   377901      +2     
=========================================
- Hits       315097   315089      -8     
- Misses      62802    62812     +10     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread programs/sbf/rust/invoke/src/lib.rs
LucasSte
LucasSte previously approved these changes Jun 25, 2025
@Lichtso Lichtso force-pushed the fix/direct_mapping_must_update_caller_when_ownership_changes branch from 87574cf to 8fe4954 Compare June 25, 2025 18:02
@Lichtso Lichtso requested a review from LucasSte June 27, 2025 19:19
@Lichtso Lichtso merged commit 517971f into anza-xyz:master Jun 28, 2025
28 checks passed
@Lichtso Lichtso deleted the fix/direct_mapping_must_update_caller_when_ownership_changes branch June 28, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants