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

Fix harmless two byte buffer write overflow in doh_encode #4352

Closed
wants to merge 3 commits into from

Conversation

@pauldreik
Copy link
Contributor

commented Sep 14, 2019

The check for buffer length in

curl/lib/doh.c

Line 87 in 5977664

if(len < (12 + hostlen + 4))
is wrong, which leads to a two byte buffer write overflow.

I reported a related bug in curl/doh at an early stage when investigating this, via private email to @bagder. After looking further into it, I filed on hackerone, and after discussions there it was concluded that

  • it is harmless
  • #4345 is insufficient
  • this pull request replaces it, if favourable reviewed

doh_encode() is an internal function, and the only exposure it gets is through

curl/lib/doh.c

Line 195 in 5977664

DOHcode d = doh_encode(host, dnstype, p->dohbuffer, sizeof(p->dohbuffer),
where it writes into a fixed length buffer in struct dnsprobe

The only way to trigger this externally is to use doh and use a hostname of a particular length such that it is short enough not to be caught by the length check, but long enough to write outside the buffer.

If the overflow happens, it is luckily harmless, because the overwrite goes into the length member of
struct dnsprobe. That length member is overwritten by

curl/lib/doh.c

Line 134 in 5977664

*olen = dnsp - orig;
and all is well again. I do not believe the compiler is allowed to rearrange the length write with the buffer write, as it can't assume the char pointer to the buffer does not alias with the size.

This pull request adds a unit test which proves the bug and that it has been fixed.

To trigger the behaviour, the following curl command can be used (the lenght of the weird hostname is carefully selected and no part between the dots may be longer than 63):

src/curl --doh-url https://irrelevant/ x....xxxxxxxxxxxxxxxxxxxxx.x....x.xxxxxxxxxx.xxxxxxxxx.xxxxxxxxxxx.xxxxxx.xxxxxxxxxxxxxxxxxxxxxxxxxxxxx...xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.x.x.......xxxxxxxxxxxxxxxxxxxxxx...xxxxxxxxx.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx...xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx......xxxxxx.....xx..........xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.xxxxxxxxxxxxxxxx..x......xxxxxxxx..xxxxxxxxxxxxxxxxxxx.x...xxxx.x.x.x...xxxxx
pauldreik added 3 commits Sep 14, 2019
the code correctly finds the flaws in the old code,
if one temporarily restores doh.c to the old version.
@bagder

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.