Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ensure input's script type and path match the scriptPubKey #2144

Merged
merged 10 commits into from
Mar 17, 2022
Merged

Ensure input's script type and path match the scriptPubKey #2144

merged 10 commits into from
Mar 17, 2022

Conversation

matejcik
Copy link
Contributor

@matejcik matejcik commented Feb 25, 2022 •
edited
Loading

TODO:

  • if safety-checks are lowered, allow unverified EXTERNAL inputs
  • tests

@andrewkozlik andrewkozlik force-pushed the matejcik/scriptpubkey-check branch from 594b3d0 to abddb49 Compare March 8, 2022 09:01
matejcik
matejcik commented Mar 10, 2022
View reviewed changes
Copy link
Contributor Author

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

LGTM, but please add incompatible changelog entries.
I wrote some previously:

Trezor will refuse to sign UTXOs that do not match the provided derivation path (e.g., transactions belonging to a different wallet, or synthetic transaction inputs).

legacy/firmware/signing.c Outdated Show resolved Hide resolved
@andrewkozlik
Copy link
Contributor

please add incompatible changelog entries.

61186a1 f4388a1

@andrewkozlik
Copy link
Contributor

andrewkozlik commented Mar 15, 2022 •
edited
Loading

In 54ac830 f7aa680 I did a little heuristic around the TxWeight calculation, so that Trezor doesn't show the fee-over-threshold dialog when a lot of unverified external inputs are present.

In 86c6620 I replaced the operation in check_internal_input(), which is no longer needed, with a sanity check. For more details see code comment.

@andrewkozlik andrewkozlik marked this pull request as ready for review March 15, 2022 09:25
@andrewkozlik andrewkozlik requested a review from prusnak as a code owner March 15, 2022 09:25
@andrewkozlik
Copy link
Contributor

Closes https://github.com/satoshilabs/trezor-firmware/issues/123, see also https://github.com/satoshilabs/trezor-firmware/pull/128.

@matejcik
Copy link
Contributor Author

LGTM, please rebase and merge

@andrewkozlik andrewkozlik force-pushed the matejcik/scriptpubkey-check branch from a6a63d9 to 0a1bcc0 Compare March 17, 2022 08:09
@andrewkozlik andrewkozlik merged commit 605e128 into master Mar 17, 2022
@andrewkozlik andrewkozlik deleted the matejcik/scriptpubkey-check branch March 17, 2022 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewkozlik andrewkozlik andrewkozlik left review comments

@prusnak prusnak Awaiting requested review from prusnak

Assignees

@andrewkozlik andrewkozlik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matejcik @andrewkozlik