fuzz-tests: Add a test for tlv_span()

[Chand-ra]

tlv_span() in common/bolt12.c parses untrusted input in the form of a u8 *tlvstream. Add a test for it.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

• The changelog has been updated in the relevant commit(s) according to the guidelines.
• Tests have been added or modified to reflect the changes.
• Documentation has been reviewed and updated as needed.
• Related issues have been listed and linked, including any that this PR closes.

CC: @morehouse

[morehouse]

Concept ACK

[morehouse]

Since fromwire_u64 automatically handles the case where size is too small, I think we can remove this check.

Suggested change

	if (size < 2 * sizeof(u64))
	return;

[morehouse]

A possible additional check:

Suggested change

	tlv_span(buf, minfield, maxfield, &size);
	size_t span_size = tlv_span(buf, minfield, maxfield, &span_start_offset);
	assert(span_start_offset + span_size <= size);

[Chand-ra]

I think this assertion might've exposed a bug. This is what's happening in the function under test, tlv_span(), from my understanding of the situation:

When tlv_span() receives an empty stream, tlvlen (which is tal_bytelen(tlvstream)) is 0. This means the following loop is never entered:

while (tlvlen) {
		const u8 *before = cursor;
		bigsize_t type = fromwire_bigsize(&cursor, &tlvlen);
		bigsize_t len = fromwire_bigsize(&cursor, &tlvlen);
		if (type >= minfield && start == NULL)
			start = before;
		if (type > maxfield)
			break;
		fromwire_pad(&cursor, &tlvlen, len);
		end = cursor;
	}

which means the local pointers start and end are never assigned a valid memory address and remain NULL, their initial value. Then at the end when calculating span_start_offset:

if (startp)
	*startp = start - tlvstream;
return end - start;

Since start is NULL, this becomes NULL - tlvstream, which is undefined behavior. The result is a garbage memory address that, when interpreted as a size_t, becomes some large garbage number. The function's return value (span_size) is calculated as end - start = NULL - NULL = 0.

The assertion evaluates assert(large_garbage_value + 0 <= 0), which is false, causing the fuzzer to terminate.

I've pushed the modified test with the assertion, feel free to take a look for yourself.

[morehouse]

Yep, that's UB. I'm not sure if we'd ever encounter an offer with 0 length in practice, even in an adversarial case.

But the fix seems simple enough either way. Probably just need to change the initial value of end:

size_t tlv_span(const u8 *tlvstream, u64 minfield, u64 maxfield,
                size_t *startp)
{
        const u8 *cursor = tlvstream;
        size_t tlvlen = tal_bytelen(tlvstream);
        const u8 *start, *end;

        start = NULL;
        end = tlvstream + tlvlen;
        while (tlvlen) {
                const u8 *before = cursor;
                bigsize_t type = fromwire_bigsize(&cursor, &tlvlen);
                bigsize_t len = fromwire_bigsize(&cursor, &tlvlen);
                if (type >= minfield && start == NULL)
                        start = before;
                if (type > maxfield)
                        break; 
                fromwire_pad(&cursor, &tlvlen, len);
                end = cursor;        
        }
        if (!start)
                start = end;

        if (startp)
                *startp = start - tlvstream;
        return end - start;
}

[Chand-ra]

I looked around and found only one invocation of tlv_span(), in plugins/fetchinvoice.c:

static bool invoice_matches_request(struct command *cmd, const u8 *invbin,
const struct tlv_invoice_request *invreq)
{
        size_t ir_len1, ir_len2, ir_start1, ir_start2;
        size_t inv_len1, inv_len2, inv_start1, inv_start2;
        u8 *wire;

        /* We linearize then strip signature.  This is dumb! */
        wire = tal_arr(tmpctx, u8, 0);
        towire_tlv_invoice_request(&wire, invreq);
        ir_len1 = tlv_span(wire, 0, 159, &ir_start1);
        ir_len2 = tlv_span(wire, 1000000000, 2999999999, &ir_start2);

        inv_len1 = tlv_span(invbin, 0, 159, &inv_start1);
        inv_len2 = tlv_span(invbin, 1000000000, 2999999999, &inv_start2);
        return memeq(wire + ir_start1, ir_len1,
                     invbin + inv_start1, inv_len1)
                && memeq(wire + ir_start2, ir_len2,
                         invbin + inv_start2, inv_len2);
}

And walking up this call stack:

static struct command_result *handle_invreq_response(struct command *cmd, struct sent *sent,
const char *buf, const jsmntok_t *om)
{
...
...
        if (!invoice_matches_request(cmd, invbin, sent->invreq)) {
                badfield = "invoice_request match";
                goto badinv;
        }

  struct command_result *handle_invoice_onion_message(struct command *cmd, const char *buf,
                                                    const jsmntok_t *om, const struct secret *pathsecret)
{
...
...
        if (sent->invreq)
                return handle_invreq_response(cmd, sent, buf, om);

        return NULL;
}

So it doesn't look like the undefined behavior can be triggered externally. Would we still want to guard against it though, perhaps to be future-proof?

[morehouse]

I would prefer to fix it, even if only to keep the assert(span_start_offset + span_size <= size) check in this fuzz test.

[Chand-ra]

A quick correction: the bug might be triggered through this code path when sent->invreq is an empty stream, since empty doesn't necessarily imply sent->invreq = NULL.

[morehouse]

tlv_span is public. We shouldn't need to include bolt12.c file directly.

[Chand-ra]

We don’t need to modify tests/fuzz/Makefile in this way, so I thought this approach would be better—especially since there’s no need to stub anything, and we likely want to minimize the number of common/ objects linked with the fuzzers.

[morehouse]

If we add bolt12.c to the Makefile, we shouldn't need to do any stubs either.

If we're concerned about the fuzz target sizes increasing, we can start linking dependencies only to the targets that need them (and which you're already doing for fuzz-error-warning).