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

Try4 fuzz decode receive #305

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

Conversation

sescandor
Copy link

@sescandor sescandor commented Mar 25, 2020

Includes fuzz target as well as corpus set.

@sescandor sescandor marked this pull request as ready for review April 2, 2020 11:17
Copy link
Member

@kazuho kazuho left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

I like the idea of fuzzing our connection startup. That's when many things happen, it's a good way of making sure that we are robust.

The limitation of the PR as is now is that it only fuzzes the decoding of datagram into multiple packets (i.e. quicly_decode_packet) and first few lines of quicly_receive. This is because Initial packets are authenticated using AEAD. All the spoofed Initial packets would end up having an incorrect AEAD tag, and that would lead to quicly_receive discarding those packets before making any use.

It would be great if we could make sure that the spoofed packet being passed to quicly_receive (or quicly_accept) would something that is actually being processed.

I think we can achieve that goal by doing the following:

  • Define and use a special random_bytes callback that returns a constant pattern. Doing so would fix the AEAD key that would be used for authenticating the Initial packets to a constant value.
  • Supply the packet image in decrypted form to the fuzzer. Then, when processing the input from the fuzzer, apply AEAD encryption, then pass that to quicly_receive (or quicly_accept).

WDYT?

quicly_cid_plaintext_t next_cid;
const char* host = "127.0.0.1";
const char* port = "4422";
ptls_iovec_t *resumption_token = (ptls_iovec_t *)malloc(sizeof(ptls_iovec_t));
Copy link
Member

Choose a reason for hiding this comment

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

I do not think we need a pointer (with allocate memory), as the only use of the value in relation to other functions is passing the value of the pointer in quicly_connect. This can be ptls_iovec_t resumption_token = ptls_iovec_init(NULL, 0).

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