[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[gnurl] 19/222: doh: fix (harmless) buffer overrun
From: |
gnunet |
Subject: |
[gnurl] 19/222: doh: fix (harmless) buffer overrun |
Date: |
Thu, 07 Nov 2019 00:08:35 +0100 |
This is an automated email from the git hooks/post-receive script.
ng0 pushed a commit to branch master
in repository gnurl.
commit b7666027296a4f89a8ce6b22af335e8aee7a7782
Author: Paul Dreik <address@hidden>
AuthorDate: Sat Sep 14 03:16:09 2019 +0200
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.
---
lib/doh.c | 17 ++++++-
tests/data/Makefile.inc | 2 +-
tests/data/test1655 | 26 +++++++++++
tests/unit/CMakeLists.txt | 1 +
tests/unit/Makefile.inc | 6 ++-
tests/unit/README | 6 ++-
tests/unit/unit1655.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
7 files changed, 163 insertions(+), 5 deletions(-)
diff --git a/lib/doh.c b/lib/doh.c
index 6d1f3303b..e84c9b0ad 100644
--- a/lib/doh.c
+++ b/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;
}
diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc
index 3733bd6b9..a3ce5a965 100644
--- a/tests/data/Makefile.inc
+++ b/tests/data/Makefile.inc
@@ -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 \
\
diff --git a/tests/data/test1655 b/tests/data/test1655
new file mode 100644
index 000000000..0c10bedf4
--- /dev/null
+++ b/tests/data/test1655
@@ -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>
diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt
index 4b0cec4a8..bc0699231 100644
--- a/tests/unit/CMakeLists.txt
+++ b/tests/unit/CMakeLists.txt
@@ -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)
diff --git a/tests/unit/Makefile.inc b/tests/unit/Makefile.inc
index f3cba1c2a..45278ba81 100644
--- a/tests/unit/Makefile.inc
+++ b/tests/unit/Makefile.inc
@@ -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)
+
diff --git a/tests/unit/README b/tests/unit/README
index b8a513b3b..060b670c6 100644
--- a/tests/unit/README
+++ b/tests/unit/README
@@ -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 )
diff --git a/tests/unit/unit1655.c b/tests/unit/unit1655.c
new file mode 100644
index 000000000..60f43d7d6
--- /dev/null
+++ b/tests/unit/unit1655.c
@@ -0,0 +1,110 @@
+/***************************************************************************
+ * _ _ ____ _
+ * Project ___| | | | _ \| |
+ * / __| | | | |_) | |
+ * | (__| |_| | _ <| |___
+ * \___|\___/|_| \_\_____|
+ *
+ * Copyright (C) 2019, Daniel Stenberg, <address@hidden>, 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
--
To stop receiving notification emails like this one, please contact
address@hidden.
- [gnurl] 13/222: FTP: skip CWD to entry dir when target is absolute, (continued)
- [gnurl] 13/222: FTP: skip CWD to entry dir when target is absolute, gnunet, 2019/11/06
- [gnurl] 10/222: curl:file2string: load large files much faster, gnunet, 2019/11/06
- [gnurl] 09/222: openssl: close_notify on the FTP data connection doesn't mean closure, gnunet, 2019/11/06
- [gnurl] 15/222: appveyor: add a winbuild, gnunet, 2019/11/06
- [gnurl] 23/222: smb: check for full size message before reading message details, gnunet, 2019/11/06
- [gnurl] 17/222: docs: fix typo in CURLOPT_HTTP_VERSION man, gnunet, 2019/11/06
- [gnurl] 18/222: docs: remove trailing ':' from section names in CURLOPT_TRAILER* man, gnunet, 2019/11/06
- [gnurl] 25/222: doh: clean up dangling DOH handles and memory on easy close, gnunet, 2019/11/06
- [gnurl] 14/222: FTP: allow "rubbish" prepended to the SIZE response, gnunet, 2019/11/06
- [gnurl] 22/222: quiche: persist connection details, gnunet, 2019/11/06
- [gnurl] 19/222: doh: fix (harmless) buffer overrun,
gnunet <=
- [gnurl] 24/222: unit1655: make it C90 compliant, gnunet, 2019/11/06
- [gnurl] 21/222: openssl: fix warning with boringssl and SSL_CTX_set_min_proto_version, gnunet, 2019/11/06
- [gnurl] 16/222: CI: inintial github action job, gnunet, 2019/11/06
- [gnurl] 20/222: doh: fix undefined behaviour and open up for gcc and clang optimization, gnunet, 2019/11/06
- [gnurl] 26/222: http2: relax verification of :authority in push promise requests, gnunet, 2019/11/06
- [gnurl] 27/222: url: cleanup dangling DOH request headers too, gnunet, 2019/11/06
- [gnurl] 28/222: mime: when disabled, avoid C99 macro, gnunet, 2019/11/06
- [gnurl] 32/222: url: only reuse TLS connections with matching pinning, gnunet, 2019/11/06
- [gnurl] 39/222: ftp: Expression 'ftpc->wait_data_conn' is always true, gnunet, 2019/11/06
- [gnurl] 46/222: easy: part of conditional expression is always true: !result, gnunet, 2019/11/06