Fun with buffer overrruns.
In get_option(): don't read past the end of the option buffer.
Also add a small unittest to verify sane behaviour for the above.
The dhcpcd code is not easily refactored into a library, nor is it
entirely possible to include some header files directly since some
structures use C++ reserved keywords ("new") for variable names.
In print_option(): use of snprintf() returns the length that
/would/ have been written. Add checks that the output buffer
is not overrun when printing.
Bug: 18356137
Bug: 18356135
Change-Id: I0f907b8a952208749226ba034a416d773e068f8a
diff --git a/Android.mk b/Android.mk
index 6b55f0d..216ee3a 100644
--- a/Android.mk
+++ b/Android.mk
@@ -49,3 +49,11 @@
LOCAL_MODULE_PATH := $(hooks_target)
LOCAL_SRC_FILES := $(hooks_dir)/$(LOCAL_MODULE)
include $(BUILD_PREBUILT)
+
+# Unit tests.
+include $(CLEAR_VARS)
+LOCAL_MODULE := dhcpcd_test
+LOCAL_CFLAGS := -Wall -Werror -Wunused-parameter
+LOCAL_SRC_FILES := dhcpcd_test.cpp dhcp.c common.c
+LOCAL_MODULE_TAGS := eng tests
+include $(BUILD_NATIVE_TEST)
diff --git a/dhcp.c b/dhcp.c
index 53f4795..0a1d220 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -313,7 +313,11 @@
const uint8_t *op = NULL;
int bl = 0;
- while (p < e) {
+ /* DHCP Options are in TLV format with T and L each being a single
+ * byte. In general, here we have p -> T, ol=p+1 -> L, op -> V.
+ * We must make sure there is enough room to read both T and L.
+ */
+ while (p + 1 < e) {
o = *p++;
if (o == opt) {
if (op) {
@@ -328,7 +332,7 @@
memcpy(bp, op, ol);
bp += ol;
}
- ol = *p;
+ ol = (p + *p < e) ? *p : e - (p + 1);
op = p + 1;
bl += ol;
}
@@ -1362,6 +1366,10 @@
data += sizeof(addr.s_addr);
} else
l = 0;
+ if (len <= l) {
+ bytes += len;
+ break;
+ }
len -= l;
bytes += l;
s += l;
diff --git a/dhcpcd_test.cpp b/dhcpcd_test.cpp
new file mode 100644
index 0000000..1fa829f
--- /dev/null
+++ b/dhcpcd_test.cpp
@@ -0,0 +1,146 @@
+/*
+ * Copyright 2014 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * dhcpcd_test.cpp - unit tests for dhcpcd
+ */
+
+#include <stdint.h>
+#include <stdlib.h>
+#include <string>
+
+#include <gtest/gtest.h>
+
+// For convenience.
+#define ARRAYSIZE(x) sizeof((x)) / sizeof((x)[0])
+
+// Regrettably, copy these defines and the dhcp_message structure in from
+// dhcp.h. This header file is not easily included, since subsequent
+// includes use C++ reserved keywords (like "new") as structure member names.
+extern "C" {
+
+#define DHO_PAD 0
+#define DHO_DNSDOMAIN 15
+
+/* Max MTU - defines dhcp option length */
+#define MTU_MAX 1500
+
+/* Sizes for DHCP options */
+#define DHCP_CHADDR_LEN 16
+#define SERVERNAME_LEN 64
+#define BOOTFILE_LEN 128
+#define DHCP_UDP_LEN (14 + 20 + 8)
+#define DHCP_FIXED_LEN (DHCP_UDP_LEN + 226)
+#define DHCP_OPTION_LEN (MTU_MAX - DHCP_FIXED_LEN)
+
+/* Some crappy DHCP servers require the BOOTP minimum length */
+#define BOOTP_MESSAGE_LENTH_MIN 300
+
+struct dhcp_message {
+ uint8_t op; /* message type */
+ uint8_t hwtype; /* hardware address type */
+ uint8_t hwlen; /* hardware address length */
+ uint8_t hwopcount; /* should be zero in client message */
+ uint32_t xid; /* transaction id */
+ uint16_t secs; /* elapsed time in sec. from boot */
+ uint16_t flags;
+ uint32_t ciaddr; /* (previously allocated) client IP */
+ uint32_t yiaddr; /* 'your' client IP address */
+ uint32_t siaddr; /* should be zero in client's messages */
+ uint32_t giaddr; /* should be zero in client's messages */
+ uint8_t chaddr[DHCP_CHADDR_LEN]; /* client's hardware address */
+ uint8_t servername[SERVERNAME_LEN]; /* server host name */
+ uint8_t bootfile[BOOTFILE_LEN]; /* boot file name */
+ uint32_t cookie;
+ uint8_t options[DHCP_OPTION_LEN]; /* message options - cookie */
+} _packed;
+
+char * get_option_string(const struct dhcp_message *dhcp, uint8_t option);
+
+}
+
+
+static const char kOptionString[] = "hostname";
+
+class DhcpcdGetOptionTest : public ::testing::Test {
+ protected:
+ virtual void SetUp() {
+ memset(dhcpmsgs, 0, ARRAYSIZE(dhcpmsgs) * sizeof(struct dhcp_message));
+ // Technically redundant.
+ memset(&(dhcpmsgs[0].options), DHO_PAD, sizeof(dhcpmsgs[0].options));
+
+ type_index = 0;
+ length_index = 0;
+ value_index = 0;
+ }
+
+ void PopulateTLV() {
+ // May very well write off the end of the first struct dhcp_message,
+ // by design.
+ length_index = type_index + 1;
+ value_index = length_index + 1;
+ dhcpmsgs[0].options[type_index] = DHO_DNSDOMAIN;
+ dhcpmsgs[0].options[length_index] = strlen(kOptionString);
+ memcpy(&(dhcpmsgs[0].options[value_index]),
+ kOptionString, strlen(kOptionString));
+ }
+
+ struct dhcp_message dhcpmsgs[2];
+ size_t type_index;
+ size_t length_index;
+ size_t value_index;
+};
+
+TEST_F(DhcpcdGetOptionTest, OptionNotPresent) {
+ // An entire option block of padding (all zeros).
+ EXPECT_EQ(NULL, ::get_option_string(&(dhcpmsgs[0]), DHO_DNSDOMAIN));
+}
+
+TEST_F(DhcpcdGetOptionTest, TypeIsOffTheEnd) {
+ type_index = sizeof(dhcpmsgs[0].options);
+ PopulateTLV();
+ EXPECT_EQ(NULL, ::get_option_string(&(dhcpmsgs[0]), DHO_DNSDOMAIN));
+}
+
+TEST_F(DhcpcdGetOptionTest, LengthIsOffTheEnd) {
+ type_index = sizeof(dhcpmsgs[0].options) - 1;
+ PopulateTLV();
+ EXPECT_EQ(NULL, ::get_option_string(&(dhcpmsgs[0]), DHO_DNSDOMAIN));
+}
+
+TEST_F(DhcpcdGetOptionTest, ValueIsOffTheEnd) {
+ type_index = sizeof(dhcpmsgs[0].options) - 2;
+ PopulateTLV();
+ EXPECT_EQ(NULL, ::get_option_string(&(dhcpmsgs[0]), DHO_DNSDOMAIN));
+}
+
+TEST_F(DhcpcdGetOptionTest, InsufficientSpaceForValue) {
+ type_index = sizeof(dhcpmsgs[0].options) - 6;
+ PopulateTLV();
+ char* value = ::get_option_string(&(dhcpmsgs[0]), DHO_DNSDOMAIN);
+ EXPECT_TRUE(NULL != value);
+ EXPECT_EQ("host", ::std::string(value));
+ free(value);
+}
+
+TEST_F(DhcpcdGetOptionTest, InsufficientSpaceForContinuedValue) {
+ type_index = sizeof(dhcpmsgs[0].options) - 16;
+ PopulateTLV();
+ type_index = sizeof(dhcpmsgs[0].options) - 6;
+ PopulateTLV();
+ char* value = ::get_option_string(&(dhcpmsgs[0]), DHO_DNSDOMAIN);
+ EXPECT_TRUE(NULL != value);
+ EXPECT_EQ("hostnamehost", ::std::string(value));
+ free(value);
+}
