From 874663baaee5eb8ed3b2219bd376d09369b421de Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Tue, 17 Oct 2023 10:52:54 +1030 Subject: [PATCH 1/6] fuzz: add test for bolt11 decoding [ Turned bech32 csum disable into a runtime flag --RR ] --- common/bech32.c | 11 +++++++++++ common/bech32.h | 6 ++++++ tests/fuzz/fuzz-bolt11-decode.c | 26 ++++++++++++++++++++++++++ tests/fuzz/fuzz-bolt11.c | 2 ++ 4 files changed, 45 insertions(+) create mode 100644 tests/fuzz/fuzz-bolt11-decode.c diff --git a/common/bech32.c b/common/bech32.c index ccf79d5b159c..38e475d3ed73 100644 --- a/common/bech32.c +++ b/common/bech32.c @@ -26,6 +26,10 @@ #include #include +#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION +bool dev_bech32_nocsum; +#endif + static uint32_t bech32_polymod_step(uint32_t pre) { uint8_t b = pre >> 25; return ((pre & 0x1FFFFFF) << 5) ^ @@ -142,6 +146,13 @@ bech32_encoding bech32_decode(char* hrp, uint8_t *data, size_t *data_len, const } ++i; } + +#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION + if (dev_bech32_nocsum) + /* Skip checksum and upper/lowercase checks when fuzzing. */ + return BECH32_ENCODING_BECH32; +#endif + if (have_lower && have_upper) { return BECH32_ENCODING_NONE; } diff --git a/common/bech32.h b/common/bech32.h index 794510be2755..ca5887a2e2aa 100644 --- a/common/bech32.h +++ b/common/bech32.h @@ -26,6 +26,7 @@ #define LIGHTNING_COMMON_BECH32_H #include "config.h" +#include #include #include @@ -130,5 +131,10 @@ int bech32_convert_bits(uint8_t* out, size_t* outlen, int outbits, extern const char bech32_charset[32]; extern const int8_t bech32_charset_rev[128]; +/* Global to weaken csum checks for fuzzing. */ +#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION +extern bool dev_bech32_nocsum; +#endif + #endif /* LIGHTNING_COMMON_BECH32_H */ diff --git a/tests/fuzz/fuzz-bolt11-decode.c b/tests/fuzz/fuzz-bolt11-decode.c new file mode 100644 index 000000000000..68f6ec9a3003 --- /dev/null +++ b/tests/fuzz/fuzz-bolt11-decode.c @@ -0,0 +1,26 @@ +#include "config.h" +#include + +#include +#include +#include +#include +#include +#include + +void init(int *argc, char ***argv) { common_setup("fuzzer"); } + +void run(const u8 *data, size_t size) +{ + char *str = tal_arr(NULL, char, size + 1); + char *fail_reason; + + memcpy(str, data, size); + str[size] = '\0'; + + bolt11_decode(str, str, /*our_features=*/NULL, /*description=*/NULL, + /*must_be_chain=*/NULL, &fail_reason); + + clean_tmpctx(); + tal_free(str); +} diff --git a/tests/fuzz/fuzz-bolt11.c b/tests/fuzz/fuzz-bolt11.c index d83040357379..6e53bfcca2b3 100644 --- a/tests/fuzz/fuzz-bolt11.c +++ b/tests/fuzz/fuzz-bolt11.c @@ -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; bolt11_decode_nosig(tmpctx, invoice_str, NULL, NULL, chainparams, &hash, &sigdata, &have_n, &fail); From 693f493c342b3f351b25ce83baee522775dada0b Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Tue, 17 Oct 2023 10:52:54 +1030 Subject: [PATCH 2/6] bolt11: return error on unexpected lightning prefix 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 ] --- common/bolt11.c | 1 - .../crash-98a2112c93362e35310c629081b5d60390062962 | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 tests/fuzz/corpora/fuzz-bolt11-decode/crash-98a2112c93362e35310c629081b5d60390062962 diff --git a/common/bolt11.c b/common/bolt11.c index 433a73d3fbd6..93414330f226 100644 --- a/common/bolt11.c +++ b/common/bolt11.c @@ -715,7 +715,6 @@ struct bolt11 *bolt11_decode_nosig(const tal_t *ctx, const char *str, memset(have_field, 0, sizeof(have_field)); b11->routes = tal_arr(b11, struct route_info *, 0); - assert(!has_lightning_prefix(str)); if (strlen(str) < 8) return decode_fail(b11, fail, "Bad bech32 string"); diff --git a/tests/fuzz/corpora/fuzz-bolt11-decode/crash-98a2112c93362e35310c629081b5d60390062962 b/tests/fuzz/corpora/fuzz-bolt11-decode/crash-98a2112c93362e35310c629081b5d60390062962 new file mode 100644 index 000000000000..0e6c3610ae2b --- /dev/null +++ b/tests/fuzz/corpora/fuzz-bolt11-decode/crash-98a2112c93362e35310c629081b5d60390062962 @@ -0,0 +1 @@ +lightning: \ No newline at end of file From 16c815890f2724644519ae13bb524525ac3870b1 Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Tue, 17 Oct 2023 10:52:54 +1030 Subject: [PATCH 3/6] bolt11: validate recovery ID 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 --- common/bolt11.c | 6 ++++++ .../crash-90ba6bcd63aa79a19b2df3d58a4fa0c4193614f0 | 1 + 2 files changed, 7 insertions(+) create mode 100644 tests/fuzz/corpora/fuzz-bolt11-decode/crash-90ba6bcd63aa79a19b2df3d58a4fa0c4193614f0 diff --git a/common/bolt11.c b/common/bolt11.c index 93414330f226..164d7283df74 100644 --- a/common/bolt11.c +++ b/common/bolt11.c @@ -917,6 +917,8 @@ struct bolt11 *bolt11_decode_nosig(const tal_t *ctx, const char *str, return b11; } +static bool valid_recovery_id(u8 recid) { return recid <= 3; } + /* Decodes and checks signature; returns NULL on error. */ struct bolt11 *bolt11_decode(const tal_t *ctx, const char *str, const struct feature_set *our_features, @@ -957,6 +959,10 @@ struct bolt11 *bolt11_decode(const tal_t *ctx, const char *str, assert(data_len == 0); + if (!valid_recovery_id(sig_and_recid[64])) + return decode_fail(b11, fail, "invalid recovery ID: %u", + sig_and_recid[64]); + if (!secp256k1_ecdsa_recoverable_signature_parse_compact (secp256k1_ctx, &sig, sig_and_recid, sig_and_recid[64])) return decode_fail(b11, fail, "signature invalid"); diff --git a/tests/fuzz/corpora/fuzz-bolt11-decode/crash-90ba6bcd63aa79a19b2df3d58a4fa0c4193614f0 b/tests/fuzz/corpora/fuzz-bolt11-decode/crash-90ba6bcd63aa79a19b2df3d58a4fa0c4193614f0 new file mode 100644 index 000000000000..cbebfa5a47a2 --- /dev/null +++ b/tests/fuzz/corpora/fuzz-bolt11-decode/crash-90ba6bcd63aa79a19b2df3d58a4fa0c4193614f0 @@ -0,0 +1 @@ +lntltc1UZZZZQQDQQpp5pppppppppppppppppZZZZZZZZZZZZQQQQQQQQQQQQQQQQQQQQQQQQQQQQQAQQQQQQQQQQQQQQQQZZZZZZZZZZZZZZZZZZZZZZZppppppppppppppppppppppppppppppppppppZZZZZZZZZZZZZZZZZZZZZZZZtltc \ No newline at end of file From f9a7f40afdda4c6e31c043bad1a9cc26d7e3469f Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Tue, 17 Oct 2023 10:52:54 +1030 Subject: [PATCH 4/6] bolt11: check return value of pull_all 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 --- common/bolt11.c | 2 ++ .../crash-6a09efacc7816949fc57d006a8b513cbb7857f2f | 1 + 2 files changed, 3 insertions(+) create mode 100644 tests/fuzz/corpora/fuzz-bolt11-decode/crash-6a09efacc7816949fc57d006a8b513cbb7857f2f diff --git a/common/bolt11.c b/common/bolt11.c index 164d7283df74..a168d66e5c30 100644 --- a/common/bolt11.c +++ b/common/bolt11.c @@ -390,6 +390,8 @@ static const char *decode_f(struct bolt11 *b11, fallback = scriptpubkey_p2sh_hash(b11, shash); } else if (version < 17) { u8 *f = pull_all(tmpctx, hu5, data, field_len, false, &err); + if (!f) + return err; if (version == 0) { if (tal_count(f) != 20 && tal_count(f) != 32) return tal_fmt(b11, diff --git a/tests/fuzz/corpora/fuzz-bolt11-decode/crash-6a09efacc7816949fc57d006a8b513cbb7857f2f b/tests/fuzz/corpora/fuzz-bolt11-decode/crash-6a09efacc7816949fc57d006a8b513cbb7857f2f new file mode 100644 index 000000000000..a088764773f5 --- /dev/null +++ b/tests/fuzz/corpora/fuzz-bolt11-decode/crash-6a09efacc7816949fc57d006a8b513cbb7857f2f @@ -0,0 +1 @@ +lnltc1Uggzzzzfzzffffffffffffffffffffffffffffffgfffffffffffffffffzzzzfzzfffffffffffffffffffffffffffffffffffffffffffffffffffff \ No newline at end of file From 7e056c4abc3b383fba9cf3dc1d9306ccdafabde0 Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Tue, 17 Oct 2023 10:52:54 +1030 Subject: [PATCH 5/6] bolt11: avoid reading uninitialized memory 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' --- common/bolt11.c | 6 +++++- .../crash-ad3693f6c454ce739ca67a4aff234cb3f3f598b5 | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 tests/fuzz/corpora/fuzz-bolt11-decode/crash-ad3693f6c454ce739ca67a4aff234cb3f3f598b5 diff --git a/common/bolt11.c b/common/bolt11.c index a168d66e5c30..c6eb0bd8f2bc 100644 --- a/common/bolt11.c +++ b/common/bolt11.c @@ -76,7 +76,11 @@ static const char *pull_uint(struct hash_u5 *hu5, err = pull_bits(hu5, data, data_len, &be_val, databits, true); if (err) return err; - *val = be64_to_cpu(be_val) >> (sizeof(be_val) * CHAR_BIT - databits); + if (databits == 0) + *val = 0; + else + *val = be64_to_cpu(be_val) >> + (sizeof(be_val) * CHAR_BIT - databits); return NULL; } diff --git a/tests/fuzz/corpora/fuzz-bolt11-decode/crash-ad3693f6c454ce739ca67a4aff234cb3f3f598b5 b/tests/fuzz/corpora/fuzz-bolt11-decode/crash-ad3693f6c454ce739ca67a4aff234cb3f3f598b5 new file mode 100644 index 000000000000..a37d233f51ce --- /dev/null +++ b/tests/fuzz/corpora/fuzz-bolt11-decode/crash-ad3693f6c454ce739ca67a4aff234cb3f3f598b5 @@ -0,0 +1 @@ +lnltc1zzzzzAzcQQQQQZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZXZZZZZZZZZZZZZZZZZJZZZZZZZZZZzzzZZZZZZZZ \ No newline at end of file From 71a3cc1ef598b796acd030b6b47e57ee5d6743b6 Mon Sep 17 00:00:00 2001 From: Matt Morehouse Date: Tue, 17 Oct 2023 11:29:47 +1030 Subject: [PATCH 6/6] bolt11: don't abort on invalid pubkey Rather than crashing the entire node on invalid pubkey, we should return an error. Detected by libFuzzer: ==250024== ERROR: libFuzzer: deadly signal [ Changed so that `n` really does check that it's valid --RR ] #7 abort #8 bolt11_decode common/bolt11.c:1002:4 --- common/bolt11.c | 16 ++++++++++++++-- ...ash-08a0ea0c1dd7003293bf5d6e05708c6918757cd7 | Bin 0 -> 3002 bytes 2 files changed, 14 insertions(+), 2 deletions(-) create mode 100644 tests/fuzz/corpora/fuzz-bolt11-decode/crash-08a0ea0c1dd7003293bf5d6e05708c6918757cd7 diff --git a/common/bolt11.c b/common/bolt11.c index c6eb0bd8f2bc..e84cf6dc784a 100644 --- a/common/bolt11.c +++ b/common/bolt11.c @@ -306,14 +306,26 @@ static const char *decode_n(struct bolt11 *b11, const u5 **data, size_t *field_len, bool *have_n) { + const char *err; + assert(!*have_n); /* BOLT #11: * * A reader... MUST skip over unknown fields, OR an `f` field * with unknown `version`, OR `p`, `h`, `s` or `n` fields that do * NOT have `data_length`s of 52, 52, 52 or 53, respectively. */ - return pull_expected_length(b11, hu5, data, field_len, 53, 'n', - have_n, &b11->receiver_id.k); + err = pull_expected_length(b11, hu5, data, field_len, 53, 'n', + have_n, &b11->receiver_id.k); + + /* If that gave us nodeid, check it. */ + if (*have_n) { + struct pubkey k; + if (!pubkey_from_node_id(&k, &b11->receiver_id)) + return tal_fmt(b11, "invalid public key %s", + node_id_to_hexstr(tmpctx, &b11->receiver_id)); + } + + return err; } /* BOLT #11: diff --git a/tests/fuzz/corpora/fuzz-bolt11-decode/crash-08a0ea0c1dd7003293bf5d6e05708c6918757cd7 b/tests/fuzz/corpora/fuzz-bolt11-decode/crash-08a0ea0c1dd7003293bf5d6e05708c6918757cd7 new file mode 100644 index 0000000000000000000000000000000000000000..7bdc44850d72228113a89cd2d105dfbc8e6b7d3a GIT binary patch literal 3002 zcmc&$Ne;p=4D6SX;0N3|<$wg`h}09Rdg%W}oW;!~A#S=*OAxPPk5d&s{rDN*r+eEf zmMdw4mW%#4l}wh_24f6DR8(vYs}$N zQNg-Awg{3-AYfWmF2=sycKPHMR`E0+)g2?C$@GvGaq?%h>ZTRg&<^W_2zHAK)(PxHv>?F%@Av*v487qKZpX)!1e8*B?saoz1vb}^ nf^}P8lcSNsn!pPtSz{oJHAuGh#)Fp>|A6?MxNl6;Nkq2q literal 0 HcmV?d00001