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

fix: check y_parity value #1107

Merged
merged 1 commit into from
Feb 12, 2025
+28 −0
Merged

fix: check y_parity value #1107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/ethereum/arrow_glacier/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,14 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
Comment on lines +244 to +245
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there cases where we'd want secp256k1_recover to accept y_parity not in (0, 1)?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's a boolean. It gets encoded in strange ways in LegacyTransaction, but that is weirdness inherited from Bitcoin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding, no
Valid recovery id values are in (0, 1, 2, 3) (see coincurve) but ECDSA signatures are considered valid by the yellow paper only when v is in (0, 1).
image

This is confirmed by the fact that even for erecover, the input of secp256k1_recover will be 0 or 1 see ecrecover in exec spec and yellow paper

Capture d’écran 2025-02-04 à 16 46 40

Example with go-ethereum ValidateSignatureValues
https://github.com/ethereum/go-ethereum/blob/e6f3ce7b168b8f346de621a8f60d2fa57c2ebfb0/crypto/crypto.go#L274

and Sign function
https://github.com/ethereum/go-ethereum/blob/e6f3ce7b168b8f346de621a8f60d2fa57c2ebfb0/crypto/secp256k1/secp256.go#L68

Copy link
Contributor

Choose a reason for hiding this comment

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

So should we move the check to secp256k1_recover?

Copy link
Contributor Author

@obatirou obatirou Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is possible in this context if not mistaken but I think it would introduce a business need in secp256k1_recover which goal should only be to recover the public key from a given valid signature imo.
Shouldn't the validation of the signature values happen before ?
The checks for r and s range are already outside the function

execution-specs/src/ethereum/cancun/transactions.py

Lines 260 to 265 in 9923823

r, s = tx.r, tx.s
if U256(0) >= r or r >= SECP256K1N:
raise InvalidSignatureError("bad r")
if U256(0) >= s or s > SECP256K1N // U256(2):
raise InvalidSignatureError("bad s")
and

execution-specs/src/ethereum/cancun/vm/precompiled_contracts/ecrecover.py

Lines 47 to 52 in 9923823

if v != U256(27) and v != U256(28):
return
if U256(0) >= r or r >= SECP256K1N:
return
if U256(0) >= s or s >= SECP256K1N:
return

Let me know what you think. Open to make the changes if you prefer of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can certainly see the argument for keeping secp256k1_recover pure. That said, the s validation is not uniform across forks, so it cannot be included in secp256k1_recover directly. See

execution-specs/src/ethereum/frontier/transactions.py

Lines 134 to 135 in 9923823

if U256(0) >= s or s >= SECP256K1N:
raise InvalidSignatureError("bad s")

vs.

execution-specs/src/ethereum/cancun/transactions.py

Lines 263 to 264 in 9923823

if U256(0) >= s or s > SECP256K1N // U256(2):
raise InvalidSignatureError("bad s")

I suppose I'm happy either way!

Copy link
Contributor Author

@obatirou obatirou Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the concrete example!
I think it conforts my opinion that if secp256k1_recover cannot fully validate the signature values it should be kept pure. Else there could be confusion on where the validation of signature values is done.
So the PR should stay as is if that is ok for you. Let me know if you prefer any changes.

public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
Expand Down
2 changes: 2 additions & 0 deletions src/ethereum/berlin/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
Expand Down
6 changes: 6 additions & 0 deletions src/ethereum/cancun/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,14 +280,20 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
elif isinstance(tx, BlobTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_4844(tx)
)
Expand Down
4 changes: 4 additions & 0 deletions src/ethereum/gray_glacier/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,14 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
Expand Down
4 changes: 4 additions & 0 deletions src/ethereum/london/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,14 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
Expand Down
4 changes: 4 additions & 0 deletions src/ethereum/paris/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,14 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
Expand Down
4 changes: 4 additions & 0 deletions src/ethereum/shanghai/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,10 +248,14 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
Expand Down
Loading