Skip to content

[TLS 1.3] Fuzz Target for Handshake Message Parsing#2977

Closed
reneme wants to merge 1 commit intomasterfrom
tls13/fuzzing
Closed

[TLS 1.3] Fuzz Target for Handshake Message Parsing#2977
reneme wants to merge 1 commit intomasterfrom
tls13/fuzzing

Conversation

@reneme
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

3 participants