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

tls-eio: add fuzz tests using crowbar #456

Merged
merged 3 commits into from
Dec 12, 2022
Merged

Conversation

talex5
Copy link
Contributor

@talex5 talex5 commented Nov 9, 2022

The test picks two random strings, one for the client to send and one for the server. It then runs a send and receive fiber for each end.

A dispatcher fiber then sends commands to these worker fibers. It may:

  • Ask a send fiber to send some plaintext from the message.
  • Ask a receive fiber to read some plaintext.
  • Ask the network to transmit some number of bytes in some direction.
  • Ask a send fiber to shut down the connection.
  • Yield (allowing the workers to make progress).

At the end, it asks any remaining send fibers to shut down, allows all network traffic to be transmitted, and waits for the receive fibers to end. It then checks that the receiver got everything that was sent (with a special case to avoid reporting errors due to #452).

By default, this just runs a few random examples. To use afl-fuzz:

opam switch create 5.0.0~beta1+afl ocaml-variants.5.0.0~beta1+options ocaml-option-afl

dune runtest   # (build the tests and do a quick run first)
mkdir input
echo hi > input/foo
cp certificates/server.{key,pem} .
afl-fuzz -m 1000 -i input -o output ./_build/default/eio/tests/fuzz.exe @@

If you revert #454, it finds the bug (even without afl-fuzz). It could be extended in future to test other features, such as reneg and key_update.

The test picks two random strings, one for the client to send and one
for the server. It then runs a send and receive fiber for each end.

A dispatcher fiber then sends commands to these worker fibers. It may:

- Ask a send fiber to send some plaintext from the message.
- Ask a receive fiber to read some plaintext.
- Ask the network to transmit some number of bytes in some direction.
- Ask a send fiber to shut down the connection.
- Yield (allowing the workers to make progress).

At the end, it asks any remaining send fibers to shut down, allows all
network traffic to be transmitted, and waits for the receive fibers to
end. It then checks that the receiver got everything that was sent.

By default, this just runs a few random examples. To use afl-fuzz:

    dune runtest
    mkdir input
    echo hi > input/foo
    cp certificates/server.{key,pem} .
    afl-fuzz -m 1000 -i input -o output ./_build/default/eio/tests/fuzz.exe @@
@hannesm
Copy link
Member

hannesm commented Nov 9, 2022

Thanks for your contribution. Since I'm pretty slow, would you mind to express what kind of failures you're after?

It looks you don't fuzz the input (for Reader), neither the TLS state machine (when which message is expected), but mostly the exposed (in tls-eio) state machine (i.e. once an error (or close) occured, there won't be any further data transported / all function fail). Is this correct?

Or rephrased, do you have an intuition (or actual measurement) about the coverage of this approach?

@talex5
Copy link
Contributor Author

talex5 commented Nov 9, 2022

Yes, this is mostly looking for bugs in the Eio wrapper (rather than in Tls itself). At the moment, it's just checking it works when used correctly, though it could be extended to inject faults and make sure they're handled sensibly. And I'd like to extend it to test reneg, since there are some XXX in the code there.

Or rephrased, do you have an intuition (or actual measurement) about the coverage of this approach?

No, but it should be possible to get the coverage with e.g. ppx_bisect.

@talex5
Copy link
Contributor Author

talex5 commented Nov 10, 2022

I added (instrumentation (backend bisect_ppx)) to the dune file and ran:

dune runtest --instrument-with bisect_ppx --force ./eio
bisect-ppx-report html

That reports 46% coverage for tls_eio.ml without the fuzz tests.

Annoyingly, you don't get coverage for fuzz tests because both bisect-ppx and crowbar use at_exit handlers, and they run in the wrong order (dumping the coverage before running the tests). But with a patched crowbar, it gets 54%. Seems to be mostly error paths missed (as expected) though some of the linger cases aren't hit.

@hannesm
Copy link
Member

hannesm commented Nov 10, 2022

Great, thanks for the numbers and clarification.

@talex5
Copy link
Contributor Author

talex5 commented Dec 12, 2022

I have updated this branch to Eio 0.7 now (it seemed better to do it here than in a separate PR because some changes are needed here too). If this isn't going to be merged soon, I can make a separate PR for that instead.

@talex5 talex5 mentioned this pull request Dec 12, 2022
@hannesm
Copy link
Member

hannesm commented Dec 12, 2022

@talex5 we can merge at any time, I'd appreciate to preserve some of the discussion in this PR as a comment in the test (since once the PR is merged, it's difficult to find it again) -- i.e. what the purpose is and what is fuzz-tested here (including some preliminary coverage stats).

@talex5
Copy link
Contributor Author

talex5 commented Dec 12, 2022

I'd appreciate to preserve some of the discussion in this PR as a comment in the test

I've added a comment to the top of the file now.

@hannesm hannesm merged commit 356772c into mirleft:main Dec 12, 2022
@hannesm
Copy link
Member

hannesm commented Dec 12, 2022

Thanks @talex5, merged.

hannesm added a commit to hannesm/opam-repository that referenced this pull request Feb 14, 2023
CHANGES:

* BREAKING: new opam package tls-lwt (formerly tls.lwt), in dune:
  (libraries tls.lwt) should now be libraries (tls-lwt)
  (mirleft/ocaml-tls#468 @hannesm, reported mirleft/ocaml-tls#449 by @mbacarella)
* tls: update to mirage-crypto 0.11 API (mirleft/ocaml-tls#468 @hannesm)
* tls: relax SignatureAlgorithms extension handling to allow OpenSSL
  interoperability tests with TLS 1.0 and TLS 1.1 (mirleft/ocaml-tls#469 @hannesm)
* tls: remove Utils.filter_map and and Utils.option, use Stdlib instead (mirleft/ocaml-tls#455
  @hannesm)
* tls: do not globally open Utils (mirleft/ocaml-tls#455 @hannesm)
* tls: export log source of Tracing module (mirleft/ocaml-tls#461 @bikallem)
* tls: remove unused ciphersuites to reduce binary size (mirleft/ocaml-tls#467 @hannesm)
* tls-lwt: do not catch out of memory exception (mirleft/ocaml-tls#469 @hannesm)
* tls-eio: add fuzz testing using crowbar (mirleft/ocaml-tls#456 mirleft/ocaml-tls#463 @talex5)
* tls-eio: update to eio 0.7 (mirleft/ocaml-tls#456 @talex5)
* tls-eio: fix test for develop with vendoring (mirleft/ocaml-tls#462 @bikallem)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants