-
Notifications
You must be signed in to change notification settings - Fork 234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: fix msg_sender direct call exploit #7404
Conversation
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 8 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
AVM SimulationTime to simulate various public functions in the AVM.
Public DB AccessTime to access various public DBs.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
Can you please take a look at |
@@ -111,7 +111,7 @@ describe('Private Execution test suite', () => { | |||
const runSimulator = ({ | |||
artifact, | |||
args = [], | |||
msgSender = AztecAddress.ZERO, | |||
msgSender = AztecAddress.fromField(new Fr(Fr.MODULUS - 1n)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: This change isn't actually required for the tests to pass (this test doesn't reach the kernels), it's just there to keep in line with Fr.MAX
as the new default msg_sender
value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!
@@ -34,7 +34,8 @@ contract AppSubscription { | |||
|
|||
#[aztec(private)] | |||
fn entrypoint(payload: DAppPayload, user_address: AztecAddress) { | |||
assert(context.msg_sender().to_field() == 0); | |||
// Default msg_sender for entrypoints is now Fr.max_value rather than 0 addr (see #7190 & #7404) | |||
assert(context.msg_sender().to_field() == 0 - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be nice to create a constant so that it's easier to find where MAX_FIELD is used!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! It's called MAX_FIELD_VALUE
across the repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.46.7</summary> ## [0.46.7](aztec-package-v0.46.6...aztec-package-v0.46.7) (2024-07-16) ### Features * Devnet updates ([#7421](#7421)) ([103f099](103f099)) ### Bug Fixes * Cli l1-chain-id option ([#7490](#7490)) ([307bc57](307bc57)) ### Miscellaneous * Turn on elaborator ([#7451](#7451)) ([0599500](0599500)) </details> <details><summary>barretenberg.js: 0.46.7</summary> ## [0.46.7](barretenberg.js-v0.46.6...barretenberg.js-v0.46.7) (2024-07-16) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.46.7</summary> ## [0.46.7](aztec-packages-v0.46.6...aztec-packages-v0.46.7) (2024-07-16) ### Features * Add unconstrained context to txe ([#7448](#7448)) ([699fb79](699fb79)) * Add unconstrained getters to sharedmutable ([#7429](#7429)) ([c0ff566](c0ff566)) * Devnet updates ([#7421](#7421)) ([103f099](103f099)) * Point::fromXandSign(...) ([#7455](#7455)) ([225c6f6](225c6f6)) ### Bug Fixes * **avm:** Update generated verifier ([#7492](#7492)) ([f1216a7](f1216a7)) * Cli l1-chain-id option ([#7490](#7490)) ([307bc57](307bc57)) * Don't pass secrets to earthly-ci 'publish docs' command ([#7481](#7481)) ([a3f6feb](a3f6feb)) * Fix msg_sender direct call exploit ([#7404](#7404)) ([1dcae45](1dcae45)) * Missing NoteSelector from JSON RPC proxies ([#7493](#7493)) ([b209fad](b209fad)) * **pxe:** Best effort noir call stack generation ([#7336](#7336)) ([0c7459b](0c7459b)) * Validate gas used ([#7459](#7459)) ([6dc7598](6dc7598)) ### Miscellaneous * **avm:** More stats and codegen cleanup ([#7475](#7475)) ([1a6c7f2](1a6c7f2)) * Checking compute_encrypted_note_log against TS impl ([#7491](#7491)) ([1e8a597](1e8a597)) * Included subrelation witness degrees in the relations relevant to zk-sumcheck ([#7479](#7479)) ([457a115](457a115)) * Replace relative paths to noir-protocol-circuits ([71960d4](71960d4)) * Turn on elaborator ([#7451](#7451)) ([0599500](0599500)) </details> <details><summary>barretenberg: 0.46.7</summary> ## [0.46.7](barretenberg-v0.46.6...barretenberg-v0.46.7) (2024-07-16) ### Features * Point::fromXandSign(...) ([#7455](#7455)) ([225c6f6](225c6f6)) ### Bug Fixes * **avm:** Update generated verifier ([#7492](#7492)) ([f1216a7](f1216a7)) ### Miscellaneous * **avm:** More stats and codegen cleanup ([#7475](#7475)) ([1a6c7f2](1a6c7f2)) * Included subrelation witness degrees in the relations relevant to zk-sumcheck ([#7479](#7479)) ([457a115](457a115)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.46.7</summary> ## [0.46.7](AztecProtocol/aztec-packages@aztec-package-v0.46.6...aztec-package-v0.46.7) (2024-07-16) ### Features * Devnet updates ([#7421](AztecProtocol/aztec-packages#7421)) ([103f099](AztecProtocol/aztec-packages@103f099)) ### Bug Fixes * Cli l1-chain-id option ([#7490](AztecProtocol/aztec-packages#7490)) ([307bc57](AztecProtocol/aztec-packages@307bc57)) ### Miscellaneous * Turn on elaborator ([#7451](AztecProtocol/aztec-packages#7451)) ([0599500](AztecProtocol/aztec-packages@0599500)) </details> <details><summary>barretenberg.js: 0.46.7</summary> ## [0.46.7](AztecProtocol/aztec-packages@barretenberg.js-v0.46.6...barretenberg.js-v0.46.7) (2024-07-16) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.46.7</summary> ## [0.46.7](AztecProtocol/aztec-packages@aztec-packages-v0.46.6...aztec-packages-v0.46.7) (2024-07-16) ### Features * Add unconstrained context to txe ([#7448](AztecProtocol/aztec-packages#7448)) ([699fb79](AztecProtocol/aztec-packages@699fb79)) * Add unconstrained getters to sharedmutable ([#7429](AztecProtocol/aztec-packages#7429)) ([c0ff566](AztecProtocol/aztec-packages@c0ff566)) * Devnet updates ([#7421](AztecProtocol/aztec-packages#7421)) ([103f099](AztecProtocol/aztec-packages@103f099)) * Point::fromXandSign(...) ([#7455](AztecProtocol/aztec-packages#7455)) ([225c6f6](AztecProtocol/aztec-packages@225c6f6)) ### Bug Fixes * **avm:** Update generated verifier ([#7492](AztecProtocol/aztec-packages#7492)) ([f1216a7](AztecProtocol/aztec-packages@f1216a7)) * Cli l1-chain-id option ([#7490](AztecProtocol/aztec-packages#7490)) ([307bc57](AztecProtocol/aztec-packages@307bc57)) * Don't pass secrets to earthly-ci 'publish docs' command ([#7481](AztecProtocol/aztec-packages#7481)) ([a3f6feb](AztecProtocol/aztec-packages@a3f6feb)) * Fix msg_sender direct call exploit ([#7404](AztecProtocol/aztec-packages#7404)) ([1dcae45](AztecProtocol/aztec-packages@1dcae45)) * Missing NoteSelector from JSON RPC proxies ([#7493](AztecProtocol/aztec-packages#7493)) ([b209fad](AztecProtocol/aztec-packages@b209fad)) * **pxe:** Best effort noir call stack generation ([#7336](AztecProtocol/aztec-packages#7336)) ([0c7459b](AztecProtocol/aztec-packages@0c7459b)) * Validate gas used ([#7459](AztecProtocol/aztec-packages#7459)) ([6dc7598](AztecProtocol/aztec-packages@6dc7598)) ### Miscellaneous * **avm:** More stats and codegen cleanup ([#7475](AztecProtocol/aztec-packages#7475)) ([1a6c7f2](AztecProtocol/aztec-packages@1a6c7f2)) * Checking compute_encrypted_note_log against TS impl ([#7491](AztecProtocol/aztec-packages#7491)) ([1e8a597](AztecProtocol/aztec-packages@1e8a597)) * Included subrelation witness degrees in the relations relevant to zk-sumcheck ([#7479](AztecProtocol/aztec-packages#7479)) ([457a115](AztecProtocol/aztec-packages@457a115)) * Replace relative paths to noir-protocol-circuits ([71960d4](AztecProtocol/aztec-packages@71960d4)) * Turn on elaborator ([#7451](AztecProtocol/aztec-packages#7451)) ([0599500](AztecProtocol/aztec-packages@0599500)) </details> <details><summary>barretenberg: 0.46.7</summary> ## [0.46.7](AztecProtocol/aztec-packages@barretenberg-v0.46.6...barretenberg-v0.46.7) (2024-07-16) ### Features * Point::fromXandSign(...) ([#7455](AztecProtocol/aztec-packages#7455)) ([225c6f6](AztecProtocol/aztec-packages@225c6f6)) ### Bug Fixes * **avm:** Update generated verifier ([#7492](AztecProtocol/aztec-packages#7492)) ([f1216a7](AztecProtocol/aztec-packages@f1216a7)) ### Miscellaneous * **avm:** More stats and codegen cleanup ([#7475](AztecProtocol/aztec-packages#7475)) ([1a6c7f2](AztecProtocol/aztec-packages@1a6c7f2)) * Included subrelation witness degrees in the relations relevant to zk-sumcheck ([#7479](AztecProtocol/aztec-packages#7479)) ([457a115](AztecProtocol/aztec-packages@457a115)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Fixes exploit as first written here #7190. Essentially, a user calling a contract directly (bypassing the account contract) could impersonate any chosen address. This PR adds a check in the private init kernel to prevent setting the
msg_sender
. Commits are:token_contract
test - apologies for the format fails (taken exactly from Lasse's PR test: add test to showcase kernel exploit #7190, slight change to allow for direct call)token_contract
means exploit is fixed)token_contract