mirror of
https://github.com/moparisthebest/curl
synced 2024-08-13 17:03:50 -04:00
doh: fix (harmless) buffer overrun
Added unit test case 1655 to verify. Close #4352 the code correctly finds the flaws in the old code, if one temporarily restores doh.c to the old version.
This commit is contained in:
parent
5eb75d4186
commit
b766602729
17
lib/doh.c
17
lib/doh.c
@ -74,17 +74,26 @@ static const char *doh_strerror(DOHcode code)
|
||||
#define UNITTEST static
|
||||
#endif
|
||||
|
||||
/* @unittest 1655
|
||||
*/
|
||||
UNITTEST DOHcode doh_encode(const char *host,
|
||||
DNStype dnstype,
|
||||
unsigned char *dnsp, /* buffer */
|
||||
size_t len, /* buffer size */
|
||||
size_t *olen) /* output length */
|
||||
{
|
||||
size_t hostlen = strlen(host);
|
||||
const size_t hostlen = strlen(host);
|
||||
unsigned char *orig = dnsp;
|
||||
const char *hostp = host;
|
||||
|
||||
if(len < (12 + hostlen + 4))
|
||||
/* The expected output length does not depend on the number of dots within
|
||||
* the host name. It will always be two more than the length of the host
|
||||
* name, one for the size and one trailing null. In case there are dots,
|
||||
* each dot adds one size but removes the need to store the dot, net zero.
|
||||
*/
|
||||
const size_t expected_len = 12 + ( 1 + hostlen + 1) + 4;
|
||||
|
||||
if(len < expected_len)
|
||||
return DOH_TOO_SMALL_BUFFER;
|
||||
|
||||
*dnsp++ = 0; /* 16 bit id */
|
||||
@ -132,6 +141,10 @@ UNITTEST DOHcode doh_encode(const char *host,
|
||||
*dnsp++ = DNS_CLASS_IN; /* IN - "the Internet" */
|
||||
|
||||
*olen = dnsp - orig;
|
||||
|
||||
/* verify that our assumption of length is valid, since
|
||||
* this has lead to buffer overflows in this function */
|
||||
DEBUGASSERT(*olen == expected_len);
|
||||
return DOH_OK;
|
||||
}
|
||||
|
||||
|
@ -183,7 +183,7 @@ test1590 test1591 test1592 test1593 test1594 \
|
||||
test1600 test1601 test1602 test1603 test1604 test1605 test1606 test1607 \
|
||||
test1608 test1609 test1620 test1621 \
|
||||
\
|
||||
test1650 test1651 test1652 test1653 test1654 \
|
||||
test1650 test1651 test1652 test1653 test1654 test1655 \
|
||||
\
|
||||
test1700 test1701 test1702 \
|
||||
\
|
||||
|
26
tests/data/test1655
Normal file
26
tests/data/test1655
Normal file
@ -0,0 +1,26 @@
|
||||
<testcase>
|
||||
<info>
|
||||
<keywords>
|
||||
unittest
|
||||
doh
|
||||
</keywords>
|
||||
</info>
|
||||
|
||||
#
|
||||
# Client-side
|
||||
<client>
|
||||
<server>
|
||||
none
|
||||
</server>
|
||||
<features>
|
||||
unittest
|
||||
</features>
|
||||
<name>
|
||||
unit test for doh_encode
|
||||
</name>
|
||||
<tool>
|
||||
unit1655
|
||||
</tool>
|
||||
</client>
|
||||
|
||||
</testcase>
|
@ -22,6 +22,7 @@ set(UT_SRC
|
||||
# Broken link on Linux
|
||||
# unit1604.c
|
||||
unit1620.c
|
||||
unit1655.c
|
||||
)
|
||||
|
||||
set(UT_COMMON_FILES ../libtest/first.c ../libtest/test.h curlcheck.h)
|
||||
|
@ -11,7 +11,7 @@ UNITPROGS = unit1300 unit1301 unit1302 unit1303 unit1304 unit1305 unit1307 \
|
||||
unit1399 \
|
||||
unit1600 unit1601 unit1602 unit1603 unit1604 unit1605 unit1606 unit1607 \
|
||||
unit1608 unit1609 unit1620 unit1621 \
|
||||
unit1650 unit1651 unit1652 unit1653 unit1654
|
||||
unit1650 unit1651 unit1652 unit1653 unit1654 unit1655
|
||||
|
||||
unit1300_SOURCES = unit1300.c $(UNITFILES)
|
||||
unit1300_CPPFLAGS = $(AM_CPPFLAGS)
|
||||
@ -118,3 +118,7 @@ unit1653_CPPFLAGS = $(AM_CPPFLAGS)
|
||||
|
||||
unit1654_SOURCES = unit1654.c $(UNITFILES)
|
||||
unit1654_CPPFLAGS = $(AM_CPPFLAGS)
|
||||
|
||||
unit1655_SOURCES = unit1655.c $(UNITFILES)
|
||||
unit1655_CPPFLAGS = $(AM_CPPFLAGS)
|
||||
|
||||
|
@ -35,6 +35,9 @@ We put tests that focus on an area or a specific function into a single C
|
||||
source file. The source file should be named 'unitNNNN.c' where NNNN is a
|
||||
number that starts with 1300 and you can pick the next free number.
|
||||
|
||||
Add your test to tests/unit/Makefile.inc (if it is a unit test).
|
||||
Add your test data to tests/data/Makefile.inc
|
||||
|
||||
You also need a separate file called tests/data/testNNNN (using the same
|
||||
number) that describes your test case. See the test1300 file for inspiration
|
||||
and the tests/FILEFORMAT documentation.
|
||||
@ -46,9 +49,10 @@ For the actual C file, here's a very simple example:
|
||||
|
||||
#include "a libcurl header.h" /* from the lib dir */
|
||||
|
||||
static void unit_setup( void )
|
||||
static CURLcode unit_setup( void )
|
||||
{
|
||||
/* whatever you want done first */
|
||||
return CURLE_OK;
|
||||
}
|
||||
|
||||
static void unit_stop( void )
|
||||
|
110
tests/unit/unit1655.c
Normal file
110
tests/unit/unit1655.c
Normal file
@ -0,0 +1,110 @@
|
||||
/***************************************************************************
|
||||
* _ _ ____ _
|
||||
* Project ___| | | | _ \| |
|
||||
* / __| | | | |_) | |
|
||||
* | (__| |_| | _ <| |___
|
||||
* \___|\___/|_| \_\_____|
|
||||
*
|
||||
* Copyright (C) 2019, Daniel Stenberg, <daniel@haxx.se>, et al.
|
||||
*
|
||||
* This software is licensed as described in the file COPYING, which
|
||||
* you should have received as part of this distribution. The terms
|
||||
* are also available at https://curl.haxx.se/docs/copyright.html.
|
||||
*
|
||||
* You may opt to use, copy, modify, merge, publish, distribute and/or sell
|
||||
* copies of the Software, and permit persons to whom the Software is
|
||||
* furnished to do so, under the terms of the COPYING file.
|
||||
*
|
||||
* This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY
|
||||
* KIND, either express or implied.
|
||||
*
|
||||
***************************************************************************/
|
||||
#include "curlcheck.h"
|
||||
|
||||
#include "doh.h" /* from the lib dir */
|
||||
|
||||
static CURLcode unit_setup(void)
|
||||
{
|
||||
/* whatever you want done first */
|
||||
return CURLE_OK;
|
||||
}
|
||||
|
||||
static void unit_stop(void)
|
||||
{
|
||||
/* done before shutting down and exiting */
|
||||
}
|
||||
|
||||
UNITTEST_START
|
||||
|
||||
/* introduce a scope and prove the corner case with write overflow,
|
||||
* so we can prove this test would detect it and that it is properly fixed
|
||||
*/
|
||||
do {
|
||||
const char *bad = "this.is.a.hostname.where.each.individual.part.is.within."
|
||||
"the.sixtythree.character.limit.but.still.long.enough.to."
|
||||
"trigger.the.the.buffer.overflow......it.is.chosen.to.be."
|
||||
"of.a.length.such.that.it.causes.a.two.byte.buffer......."
|
||||
"overwrite.....making.it.longer.causes.doh.encode.to....."
|
||||
".return.early.so.dont.change.its.length.xxxx.xxxxxxxxxxx"
|
||||
"..xxxxxx.....xx..........xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
|
||||
"xxxxxxxxxxxxxxxxxxxxxxxxxx.xxxxxxxxxxxxxxxx..x......xxxx"
|
||||
"xxxx..xxxxxxxxxxxxxxxxxxx.x...xxxx.x.x.x...xxxxx";
|
||||
|
||||
/* plays the role of struct dnsprobe in urldata.h */
|
||||
struct demo {
|
||||
unsigned char dohbuffer[512];
|
||||
unsigned char canary1;
|
||||
unsigned char canary2;
|
||||
unsigned char canary3;
|
||||
};
|
||||
|
||||
size_t olen = 100000;
|
||||
struct demo victim;
|
||||
victim.canary1 = 87; /* magic numbers, arbritrarily picked */
|
||||
victim.canary2 = 35;
|
||||
victim.canary3 = 41;
|
||||
DOHcode d = doh_encode(bad, DNS_TYPE_A, victim.dohbuffer,
|
||||
sizeof(victim.dohbuffer), &olen);
|
||||
fail_unless(victim.canary1 == 87, "one byte buffer overwrite has happened");
|
||||
fail_unless(victim.canary2 == 35, "two byte buffer overwrite has happened");
|
||||
fail_unless(victim.canary3 == 41, "three byte buffer overwrite has happened");
|
||||
if(d == DOH_OK)
|
||||
{
|
||||
fail_unless(olen <= sizeof(victim.dohbuffer), "wrote outside bounds");
|
||||
fail_unless(olen > strlen(bad), "unrealistic low size");
|
||||
}
|
||||
} while(0);
|
||||
|
||||
/* run normal cases and try to trigger buffer length related errors */
|
||||
do {
|
||||
DNStype dnstype = DNS_TYPE_A;
|
||||
unsigned char buffer[128];
|
||||
const size_t buflen = sizeof(buffer);
|
||||
const size_t magic1 = 9765;
|
||||
size_t olen1 = magic1;
|
||||
const char *sunshine1 = "a.com";
|
||||
const char *sunshine2 = "aa.com";
|
||||
|
||||
DOHcode ret = doh_encode(sunshine1, dnstype, buffer, buflen, &olen1);
|
||||
fail_unless(ret == DOH_OK, "sunshine case 1 should pass fine");
|
||||
fail_if(olen1 == magic1, "olen has not been assigned properly");
|
||||
fail_unless(olen1 > strlen(sunshine1), "bad out length");
|
||||
|
||||
/* add one letter, the response should be one longer */
|
||||
size_t olen2 = magic1;
|
||||
DOHcode ret2 = doh_encode(sunshine2, dnstype, buffer, buflen, &olen2);
|
||||
fail_unless(ret2 == DOH_OK, "sunshine case 2 should pass fine");
|
||||
fail_if(olen2 == magic1, "olen has not been assigned properly");
|
||||
fail_unless(olen1 + 1 == olen2, "olen should grow with the hostname");
|
||||
|
||||
/* pass a short buffer, should fail */
|
||||
size_t olen;
|
||||
ret = doh_encode(sunshine1, dnstype, buffer, olen1 - 1, &olen);
|
||||
fail_if(ret == DOH_OK, "short buffer should have been noticed");
|
||||
|
||||
/* pass a minimum buffer, should succeed */
|
||||
ret = doh_encode(sunshine1, dnstype, buffer, olen1, &olen);
|
||||
fail_unless(ret == DOH_OK, "minimal length buffer should be long enough");
|
||||
fail_unless(olen == olen1, "bad buffer length");
|
||||
} while(0);
|
||||
UNITTEST_STOP
|
Loading…
Reference in New Issue
Block a user