More bolt11 fixes #6776
More bolt11 fixes #6776
Conversation
rustyrussell
force-pushed
the
more-bolt11-fixes
branch
from
95a34d6 to
8a6bc6a
Compare
There was a problem hiding this comment.
I have a comment for 88ce922
This is in some sense reverting what we did in 5f6642a
The assert is in here because the string MUST be always valid because all the RPC should convert the string to a canonical one.
The bug is in to_canonical_invstr
https://github.com/ElementsProject/lightning/blob/master/common/bolt11.h#L128C1-L132C74
|
Rather than merging a second fuzz target for bolt11 decoding, I think we should extend @dergoegge's #6750. That target has the benefit of generating valid bech32 100% of the time, and I have some local changes that allow it to find all of these bugs. I can submit a couple PRs doing this in the next few days. |
|
I will apply this now, happy for later refinement. |
rustyrussell
force-pushed
the
more-bolt11-fixes
branch
from
8a6bc6a to
b70fe2a
Compare
|
Rebased with fix for my misunderstanding in decode_n (thanks, valgrind!). Really, bolt11 parsing is nasty :( |
rustyrussell
force-pushed
the
more-bolt11-fixes
branch
from
b70fe2a to
dfcf81d
Compare
|
Had to turn the bech32 csum reduction into a runtime flag, since it broke the bech32 fuzzing! |
[ Turned bech32 csum disable into a runtime flag --RR ]
An error is preferable to an assertion failure. Before this fix, the entire node crashes if there's an extra "lightning:" prefix: $ lightning-cli pay "lightning:lightning:" Node log: pay: common/bolt11.c:718: bolt11_decode_nosig: Assertion `!has_lightning_prefix(str)' failed. pay: FATAL SIGNAL 6 ... INFO plugin-pay: Killing plugin: exited during normal operation **BROKEN** plugin-pay: Plugin marked as important, shutting down lightningd [ I simply removed the check, so it fails on a bad bech32 string as expected --RR ]
Invalid recovery IDs cause secp256k1_ecdsa_recoverable_signature_parse_compact to abort, which crashes the entire node. We should return an error instead. Detected by libFuzzer: [libsecp256k1] illegal argument: recid >= 0 && recid <= 3
Otherwise, if pull_all fails, we attempt to create a script from NULL, causing a UBSan report: bitcoin/script.c:29:28: runtime error: null pointer passed as argument 2, which is declared to never be null
If both databits and *data_len are 0, pull_uint returns unitialized stack memory in *val. Detected by valgrind and UBSan. valgrind: ==225078== Use of uninitialised value of size 8 ==225078== __sanitizer_cov_trace_cmp8 ==225078== decode_c (bolt11.c:294) ==225078== bolt11_decode_nosig (bolt11.c:881) ==225078== bolt11_decode (bolt11.c:945) UBSan: common/bolt11.c:79:29: runtime error: shift exponent 64 is too large for 64-bit type 'uint64_t'
rustyrussell
force-pushed
the
more-bolt11-fixes
branch
from
dfcf81d to
71a3cc1
Compare
| @@ -126,6 +126,8 @@ void run(const uint8_t *data, size_t size) | |||
| const u5 *sigdata; | |||
| bool have_n = false; | |||
| char *fail; | |||
|
|
|||
| dev_bech32_nocsum = true; | |||
There was a problem hiding this comment.
This is the wrong fuzz test to enable the flag. We should do this during init in fuzz-bolt11-decode.c instead.
Alternative PR: #6789 |
|
Preferred version merged. |
Fixed from @morehouse with minor tweaks from me.