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.

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

Fail channel in case of high-S remote signature reception #807

Merged
merged 1 commit into from
Nov 3, 2020

Conversation

ariard
Copy link

@ariard ariard commented Oct 20, 2020

See CVE-2020-26895 for context.

Note, the requirement is on every remote signature reception instead of a global note. We make this a reception requirement, which doesn't prevent implementation to harden by normalizing to low-S at transactions broadcasting.

@@ -388,7 +388,8 @@ The sender MUST set:
- `signature` to the valid signature, using its `funding_pubkey` for the initial commitment transaction, as defined in [BOLT #3](03-transactions.md#commitment-transaction).

The recipient:
- if `signature` is incorrect:
- if `signature` is incorrect OR non-compliant with LOW-S-standard rule<sup>[LOWS](https://github.com/bitcoin/bitcoin/pull/6769)</sup>:

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: extra new line

@rustyrussell
Copy link
Collaborator

Ack. Sometimes you just gotta repeat yourself.

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🐲

@ariard ariard force-pushed the 2020-10-reject-high-s branch from f4675b6 to c93d772 Compare October 26, 2020 20:35
@ariard
Copy link
Author

ariard commented Oct 26, 2020

Updated c93d772 with nit fix.

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK c93d772

@t-bast
Copy link
Collaborator

t-bast commented Nov 3, 2020

Merging according to spec meeting decision.

@t-bast t-bast merged commit 01f29bf into lightning:master Nov 3, 2020
SomberNight added a commit to SomberNight/electrum that referenced this pull request May 27, 2024
The low-S rule for ecdsa signatures is mandated by Bitcoin Core policy/standardness (though not by consensus). If we get signatures from untrusted sources, we should mandate they obey the policy rules. (e.g. from LN peers)

Note that we normalize the signatures in the sig format conversion methods (DER <-> (r,s) <-> sig64).

The BOLTs treat high-S signatures as invalid, and this changes our behaviour to that.
(previously we would silently normalize the S value)

see bitcoin/bitcoin#6769
see lightning/bolts#807
ahmedbodi pushed a commit to ahmedbodi/electrum-hemis that referenced this pull request Aug 28, 2024
The low-S rule for ecdsa signatures is mandated by Bitcoin Core policy/standardness (though not by consensus). If we get signatures from untrusted sources, we should mandate they obey the policy rules. (e.g. from LN peers)

Note that we normalize the signatures in the sig format conversion methods (DER <-> (r,s) <-> sig64).

The BOLTs treat high-S signatures as invalid, and this changes our behaviour to that.
(previously we would silently normalize the S value)

see bitcoin/bitcoin#6769
see lightning/bolts#807
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants