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

bolt11: fix crashes and memory safety errors #6789

Merged
merged 7 commits into from
Oct 17, 2023

Conversation

morehouse
Copy link
Contributor

This PR extends @dergoegge's fuzz-bolt11 target slightly and fixes the bugs found by it.

This is an alternative to #6776 and incorporates @rustyrussell's feedback into the fixes.

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

Corpus input bf703c2c20c0818af70a8c4caad6e6fd8cfd1ac6 triggers the UBSan
report, but we didn't previously realize this because UBSan has been
disabled in the CI run. We rename the input to indicate its usefulness
as a permanent regression test.
If both databits and *data_len are 0, pull_uint return uninitialized
stack memory in *val.

Detected by valgrind and UBSan.

valgrind:
==173904== Use of uninitialised value of size 8
==173904==    __sanitizer_cov_trace_cmp8
==173904==    decode_c (bolt11.c:292)
==173904==    bolt11_decode_nosig (bolt11.c:877)

UBSan:
common/bolt11.c:79:29: runtime error: shift exponent 64 is too large for 64-bit type 'uint64_t' (aka 'unsigned long')

Corpus input e6f7b9744a7d79b2aa4f7c477707bdd3483f40fa triggers the UBSan
report, but we didn't previously realize this because UBSan has been
disabled in the CI run. We rename the input to indicate its usefulness
as a permanent regression test.
ASan and UBSan were disabled by
364de00, which has already prevented
multiple bugs from being discovered prior to merging.
Remove the assertion so that an error is returned for invalid bech32.
An error is preferable to crashing the entire node 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
There may be bugs in signature validation, so we should fuzz that too.
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
Rather than crashing the entire node on invalid pubkey, check the
validity of the pubkey in decode_n, and return an error if invalid.

Detected by libFuzzer:
==265599== ERROR: libFuzzer: deadly signal
    ElementsProject#7 abort
    ElementsProject#8 bolt11_decode common/bolt11.c:999:4
@morehouse morehouse mentioned this pull request Oct 17, 2023
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 8a9349a for the bolt11.c changes

Related the fuzz I can not help :)

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack 8a9349a

@rustyrussell rustyrussell merged commit 87f4907 into ElementsProject:master Oct 17, 2023
38 checks passed
@morehouse morehouse deleted the fix_bolt11 branch October 18, 2023 14:07
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.

3 participants