[master] Corrected refcnt loss in option parsing

    Merges in 47140.

diff --git a/RELNOTES b/RELNOTES
index ca27e6b5a42ed5e347ab141b752c4d0c666d663e..e8d2a5c740da35f4e3a7eaa84103b56de6ba2ded 100644 (file)
--- a/RELNOTES
+++ b/RELNOTES
@@ -89,7 +89,7 @@ by Eric Young (eay@cryptsoft.com).
 
                 Changes since 4.4.0 (New Features)
 - none
-                Changes since 4.4.0 (Bugs)
+               Changes since 4.4.0 (Bug Fixes)
 
 - A delayed-ack value of 0 (the default), now correctly disables the delayed
   feature.  A change in 4.4.0 prohibited lease updates marking leases active
@@ -97,6 +97,12 @@ by Eric Young (eay@cryptsoft.com).
   caused servers to lose active lease assignments upon restart.
   [ISC-Bugs #47141]
 
+! Option reference count was not correctly decremented in error path
+  when parsing buffer for options. Reported by Felix Wilhelm, Google
+  Security Team.
+  [ISC-Bugs #47140]
+  CVE: CVE-2018-xxxx
+
                 Changes since 4.4.0b1 (New Features)
 
 - Duplicate address detection when binding to a new IPv6 address was added

diff --git a/common/options.c b/common/options.c
index 5044d4a1e9f003eb622d98a3bcc1a1eb6b9aee44..6f23bc1575229eb06864f76db751a767a2b651a2 100644 (file)
--- a/common/options.c
+++ b/common/options.c
@@ -3,7 +3,7 @@
    DHCP options parsing and reassembly. */
 
 /*
- * Copyright (c) 2004-2017 by Internet Systems Consortium, Inc. ("ISC")
+ * Copyright (c) 2004-2018 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 1995-2003 by Internet Software Consortium
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
@@ -177,6 +177,8 @@ int parse_option_buffer (options, buffer, length, universe)
 
                /* If the length is outrageous, the options are bad. */
                if (offset + len > length) {
+                       /* Avoid reference count overflow */
+                       option_dereference(&option, MDL);
                        reason = "option length exceeds option buffer length";
                      bogus:
                        log_error("parse_option_buffer: malformed option "

diff --git a/common/tests/Kyuafile b/common/tests/Kyuafile
index cb1e2cf5bc27c52a6d30e0f41905bb588f69cfff..f3c352dbdadca6e2c5d6af64c9e59a86d4b7ee91 100644 (file)
--- a/common/tests/Kyuafile
+++ b/common/tests/Kyuafile
@@ -5,3 +5,4 @@ atf_test_program{name='alloc_unittest'}
 atf_test_program{name='dns_unittest'}
 atf_test_program{name='misc_unittest'}
 atf_test_program{name='ns_name_unittest'}
+atf_test_program{name='option_unittest'}

diff --git a/common/tests/Makefile.am b/common/tests/Makefile.am
index 4129e4506f6f85bcc93d3e12f6077258032c0082..322f77e832425eb385221ea72b66579f37386199 100644 (file)
--- a/common/tests/Makefile.am
+++ b/common/tests/Makefile.am
@@ -8,7 +8,8 @@ ATF_TESTS =
 
 if HAVE_ATF
 
-ATF_TESTS += alloc_unittest dns_unittest misc_unittest ns_name_unittest
+ATF_TESTS += alloc_unittest dns_unittest misc_unittest ns_name_unittest \
+       option_unittest
 
 alloc_unittest_SOURCES = test_alloc.c $(top_srcdir)/tests/t_api_dhcp.c
 alloc_unittest_LDADD = $(ATF_LDFLAGS)
@@ -42,6 +43,14 @@ ns_name_unittest_LDADD += ../libdhcp.@A@ ../../omapip/libomapi.@A@ \
        @BINDLIBISCCFGDIR@/libisccfg.@A@  \
        @BINDLIBISCDIR@/libisc.@A@
 
+option_unittest_SOURCES = option_unittest.c $(top_srcdir)/tests/t_api_dhcp.c
+option_unittest_LDADD = $(ATF_LDFLAGS)
+option_unittest_LDADD += ../libdhcp.@A@ ../../omapip/libomapi.@A@ \
+       @BINDLIBIRSDIR@/libirs.@A@ \
+       @BINDLIBDNSDIR@/libdns.@A@ \
+       @BINDLIBISCCFGDIR@/libisccfg.@A@  \
+       @BINDLIBISCDIR@/libisc.@A@
+
 check: $(ATF_TESTS)
        @if test $(top_srcdir) != ${top_builddir}; then \
                cp $(top_srcdir)/common/tests/Atffile Atffile; \

diff --git a/common/tests/Makefile.in b/common/tests/Makefile.in
index 60b176f047d0122f3d3c8d3864453321c76d1d96..89ca98d72aef1e2e134a3b22dff4a931dfb6eec2 100644 (file)
--- a/common/tests/Makefile.in
+++ b/common/tests/Makefile.in
@@ -87,7 +87,9 @@ PRE_UNINSTALL = :
 POST_UNINSTALL = :
 build_triplet = @build@
 host_triplet = @host@
-@HAVE_ATF_TRUE@am__append_1 = alloc_unittest dns_unittest misc_unittest ns_name_unittest
+@HAVE_ATF_TRUE@am__append_1 = alloc_unittest dns_unittest misc_unittest ns_name_unittest \
+@HAVE_ATF_TRUE@        option_unittest
+
 check_PROGRAMS = $(am__EXEEXT_2)
 subdir = common/tests
 ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
@@ -101,7 +103,8 @@ CONFIG_CLEAN_FILES =
 CONFIG_CLEAN_VPATH_FILES =
 @HAVE_ATF_TRUE@am__EXEEXT_1 = alloc_unittest$(EXEEXT) \
 @HAVE_ATF_TRUE@        dns_unittest$(EXEEXT) misc_unittest$(EXEEXT) \
-@HAVE_ATF_TRUE@        ns_name_unittest$(EXEEXT)
+@HAVE_ATF_TRUE@        ns_name_unittest$(EXEEXT) \
+@HAVE_ATF_TRUE@        option_unittest$(EXEEXT)
 am__EXEEXT_2 = $(am__EXEEXT_1)
 am__alloc_unittest_SOURCES_DIST = test_alloc.c \
        $(top_srcdir)/tests/t_api_dhcp.c
@@ -132,6 +135,13 @@ am__ns_name_unittest_SOURCES_DIST = ns_name_test.c \
 ns_name_unittest_OBJECTS = $(am_ns_name_unittest_OBJECTS)
 @HAVE_ATF_TRUE@ns_name_unittest_DEPENDENCIES = $(am__DEPENDENCIES_1) \
 @HAVE_ATF_TRUE@        ../libdhcp.@A@ ../../omapip/libomapi.@A@
+am__option_unittest_SOURCES_DIST = option_unittest.c \
+       $(top_srcdir)/tests/t_api_dhcp.c
+@HAVE_ATF_TRUE@am_option_unittest_OBJECTS = option_unittest.$(OBJEXT) \
+@HAVE_ATF_TRUE@        t_api_dhcp.$(OBJEXT)
+option_unittest_OBJECTS = $(am_option_unittest_OBJECTS)
+@HAVE_ATF_TRUE@option_unittest_DEPENDENCIES = $(am__DEPENDENCIES_1) \
+@HAVE_ATF_TRUE@        ../libdhcp.@A@ ../../omapip/libomapi.@A@
 AM_V_P = $(am__v_P_@AM_V@)
 am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
 am__v_P_0 = false
@@ -165,11 +175,13 @@ am__v_CCLD_ = $(am__v_CCLD_@AM_DEFAULT_V@)
 am__v_CCLD_0 = @echo "  CCLD    " $@;
 am__v_CCLD_1 = 
 SOURCES = $(alloc_unittest_SOURCES) $(dns_unittest_SOURCES) \
-       $(misc_unittest_SOURCES) $(ns_name_unittest_SOURCES)
+       $(misc_unittest_SOURCES) $(ns_name_unittest_SOURCES) \
+       $(option_unittest_SOURCES)
 DIST_SOURCES = $(am__alloc_unittest_SOURCES_DIST) \
        $(am__dns_unittest_SOURCES_DIST) \
        $(am__misc_unittest_SOURCES_DIST) \
-       $(am__ns_name_unittest_SOURCES_DIST)
+       $(am__ns_name_unittest_SOURCES_DIST) \
+       $(am__option_unittest_SOURCES_DIST)
 RECURSIVE_TARGETS = all-recursive check-recursive cscopelist-recursive \
        ctags-recursive dvi-recursive html-recursive info-recursive \
        install-data-recursive install-dvi-recursive \
@@ -393,6 +405,13 @@ ATF_TESTS = $(am__append_1)
 @HAVE_ATF_TRUE@        @BINDLIBDNSDIR@/libdns.@A@ \
 @HAVE_ATF_TRUE@        @BINDLIBISCCFGDIR@/libisccfg.@A@ \
 @HAVE_ATF_TRUE@        @BINDLIBISCDIR@/libisc.@A@
+@HAVE_ATF_TRUE@option_unittest_SOURCES = option_unittest.c $(top_srcdir)/tests/t_api_dhcp.c
+@HAVE_ATF_TRUE@option_unittest_LDADD = $(ATF_LDFLAGS) ../libdhcp.@A@ \
+@HAVE_ATF_TRUE@        ../../omapip/libomapi.@A@ \
+@HAVE_ATF_TRUE@        @BINDLIBIRSDIR@/libirs.@A@ \
+@HAVE_ATF_TRUE@        @BINDLIBDNSDIR@/libdns.@A@ \
+@HAVE_ATF_TRUE@        @BINDLIBISCCFGDIR@/libisccfg.@A@ \
+@HAVE_ATF_TRUE@        @BINDLIBISCDIR@/libisc.@A@
 all: all-recursive
 
 .SUFFIXES:
@@ -446,6 +465,10 @@ ns_name_unittest$(EXEEXT): $(ns_name_unittest_OBJECTS) $(ns_name_unittest_DEPEND
        @rm -f ns_name_unittest$(EXEEXT)
        $(AM_V_CCLD)$(LINK) $(ns_name_unittest_OBJECTS) $(ns_name_unittest_LDADD) $(LIBS)
 
+option_unittest$(EXEEXT): $(option_unittest_OBJECTS) $(option_unittest_DEPENDENCIES) $(EXTRA_option_unittest_DEPENDENCIES) 
+       @rm -f option_unittest$(EXEEXT)
+       $(AM_V_CCLD)$(LINK) $(option_unittest_OBJECTS) $(option_unittest_LDADD) $(LIBS)
+
 mostlyclean-compile:
        -rm -f *.$(OBJEXT)
 
@@ -455,6 +478,7 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/dns_unittest.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/misc_unittest.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ns_name_test.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/option_unittest.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/t_api_dhcp.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/test_alloc.Po@am__quote@
 

diff --git a/common/tests/option_unittest.c b/common/tests/option_unittest.c
new file mode 100644 (file)
index 0000000..36236b8
--- /dev/null
+++ b/common/tests/option_unittest.c
@@ -0,0 +1,79 @@
+/*
+ * Copyright (C) 2018 Internet Systems Consortium, Inc. ("ISC")
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+ * REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+ * AND FITNESS.         IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+ * INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+ * LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+ * OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+ * PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <config.h>
+#include <atf-c.h>
+#include "dhcpd.h"
+
+ATF_TC(option_refcnt);
+
+ATF_TC_HEAD(option_refcnt, tc)
+{
+    atf_tc_set_md_var(tc, "descr",
+                     "Verify option reference count does not overflow.");
+}
+
+/* This test does a simple check to see if option reference count is
+ * decremented even an error path exiting parse_option_buffer()
+ */
+ATF_TC_BODY(option_refcnt, tc)
+{
+    struct option_state *options;
+    struct option *option;
+    unsigned code;
+    int refcnt;
+    unsigned char buffer[3] = { 15, 255, 0 };
+
+    initialize_common_option_spaces();
+
+    options = NULL;
+    if (!option_state_allocate(&options, MDL)) {
+       atf_tc_fail("can't allocate option state");
+    }
+    
+    option = NULL;
+    code = 15; /* domain-name */
+    if (!option_code_hash_lookup(&option, dhcp_universe.code_hash,
+                                &code, 0, MDL)) {
+       atf_tc_fail("can't find option 15");
+    }
+    if (option == NULL) {
+       atf_tc_fail("option is NULL");
+    }
+    refcnt = option->refcnt;
+
+    buffer[0] = 15;
+    buffer[1] = 255; /* invalid */
+    buffer[2] = 0;
+
+    if (parse_option_buffer(options, buffer, 3, &dhcp_universe)) {
+       atf_tc_fail("parse_option_buffer is expected to fail");
+    }
+
+    if (refcnt != option->refcnt) {
+       atf_tc_fail("refcnt changed from %d to %d", refcnt, option->refcnt);
+    }
+}
+
+/* This macro defines main() method that will call specified
+   test cases. tp and simple_test_case names can be whatever you want
+   as long as it is a valid variable identifier. */
+ATF_TP_ADD_TCS(tp)
+{
+    ATF_TP_ADD_TC(tp, option_refcnt);
+
+    return (atf_no_error());
+}