Skip to content

[WIP] fuzz-tests: Add stateful fuzz test for the handshake protocol #8228

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Chand-ra
Copy link

Existing fuzz targets focus on testing individual components of
the handshake protocol as defined by BOLT #8. Add a stateful
fuzz test to evaluate the protocol's state transitions thoroughly.

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.

Copy link
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.

This fuzz target allows acts to be executed out-of-order, so I don't think it's a useful test as-is.

Even if we fix that, I'm skeptical that this target will provide much benefit over the existing single-act fuzz targets, since it is already quite difficult for the fuzzer to successfully complete just a single act. Trying to get the fuzzer to complete multiple acts successfully seems quite unlikely.

That said, if you can demonstrate the fuzzer completing multiple acts in sequence using a coverage report, I would be interested in merging this target.

struct io_conn *conn = io_new_conn(tmpctx, -1, NULL, NULL);

while (bytes_remaining > 0) {
int op = bytes[0] % (STATES_COUNT - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the -1 prevents RESPONDER_ACT3_RECEIVE from ever being chosen.

while (bytes_remaining > 0) {
int op = bytes[0] % (STATES_COUNT - 1);
bytes++; bytes_remaining--;
switch(op) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach allows the various acts to be executed out-of-order, which should never happen in the real world. I think you would want to execute each act in order rather than letting the fuzzer choose the order.

Copy link
Author

@Chand-ra Chand-ra Apr 12, 2025

Choose a reason for hiding this comment

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

I was aiming to simulate a corrupted channel where messages could arrive out of order. But if we abandon that idea, where do we inject the fuzzer’s data? Corrupting individual acts is already well-covered by existing tests, so that approach might not yield much value. Maybe, as you suggested, this fuzz test isn’t particularly worthwhile.

Copy link
Contributor

@morehouse morehouse Apr 15, 2025

Choose a reason for hiding this comment

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

Incorrect messages can certainly arrive, but the various act handlers should still always execute in the correct order.

Maybe, as you suggested, this fuzz test isn’t particularly worthwhile.

Yeah, if you're interested in stateful fuzzing, I'd suggest looking at something like the funding flow. There's various messages that arrive and change state (e.g., open_channel, funding_signed). It would be good to test that out-of-order messages don't cause any crashes or other bad things.

Changelog-None: Existing fuzz targets focus on testing individual
components of the handshake protocol as defined by BOLT ElementsProject#8. Add a
stateful fuzz test to evaluate the protocol's state transitions
thoroughly.
@Chand-ra
Copy link
Author

Hey @morehouse,

I’ve reworked the test so it now drives the acts in real‑world sequence and successfully completes a full handshake. Right now it doesn’t do much fuzzing, so I’m thinking of adding custom mutators to target specific fields in each act and the handshake state—while still keeping the overall structure valid. Do you think it’s worth the extra effort, or should we abandon this ship altogether after all?

@morehouse
Copy link
Contributor

I’ve reworked the test so it now drives the acts in real‑world sequence and successfully completes a full handshake.

Not sure I understand what's really being tested by the new target.

Does this actually complete a valid handshake? It looks like it's failing the handshake multiple times and then continuing on anyway. What is actually being fuzzed?

@Chand-ra
Copy link
Author

Not sure I understand what's really being tested by the new target.

Does this actually complete a valid handshake? It looks like it's failing the handshake multiple times and then continuing on anyway.

The handshake isn’t allowed to “fail” outright—instead, we deliberately abort after each write to the io_conn (in our test, that’s simulated by the fuzz_ctx::act_* buffers). By forcing an error on every write, we simulate a two‐way exchange: one side writes its Act, errors out immediately, then the other side reads from the buffer and proceeds. Without this break, act_one_initiator would try to write Act 1 and then immediately read Act 2—which hasn’t been written yet—so it would always fail. Splitting the process at each write lets us drive the exact six‑step flow:

  1. Initiator writes Act 1 → forced error
  2. Responder reads Act 1
  3. Responder writes Act 2 → forced error
  4. Initiator reads Act 2
  5. Initiator writes Act 3 → forced error
  6. Responder reads Act 3
  7. If all the steps were successfully executed, the handshake completes successfully.

The handshake's success can be verified by the fact that silent_success() is called at the end of each run.

What is actually being fuzzed?

​Not much beyond the ephemeral keys. Right now this target only unit‑tests the handshake’s state‑transition sequence. I’m thinking of injecting fuzzer data into the Acts' fields and handshake states mid‑flight—while still enforcing the correct Act order—to try and trigger edge cases. Do you think that approach will be worth the effort?

@morehouse
Copy link
Contributor

The handshake isn’t allowed to “fail” outright—instead, we deliberately abort after each write to the io_conn (in our test, that’s simulated by the fuzz_ctx::act_* buffers). By forcing an error on every write, we simulate a two‐way exchange: one side writes its Act, errors out immediately, then the other side reads from the buffer and proceeds. Without this break, act_one_initiator would try to write Act 1 and then immediately read Act 2—which hasn’t been written yet—so it would always fail. Splitting the process at each write lets us drive the exact six‑step flow:

This makes sense, thanks for explaining.

The handshake's success can be verified by the fact that silent_success() is called at the end of each run.

This is a hint that the target isn't doing the right thing. The handshake should fail sometimes too if the fuzzer is actually influencing the process in a meaningful way.

What is actually being fuzzed?

​Not much beyond the ephemeral keys. Right now this target only unit‑tests the handshake’s state‑transition sequence.

Yeah, in the current state the fuzz target is fairly useless, only varying the ephemeral key RNG seed, which the other targets already do.

I’m thinking of injecting fuzzer data into the Acts' fields and handshake states mid‑flight—while still enforcing the correct Act order—to try and trigger edge cases. Do you think that approach will be worth the effort?

The other fuzz targets already do this. I don't imagine there's much benefit to also doing it here.

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.

2 participants