Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Fix CPI duplicate account privilege escalation#22752

Merged
Lichtso merged 2 commits intosolana-labs:masterfrom
Lichtso:fix/#22107
Jan 27, 2022
Merged

Fix CPI duplicate account privilege escalation#22752
Lichtso merged 2 commits intosolana-labs:masterfrom
Lichtso:fix/#22107

Conversation

@Lichtso
Copy link
Copy Markdown
Contributor

@Lichtso Lichtso commented Jan 26, 2022

Problem

#22107 introduced a privilege escalation bug:
If a CPI uses duplicate Pubkeys only the privileges of the first were verified.

Summary of Changes

  • Moves CPI privilege verification out of deduplication loop.
  • Modifies TEST_PRIVILEGE_ESCALATION_SIGNER and TEST_PRIVILEGE_ESCALATION_WRITABLE to cover duplicate entries as well.

Fixes #

@Lichtso Lichtso requested review from jstarry and ryoqun January 26, 2022 10:36
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 26, 2022

Codecov Report

Merging #22752 (7a3d23d) into master (071e970) will increase coverage by 0.0%.
The diff coverage is 53.3%.

@@           Coverage Diff           @@
##           master   #22752   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         558      558           
  Lines      149899   149901    +2     
=======================================
+ Hits       122189   122224   +35     
+ Misses      27710    27677   -33     

Copy link
Copy Markdown
Contributor

@jstarry jstarry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good, would be nice to have separate tests for both original and dupe account privilege escalation though for future protection

ryoqun
ryoqun previously approved these changes Jan 27, 2022
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm (i second with @jstarry 's comment about separate tests; also backporting only those new tests would be a bonus; i mean it seems that our test suite lacked this duplicate account privilege escalation case in the first case. and that allowed this bug slipped in, right?).

@mergify mergify Bot dismissed ryoqun’s stale review January 27, 2022 16:18

Pull request has been modified.

@Lichtso Lichtso merged commit a71f05f into solana-labs:master Jan 27, 2022
@Lichtso Lichtso deleted the fix/#22107 branch January 27, 2022 23:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants