Update rust-bitcoin and add secp256k1 context randomization #802

TheBlueMatt · 2021-02-19T19:17:23Z

This updates our rust-bitcoin dependency and adds secp256k1 context randomization. Note that it relies on an open PR upstream in lightning-c-bindings and a separate open PR upstream for fuzzing. That's probably fine for fuzzing, but for bindings it kinda sucks. Hopefully we can get those merged soon.

TheBlueMatt · 2021-02-19T19:22:48Z

Also added a wasm32-wasi bindings build for good measure (since we need the update to do it).

codecov · 2021-02-19T19:58:07Z

Codecov Report

Merging #802 (ea48a5a) into main (e77b160) will decrease coverage by 0.01%.
The diff coverage is 96.00%.

@@            Coverage Diff             @@
##             main     #802      +/-   ##
==========================================
- Coverage   91.04%   91.02%   -0.02%     
==========================================
  Files          48       48              
  Lines       25480    25492      +12     
==========================================
+ Hits        23199    23205       +6     
- Misses       2281     2287       +6

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`90.90% <0.00%> (+0.29%)`	⬆️
lightning/src/chain/keysinterface.rs	`93.15% <90.00%> (-0.21%)`	⬇️
lightning/src/chain/channelmonitor.rs	`95.76% <100.00%> (+0.08%)`	⬆️
lightning/src/ln/channel.rs	`87.83% <100.00%> (+0.03%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.26% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/onchaintx.rs	`94.31% <100.00%> (+0.20%)`	⬆️
lightning/src/ln/onion_route_tests.rs	`96.85% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`83.49% <100.00%> (+0.33%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.97% <0.00%> (-0.16%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e77b160...ea48a5a. Read the comment docs.

valentinewallace

Thanks for taking care of this! Nice bite-sized commits + good to squeeze in the side channel attack mitigation.

Edit: fine to approve once CI is fixed

lightning/src/util/test_utils.rs

valentinewallace · 2021-02-19T20:51:10Z

lightning/src/ln/onion_route_tests.rs

@@ -219,7 +219,7 @@ impl msgs::ChannelUpdate {
 		use bitcoin::secp256k1::ffi::Signature as FFISignature;
 		use bitcoin::secp256k1::Signature;
 		msgs::ChannelUpdate {
-			signature: Signature::from(FFISignature::new()),
+			signature: Signature::from(unsafe { FFISignature::new() }),


OOC, do you know why secp256k1:;Signature was suddenly made unsafe?

I'm not 100% sure, but it does allow you to create an obviously-bogus Signature object which generally you're not supposed to be able to do (once its in a Signature its supposed to be a valid object, even if not a valid signature for a given message, I think).

genbindings.sh

fuzz broken

TheBlueMatt · 2021-02-19T22:36:00Z

Yea, sorry, I got too excited with this. I think I also need to pull in rust-bitcoin/bitcoin_hashes#111 for fuzzing and then re-redo the full_stack_target test.

TheBlueMatt · 2021-02-19T22:56:38Z

OK, should be resolve now, also address your comments.

c-bindings-gen/README.md

TheBlueMatt · 2021-02-22T17:00:23Z

Rebased.

jkczyz

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those? I'm not sure if I have suitable knowledge to do so on my own.

lightning/src/util/test_utils.rs

jkczyz · 2021-02-23T17:52:25Z

fuzz/src/full_stack.rs

-		// 0000000000000000000000000000000000000000000000000000000000000000 - our network key
+		// 0100000000000000000000000000000000000000000000000000000000000000 - our network key


To confirm my understanding, do these diffs extend from the fact that the use of get_secure_random_bytes to seed Secp256k1 causes an increment to a rand_bytes_child_index?

They extend from a few things (which is why there's so many of them). This particular one extends from the fact that secp256k1's fuzzing mode now requires the private key not be all-0s (in fact it enforces the regular non-fuzzing rules around private key validity). Other changes are required because, as a result of that, we now have sha256 only return hashes with one byte set to 1-0xff instead of 0-0xff as we did previously.

genbindings.sh

TheBlueMatt · 2021-02-24T04:15:25Z

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those?

Yea, to merge that probably makes sense.

TheBlueMatt · 2021-02-26T16:28:11Z

Relevant IRC discussion:

<BlueMatt> andytoshi: plz2conceptack https://github.com/rust-bitcoin/rust-secp256k1/pull/282
...
<andytoshi> BlueMatt: concept ack
<BlueMatt> ariard: I should probably listen in to those, but not sure I have much to add.
<BlueMatt> andytoshi: note there's a bitcoin_hashes companion which checks for 0 hashes and returns 1 instead
<andytoshi> nice, concept ack that too
<BlueMatt> ok, thanks
<BlueMatt> I'm gonna depend on those PRs in rust-lightning's fuzzing subcrate, fwiw
<BlueMatt> and also depend on https://github.com/rust-bitcoin/rust-secp256k1/pull/279 in our c bindings
<BlueMatt> (though *most* of our contexts are randomized, thats just for one group of them, which honestly I should probably just drop in the api)
<andytoshi> yeah concept ack that too
<BlueMatt> oh, no, I cant drop all of them, damn.
<andytoshi> i'm not so worried about the context randomization, it's a defense in depth against sidechannel attacks and i'd be hard-pressed to demonstrate a benefit
<andytoshi> not that you shouldn't do it ... but it's far from critical
<BlueMatt> yea....I'd need to audit, but it may very well be the only places we actually use the global contexts are verify tables anyway
<BlueMatt> oh, no, we do public key combines and private key multiplies and such which takes a signing context

TheBlueMatt · 2021-02-26T16:33:02Z

Squashed. I'd ideally like to take this for 0.0.13, unless someone has objections to the PR dependencies, which would be reasonable.

valentinewallace · 2021-02-26T19:53:55Z

fix build?

Note that rust-fuzz wrappers (including honggfuzz) already apply this for us.

TheBlueMatt · 2021-02-26T20:29:32Z

Oops, rebase introduced errors. I droped the bindings updates cause its gonna require some minor changes in the generator, though likely not too bad.

valentinewallace

LGTM!

This is useful when rebuilding the full_stack_target test vector

TheBlueMatt · 2021-02-27T04:51:11Z

Bumped to latest version of the rust-secp pull to fix (most of) the performance degredation.

TheBlueMatt · 2021-02-27T05:19:16Z

Note that, sadly, this still decreases the performance of, especially, chanmon_consistency, taking the CI fuzz runs from 18/91 minutes to 22/23 minutes for the fuzzing section.

jkczyz · 2021-02-28T05:12:16Z

I'm seeing this failure at ea48a5a:

$ cargo test
    Finished test [optimized + debuginfo] target(s) in 0.04s
     Running target/debug/deps/lightning_fuzz-2b6f767ab0ec4198

running 1 test
test full_stack::tests::test_no_existing_test_breakage ... FAILED

failures:

---- full_stack::tests::test_no_existing_test_breakage stdout ----
TRACE [lightning::ln::peer_handler : /Users/jkczyz/src/rust-lightning-review/lightning/src/ln/peer_handler.rs, 579] Got Err handling message, disconnecting peer because Bad MAC
thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(1)`', src/full_stack.rs:902:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    full_stack::tests::test_no_existing_test_breakage

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--lib'

TheBlueMatt · 2021-02-28T05:19:10Z

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.

jkczyz

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.

Yeah, that should have been obvious. :P

LGTM

TheBlueMatt changed the title ~~Update rust-bitcoin~~ Update rust-bitcoin and add secp256k1 context randomization Feb 19, 2021

TheBlueMatt force-pushed the 2021-01-update-rust-bitcoin branch 3 times, most recently from 5a166a1 to e81fab9 Compare February 19, 2021 19:41

TheBlueMatt force-pushed the 2021-01-update-rust-bitcoin branch 3 times, most recently from a5cb963 to 85dcb52 Compare February 19, 2021 21:12

valentinewallace previously approved these changes Feb 19, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-01-update-rust-bitcoin branch from 85dcb52 to 3dd72e5 Compare February 19, 2021 22:49

TheBlueMatt force-pushed the 2021-01-update-rust-bitcoin branch 2 times, most recently from 386ccf3 to 49718cf Compare February 19, 2021 23:11

valentinewallace reviewed Feb 20, 2021

View reviewed changes

c-bindings-gen/README.md Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-01-update-rust-bitcoin branch from 49718cf to a7863d4 Compare February 22, 2021 17:00

jkczyz reviewed Feb 23, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-01-update-rust-bitcoin branch from a7863d4 to 4e2bdf2 Compare February 24, 2021 04:15

TheBlueMatt force-pushed the 2021-01-update-rust-bitcoin branch 2 times, most recently from 1543730 to b328fcf Compare February 26, 2021 16:32

TheBlueMatt added this to the 0.0.13 milestone Feb 26, 2021

TheBlueMatt added 2 commits February 26, 2021 15:15

Update rust-bitcoin

a51d5ce

Apply --cfg=fuzzing when fuzzing, as required by new secp256k1

9b7609a

Note that rust-fuzz wrappers (including honggfuzz) already apply this for us.

TheBlueMatt force-pushed the 2021-01-update-rust-bitcoin branch from b328fcf to bd48726 Compare February 26, 2021 20:28

valentinewallace approved these changes Feb 26, 2021

View reviewed changes

TheBlueMatt mentioned this pull request Feb 26, 2021

Update bindings for new Listen interface #811

Merged

TheBlueMatt added 5 commits February 26, 2021 23:47

Use rust-secp256k1 PR 282 and bitcoin_hashes PR 111 in fuzzing

4594f6e

Use lowmemory mode for secp256k1 fuzzing, reducing memcpy time

33da665

Randomize secp contexts for marginally better sidechannel resistance

1521633

Log full signed remote commitment transaction hex in channel

c5c0abb

This is useful when rebuilding the full_stack_target test vector

Rebuild full_stack_target test vector for new secp256k1 fuzz req

ea48a5a

TheBlueMatt force-pushed the 2021-01-update-rust-bitcoin branch from bd48726 to ea48a5a Compare February 27, 2021 04:50

jkczyz approved these changes Feb 28, 2021

View reviewed changes

TheBlueMatt merged commit 9fba7c9 into lightningdevkit:main Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rust-bitcoin and add secp256k1 context randomization #802

Update rust-bitcoin and add secp256k1 context randomization #802

TheBlueMatt commented Feb 19, 2021 •

edited

Loading

TheBlueMatt commented Feb 19, 2021

codecov bot commented Feb 19, 2021 •

edited

Loading

valentinewallace left a comment •

edited

Loading

valentinewallace Feb 19, 2021

TheBlueMatt Feb 19, 2021

TheBlueMatt commented Feb 19, 2021

TheBlueMatt commented Feb 19, 2021

TheBlueMatt commented Feb 22, 2021

jkczyz left a comment

jkczyz Feb 23, 2021

TheBlueMatt Feb 24, 2021

TheBlueMatt commented Feb 24, 2021

TheBlueMatt commented Feb 26, 2021

TheBlueMatt commented Feb 26, 2021

valentinewallace commented Feb 26, 2021

TheBlueMatt commented Feb 26, 2021

valentinewallace left a comment

TheBlueMatt commented Feb 27, 2021

TheBlueMatt commented Feb 27, 2021

jkczyz commented Feb 28, 2021

TheBlueMatt commented Feb 28, 2021 •

edited

Loading

jkczyz left a comment

		// 0000000000000000000000000000000000000000000000000000000000000000 - our network key
		// 0100000000000000000000000000000000000000000000000000000000000000 - our network key

Update rust-bitcoin and add secp256k1 context randomization #802

Update rust-bitcoin and add secp256k1 context randomization #802

Conversation

TheBlueMatt commented Feb 19, 2021 • edited Loading

TheBlueMatt commented Feb 19, 2021

codecov bot commented Feb 19, 2021 • edited Loading

Codecov Report

valentinewallace left a comment • edited Loading

Choose a reason for hiding this comment

valentinewallace Feb 19, 2021

Choose a reason for hiding this comment

TheBlueMatt Feb 19, 2021

Choose a reason for hiding this comment

TheBlueMatt commented Feb 19, 2021

TheBlueMatt commented Feb 19, 2021

TheBlueMatt commented Feb 22, 2021

jkczyz left a comment

Choose a reason for hiding this comment

jkczyz Feb 23, 2021

Choose a reason for hiding this comment

TheBlueMatt Feb 24, 2021

Choose a reason for hiding this comment

TheBlueMatt commented Feb 24, 2021

TheBlueMatt commented Feb 26, 2021

TheBlueMatt commented Feb 26, 2021

valentinewallace commented Feb 26, 2021

TheBlueMatt commented Feb 26, 2021

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 27, 2021

TheBlueMatt commented Feb 27, 2021

jkczyz commented Feb 28, 2021

TheBlueMatt commented Feb 28, 2021 • edited Loading

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 19, 2021 •

edited

Loading

codecov bot commented Feb 19, 2021 •

edited

Loading

valentinewallace left a comment •

edited

Loading

TheBlueMatt commented Feb 28, 2021 •

edited

Loading