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 1.3] Fuzz Target for Handshake Message Parsing #2977

Closed
wants to merge 1 commit into from

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented May 19, 2022

This is a basic fuzz target that shoves the fuzzer input into Handshake_Layer_13::copy_data() and tries to parse the messages. We've seen fairly good coverage of the handshake message parsing code with a seed corpus based on the RFC 8448 handshake messages.

We weren't able to figure out how to integrate this target into the OSS-Fuzz run, though. OSS-Fuzz seems to pick up the new target but (obviously) fails to find a seed corpus for it.

For CI convenience we left the base commits of the TLS 1.3 PR in here. The relevant fuzz target can be found in this commit: 3151d0d

@lz101010
Copy link
Contributor

lz101010 commented May 19, 2022

The fuzzer configuration is rather opaque to us, we're guessing it's somewhere in https://oss-fuzz.com/ to which we don't have access. Ideally the seed would change between runs, it's currently always 1337.

In any case - we'd like to speed up the fuzzers coverage, so we added a corpus over at randombit/crypto-corpus#1 which oss-fuzz should then be able to pick up here: https://github.com/google/oss-fuzz/blob/master/projects/botan/Dockerfile#L20


Regarding our local test setup, we had to do a little workaround in order to run libFuzzer:

./configure.py --compiler-cache=ccache --cc=clang --cc-bin=/usr/local/opt/llvm@13/bin/clang++ --build-fuzzer=libfuzzer2 --unsafe-fuzzer-mode --enable-sanitizers=coverage,address,undefined,fuzzer
make -j$(nproc) fuzzers
DYLD_LIBRARY_PATH=. build/fuzzer/tls_13_handshake_layer msg_corpus

Notes:

We also visualized the coverage of the corpus (without additional fuzzing) via

mkdir coverage
./configure.py --compiler-cache=ccache --cc=clang --cc-bin=/usr/local/opt/llvm@13/bin/clang++ --build-fuzzer=libfuzzer2 --unsafe-fuzzer-mode --enable-sanitizers=coverage,address,undefined,fuzzer --with-coverage-info
make -j$(nproc) fuzzers
DYLD_LIBRARY_PATH=. build/fuzzer/tls_13_handshake_layer -runs=0 msg_corpus
gcovr --html --html-details --object-directory=build/obj/lib -r . -o coverage/coverage.html
open coverage/coverage.html

See google/fuzzing#41 (comment) for details.

@codecov-commenter
Copy link

codecov-commenter commented May 20, 2022

Codecov Report

Merging #2977 (26738a0) into master (987c7af) will decrease coverage by 0.03%.
The diff coverage is 92.62%.

❗ Current head 26738a0 differs from pull request most recent head 6486350. Consider uploading reports for the commit 6486350 to get more accurate results

@@            Coverage Diff             @@
##           master    #2977      +/-   ##
==========================================
- Coverage   92.57%   92.53%   -0.04%     
==========================================
  Files         596      595       -1     
  Lines       69729    69713      -16     
  Branches     6613     6598      -15     
==========================================
- Hits        64552    64512      -40     
- Misses       5144     5168      +24     
  Partials       33       33              
Impacted Files Coverage Δ
src/cli/tls_utils.cpp 77.64% <ø> (ø)
src/lib/tls/msg_cert_req.cpp 89.85% <ø> (ø)
src/lib/tls/tls12/tls_channel_impl_12.cpp 89.04% <0.00%> (ø)
src/lib/tls/tls12/tls_server_impl_12.cpp 88.41% <ø> (ø)
src/lib/tls/tls_server.cpp 65.90% <0.00%> (ø)
src/lib/tls/tls_suite_info.cpp 100.00% <ø> (ø)
src/lib/utils/assert.cpp 100.00% <ø> (ø)
src/lib/tls/tls_algos.cpp 69.61% <38.88%> (-16.99%) ⬇️
src/lib/tls/tls12/tls_handshake_state.cpp 84.03% <47.05%> (-2.11%) ⬇️
src/lib/tls/msg_session_ticket.cpp 76.92% <50.00%> (ø)
... and 83 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 987c7af...6486350. Read the comment docs.

@reneme
Copy link
Collaborator Author

reneme commented Jul 5, 2022

Rebased to master.

Co-Authored-By: Lukas Zeller <lukas.zeller@nexenio.com>
Comment on lines +15 to +31
Botan::TLS::Handshake_Layer prepare(const std::vector<uint8_t>& data)
{
Botan::TLS::Handshake_Layer hl(Botan::TLS::Connection_Side::CLIENT);
hl.copy_data(data);
return hl;
}

} // namespace;


void fuzz(const uint8_t in[], size_t len)
{
static Botan::TLS::Default_Policy policy;

try
{
std::vector<uint8_t> v(in, in + len);
Copy link
Contributor

@lz101010 lz101010 Jul 25, 2022

Choose a reason for hiding this comment

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

@reneme I'm unable to push the following change (403) to the vector's type:

Suggested change
Botan::TLS::Handshake_Layer prepare(const std::vector<uint8_t>& data)
{
Botan::TLS::Handshake_Layer hl(Botan::TLS::Connection_Side::CLIENT);
hl.copy_data(data);
return hl;
}
} // namespace;
void fuzz(const uint8_t in[], size_t len)
{
static Botan::TLS::Default_Policy policy;
try
{
std::vector<uint8_t> v(in, in + len);
Botan::TLS::Handshake_Layer prepare(const Botan::secure_vector<uint8_t>& data)
{
Botan::TLS::Handshake_Layer hl(Botan::TLS::Connection_Side::CLIENT);
hl.copy_data(data);
return hl;
}
} // namespace;
void fuzz(const uint8_t in[], size_t len)
{
static Botan::TLS::Default_Policy policy;
try
{
Botan::secure_vector<uint8_t> v(in, in + len);

Can you approve the suggestion so the build can hopefully pass? Alternately I can open a separate PR, but that feels overkill.

Copy link
Contributor

Choose a reason for hiding this comment

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

I decided to open a new PR anyway, the fuzzer now works as intended: #3013

I suggest closing this PR (I apparently can't do this myself) in favor of the other one, because (a) the other one is ready to merge (b) I can rebase the other PR if something else changes in the meantime.

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.

None yet

3 participants