Skip to content

Security Review 1.16.0#760

Merged
apaillier-ledger merged 16 commits into
developfrom
security-review-1-16-0
Apr 8, 2025
Merged

Security Review 1.16.0#760
apaillier-ledger merged 16 commits into
developfrom
security-review-1-16-0

Conversation

@bboilot-ledger
Copy link
Copy Markdown
Contributor

@bboilot-ledger bboilot-ledger commented Mar 26, 2025

Description

This PR is used to review the 1.16.0 release of the Ethereum app.
The target branch will be set to develop once the review is finished and the fixes are pushed on the branch. It will be kept as draft to avoid any unwanted merge.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)
  • Tests
  • Documentation
  • Other (for changes that might not fit in any category)

Breaking changes

Please complete this section if any breaking changes have been made, otherwise delete it.

Additional comments

Please post additional comments in this section if you have them, otherwise delete it.

bboilot-ledger
bboilot-ledger commented Mar 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@bboilot-ledger bboilot-ledger left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@apaillier-ledger @cedelavergne-ledger Hi 👋, here is a first batch of comments on the tx simulation feature. I will start a fuzzing campaign for tonight.

Comment thread src_features/provide_tx_simulation/cmd_get_tx_simulation.c Outdated
Comment thread src_features/provide_tx_simulation/cmd_get_tx_simulation.c Outdated
Comment thread src_features/provide_tx_simulation/cmd_get_tx_simulation.c Outdated
Comment thread src_features/provide_tx_simulation/cmd_get_tx_simulation.c Outdated
Comment thread src_features/provide_tx_simulation/cmd_get_tx_simulation.c
Comment thread src_features/provide_tx_simulation/cmd_get_tx_simulation.c
bboilot-ledger
bboilot-ledger commented Mar 28, 2025
View reviewed changes
Comment thread src_features/provide_network_info/network_info.c Outdated
Comment thread src_features/provide_tx_simulation/cmd_get_tx_simulation.c
Comment thread src_features/provide_network_info/network_info.c Outdated
Comment thread src_features/generic_tx_parser/calldata.c
bboilot-ledger
bboilot-ledger commented Mar 31, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@bboilot-ledger bboilot-ledger left a comment

Choose a reason for hiding this comment

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

Few comments regarding the current allocator. Dirty fixes can be enough since we're planning on switching to a real allocator in the next release.
Would recommend taking a look at https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html :)

Comment thread src/mem.c Outdated
Comment thread src/mem.c
Comment thread src/mem.c Outdated
@bboilot-ledger
Copy link
Copy Markdown
Contributor Author

bboilot-ledger commented Mar 31, 2025

In add_to_field_table, strlen must be computed after the NULL check. (can't add a review on that since its not part of the modifications).

@bboilot-ledger bboilot-ledger force-pushed the security-review-1-16-0 branch from b0c2c79 to e4790d8 Compare March 31, 2025 14:45
@bboilot-ledger
Copy link
Copy Markdown
Contributor Author

bboilot-ledger commented Mar 31, 2025

Fuzzers should pass once the review's recommendations are implemented.

bboilot-ledger
bboilot-ledger commented Apr 1, 2025
View reviewed changes
Comment thread tests/ragger/test_tx_simulation.py
Comment thread tests/ragger/test_tx_simulation.py
Comment thread src_features/provide_tx_simulation/cmd_get_tx_simulation.c Outdated
@cedelavergne-ledger cedelavergne-ledger mentioned this pull request Apr 7, 2025
Closed
1 task
@apaillier-ledger apaillier-ledger force-pushed the security-review-1-16-0 branch from cf6853d to de46712 Compare April 7, 2025 16:07
@apaillier-ledger
Copy link
Copy Markdown
Contributor

apaillier-ledger commented Apr 7, 2025

In add_to_field_table, strlen must be computed after the NULL check. (can't add a review on that since its not part of the modifications).

@bboilot-ledger
Done in 5996919

@bboilot-ledger bboilot-ledger changed the base branch from master to develop April 8, 2025 13:25
@bboilot-ledger bboilot-ledger marked this pull request as ready for review April 8, 2025 13:28
@bboilot-ledger bboilot-ledger force-pushed the security-review-1-16-0 branch from ab4e99c to 1ff7182 Compare April 8, 2025 13:42
@apaillier-ledger apaillier-ledger merged commit dbba2fb into develop Apr 8, 2025
201 checks passed
@apaillier-ledger apaillier-ledger deleted the security-review-1-16-0 branch April 8, 2025 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apaillier-ledger apaillier-ledger apaillier-ledger approved these changes

@cedelavergne-ledger cedelavergne-ledger Awaiting requested review from cedelavergne-ledger

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bboilot-ledger @apaillier-ledger @cedelavergne-ledger