Skip to content

fuzz: Add input for from_bech32_charset off-by-one bug#7327

Merged
rustyrussell merged 1 commit into
ElementsProject:masterfrom
dergoegge:2024-05-bolt12-obo-input
Jun 20, 2024
Merged

fuzz: Add input for from_bech32_charset off-by-one bug#7327
rustyrussell merged 1 commit into
ElementsProject:masterfrom
dergoegge:2024-05-bolt12-obo-input

Conversation

@dergoegge
Copy link
Copy Markdown
Contributor

This PR adds the triggering input for the bug described in #7322 to the fuzz-bolt12-bech32-decode corpus (as suggested in #7322 (comment)).

@dergoegge
Copy link
Copy Markdown
Contributor Author

cc @morehouse

@morehouse
Copy link
Copy Markdown
Contributor

morehouse commented May 22, 2024

This input doesn't seem to trigger the error case fixed by #7322.

To test I applied this patch and recompiled:

diff --git a/common/bech32_util.c b/common/bech32_util.c
index bae0f1e81..de55bc036 100644
--- a/common/bech32_util.c
+++ b/common/bech32_util.c
@@ -83,6 +83,7 @@ bool from_bech32_charset(const tal_t *ctx,
        for (size_t i = 0; i < datalen; i++) {
                int c = sep[1+i];
                c = fixup_char(c, &upper, &lower);
+               if (c == 128) abort();
                if (c < 0 || c >= 128)
                        goto fail;
                if (bech32_charset_rev[c] == -1)

Then I did:

$ ./tests/fuzz/fuzz-bolt12-bech32-decode tests/fuzz/corpora/fuzz-bolt12-bech32-decode/crash-0018e3d297beda62648c4cda4fff4537bcbf1986

Running: tests/fuzz/corpora/fuzz-bolt12-bech32-decode/crash-0018e3d297beda62648c4cda4fff4537bcbf1986
Executed tests/fuzz/corpora/fuzz-bolt12-bech32-decode/crash-0018e3d297beda62648c4cda4fff4537bcbf1986 in 0 ms
***
*** NOTE: fuzzing was not performed, you have only
***       executed the target code on a fixed set of inputs.
***

@dergoegge
Copy link
Copy Markdown
Contributor Author

This input doesn't seem to trigger the error case

Very interesting, I'm gonna try to have a closer look tomorrow but just skimming the code again now, could this potentially be caused by the signedness of sep not being explicitly set? Googling around a bit it looks like the signedness of char (i.e. without a signed or unsigned) is not defined by the C spec (and e.g. on arm it is unsigned).

@morehouse
Copy link
Copy Markdown
Contributor

Seems like you nailed it:

--- a/common/bech32_util.c
+++ b/common/bech32_util.c
@@ -81,6 +81,7 @@ bool from_bech32_charset(const tal_t *ctx,
        datalen = bech32_len - (sep + 1 - bech32);
        u5data = tal_arr(NULL, u5, datalen);
        for (size_t i = 0; i < datalen; i++) {
+               if (sep[1+i] == (char)128) abort();
                int c = sep[1+i];
                c = fixup_char(c, &upper, &lower);
                if (c < 0 || c >= 128)
$ make
$ ./tests/fuzz/fuzz-bolt12-bech32-decode tests/fuzz/corpora/fuzz-bolt12-bech32-decode/crash-0018e3d297beda62648c4cda4fff4537bcbf1986
...
==243216== ERROR: libFuzzer: deadly signal                                                                            
...
    #6 0x7f72318cc8fe in abort (/lib64/libc.so.6+0x268fe)
    #7 0x471146 in from_bech32_charset common/bech32_util.c:84:30                           

Copy link
Copy Markdown
Contributor

@morehouse morehouse left a comment

Choose a reason for hiding this comment

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

ACK 4a423a9

@rustyrussell
Copy link
Copy Markdown
Contributor

Thanks!

@rustyrussell rustyrussell merged commit 2590157 into ElementsProject:master Jun 20, 2024
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