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

Add HTTP/3 fuzzing harness #97

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

elopez
Copy link
Contributor

@elopez elopez commented Jan 5, 2024

This PR implements support for building cURL with QuicTLS, ngtcp2 and nghttp3 and adds a new harness, curl_fuzzer_http3, that fuzzes the HTTP/3 implementation.

scripts/download_ngtcp2.sh includes a patch that adds minimal support on ngtcp2 for using AF_UNIX sockets.

scripts/install_curl.sh includes a patch to disable failing when calling connect() on the QUIC socket (as it will fail if you're using an AF_UNIX socket, like the fuzzer does).

@elopez elopez marked this pull request as ready for review January 5, 2024 15:11
@cmeister2 cmeister2 self-assigned this Jan 5, 2024
Copy link
Collaborator

@cmeister2 cmeister2 left a comment

Choose a reason for hiding this comment

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

I'm a little dubious about the direction here; rather than http3 interfering with working builds (and the proper openssl) I'd be pushing for a separate library install directory just for http3.

mainline.sh Outdated Show resolved Hide resolved
scripts/download_nghttp3.sh Outdated Show resolved Hide resolved

# Teach ngtcp2 about sockets
pushd $1
patch -p1 <<'EOF'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inlining the patch here is nasty. I'd rather this were in a patch file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, doing this via a patch mechanism severely limits updating this dependency in the future. What's the long-term plan here - add support to ngtcp2 upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, having the patch here is not a permanent solution. The long-term plan would be to work with ngtcp2 to make it more fuzzing friendly -- e.g. by allowing it to work with AF_UNIX sockets like this patch does.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bagder do you have an opinion here? In my experience this kind of mechanism is fairly brittle; I don't know if it's something we want to support moving forwards.

scripts/download_openssl_quic.sh Outdated Show resolved Hide resolved
@@ -45,16 +45,39 @@ fi

pushd ${SRCDIR}

# TODO: Ignore HTTP3 socket connection failures
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto: inline patches.

@@ -45,16 +45,39 @@ fi

pushd ${SRCDIR}

# TODO: Ignore HTTP3 socket connection failures
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this a TODO? Should it be?

rc = connect(ctx->sock, &ctx->addr.sa_addr, ctx->addr.addrlen);
if(-1 == rc) {
- return socket_connect_result(data, ctx->r_ip, SOCKERRNO);
+ /* return socket_connect_result(data, ctx->r_ip, SOCKERRNO); */
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels wrong - why are we disabling this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is quite hacky as you noticed, so I've specially marked it with a TODO. The harness uses AF_UNIX sockets and it makes the connect() call fail, which blocks fuzzing. Ideally we would have something nicer than this patch on cURL itself, to allow for fuzzing to proceed.

mainline.sh Outdated

# Install openssl
${SCRIPTDIR}/handle_x.sh openssl ${OPENSSLDIR} ${INSTALLDIR} || exit 1
# ${SCRIPTDIR}/handle_x.sh openssl ${OPENSSLDIR} ${INSTALLDIR} || exit 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is the solution, commenting this out is wrong. Remove the block altogether.

scripts/download_ngtcp2.sh Outdated Show resolved Hide resolved
"QUICTLS_VERSION=(?<currentValue>.*)\\s"
],
"datasourceTemplate": "github-tags",
"depNameTemplate": "openssl/openssl",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this actually wants to be quictls/openssl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! I'll get that changed


# Teach ngtcp2 about sockets
pushd $1
patch -p1 <<'EOF'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bagder do you have an opinion here? In my experience this kind of mechanism is fairly brittle; I don't know if it's something we want to support moving forwards.

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.

3 participants