4:12 a.m.
Hi,
Here are a few fixes for stricter handling of DNS requests. If there
are some extra sections or more than one question in the request, the
request is dropped. Nowadays a properly formatted DNS request contains
exactly one question, it might have been different when the DNS
protocol was initially specified.
Also, if there are no newlines in the backtrace output, something
surprising has happened and processing cannot continue.
Cheers,
Patrik
Patrik Flykt (3):
dnsproxy: Send a short response on error
dnsproxy: Be more strict with incoming DNS requests
backtrace: Add checks for backtrace consistency
src/backtrace.c | 10 ++++++++
src/dnsproxy.c | 71 +++++++++++++++++++++++++++++++++++----------------------
2 files changed, 54 insertions(+), 27 deletions(-)
--
2.11.0
4:12 a.m.
New subject: [PATCH 1/3] dnsproxy: Send a short response on error
On error there is no need to send the whole packet back to the
client, only the basic headers informing of the error. Also check
the length of the buffer, including protocol offsets.
---
src/dnsproxy.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 40b4f159..e50f2740 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -470,7 +470,7 @@ static void send_cached_response(int sk, unsigned char *buf, int len,
err, len, dns_len);
}
-static void send_response(int sk, unsigned char *buf, int len,
+static void send_response(int sk, unsigned char *buf, size_t len,
const struct sockaddr *to, socklen_t tolen,
int protocol)
{
@@ -482,21 +482,27 @@ static void send_response(int sk, unsigned char *buf, int len,
if (offset < 0)
return;
- if (len < 12)
+ if (len < sizeof(struct domain_hdr) + offset)
return;
hdr = (void *) (buf + offset);
+ if (offset) {
+ buf[0] = 0;
+ buf[1] = 12;
+ }
debug("id 0x%04x qr %d opcode %d", hdr->id, hdr->qr, hdr->opcode);
hdr->qr = 1;
hdr->rcode = ns_r_servfail;
+ hdr->qdcount = 0;
hdr->ancount = 0;
hdr->nscount = 0;
hdr->arcount = 0;
- err = sendto(sk, buf, len, MSG_NOSIGNAL, to, tolen);
+ err = sendto(sk, buf, sizeof(struct domain_hdr) + offset, MSG_NOSIGNAL,
+ to, tolen);
if (err < 0) {
connman_error("Failed to send DNS response to %d: %s",
sk, strerror(errno));
--
2.11.0
4:52 a.m.
New subject: [PATCH 1/3] dnsproxy: Send a short response on error
Hi Patrik,
Patrik Flykt <patrik.flykt(a)linux.intel.com> writes:
On error there is no need to send the whole packet back to the
client, only the basic headers informing of the error. Also check
the length of the buffer, including protocol offsets.
---
src/dnsproxy.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 40b4f159..e50f2740 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -470,7 +470,7 @@ static void send_cached_response(int sk, unsigned char *buf, int
len,
err, len, dns_len);
}
-static void send_response(int sk, unsigned char *buf, int len,
+static void send_response(int sk, unsigned char *buf, size_t len,
const struct sockaddr *to, socklen_t tolen,
int protocol)
{
@@ -482,21 +482,27 @@ static void send_response(int sk, unsigned char *buf, int len,
if (offset < 0)
return;
- if (len < 12)
+ if (len < sizeof(struct domain_hdr) + offset)
return;
hdr = (void *) (buf + offset);
+ if (offset) {
+ buf[0] = 0;
+ buf[1] = 12;
+ }
I know it is almost a lost thing in this file, but could you add some
defines here for the values here?
The rest looks good.
Thanks,
Daniel
5:18 a.m.
New subject: [PATCH 1/3] dnsproxy: Send a short response on error
On Fri, 2017-08-25 at 11:52 +0200, Daniel Wagner wrote:
> hdr = (void *) (buf + offset);
> + if (offset) {
> + buf[0] = 0;
> + buf[1] = 12;
> + }
I know it is almost a lost thing in this file, but could you add some
defines here for the values here?
This is actually the length (12 bytes), so it should say sizeof(struct
domain_header)... I'll update.
Patrik
4:12 a.m.
New subject: [PATCH 2/3] dnsproxy: Be more strict with incoming DNS requests
Be more strict with incoming DNS requests. Verify that there is
only one entry in the question section and none in the answer or
name server sections.
Ensure that the question section has a proper IN class and
compute the remaing length and the pointer position with respect
to the end of the question section. If there is an EDNS0 extension,
log its length.
---
src/dnsproxy.c | 59 ++++++++++++++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 24 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index e50f2740..06c53b26 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -2919,26 +2919,33 @@ static struct connman_notifier dnsproxy_notifier = {
static unsigned char opt_edns0_type[2] = { 0x00, 0x29 };
-static int parse_request(unsigned char *buf, int len,
+static int parse_request(unsigned char *buf, size_t len,
char *name, unsigned int size)
{
struct domain_hdr *hdr = (void *) buf;
uint16_t qdcount = ntohs(hdr->qdcount);
+ uint16_t ancount = ntohs(hdr->ancount);
+ uint16_t nscount = ntohs(hdr->nscount);
uint16_t arcount = ntohs(hdr->arcount);
unsigned char *ptr;
- char *last_label = NULL;
unsigned int remain, used = 0;
- if (len < 12)
+ if (len < sizeof(struct domain_hdr) + 5 || hdr->qr ||
+ qdcount != 1 || ancount || nscount) {
+ DBG("Dropped DNS request qr %d with len %zd qdcount %d "
+ "ancount %d nscount %d", hdr->qr, len, qdcount, ancount,
+ nscount);
+
+ return -EINVAL;
+ }
+
+ if (!name || !size)
return -EINVAL;
debug("id 0x%04x qr %d opcode %d qdcount %d arcount %d",
hdr->id, hdr->qr, hdr->opcode,
qdcount, arcount);
- if (hdr->qr != 0 || qdcount != 1)
- return -EINVAL;
-
name[0] = '\0';
ptr = buf + sizeof(struct domain_hdr);
@@ -2948,7 +2955,22 @@ static int parse_request(unsigned char *buf, int len,
uint8_t label_len = *ptr;
if (label_len == 0x00) {
- last_label = (char *) (ptr + 1);
+ uint16_t class;
+
+ if (remain < 5) {
+ DBG("Dropped malformed DNS query");
+ return -EINVAL;
+ }
+
+ class = ptr[3] << 8 | ptr[4];
+ if (class != 1 && class != 255) {
+ DBG("Dropped non-IN DNS class %d", class);
+
+ return -EINVAL;
+ }
+
+ ptr += 5;
+ remain -= 5;
break;
}
@@ -2964,26 +2986,15 @@ static int parse_request(unsigned char *buf, int len,
remain -= label_len + 1;
}
- if (last_label && arcount && remain >= 9 && last_label[4] ==
0 &&
- !memcmp(last_label + 5, opt_edns0_type, 2)) {
+ if (arcount && remain >= 11 && !ptr[0] &&
+ ptr[1] == opt_edns0_type[0] && ptr[2] == opt_edns0_type[1]) {
uint16_t edns0_bufsize;
- edns0_bufsize = last_label[7] << 8 | last_label[8];
+ edns0_bufsize = ptr[3] << 8 | ptr[4];
- debug("EDNS0 buffer size %u", edns0_bufsize);
-
- /* This is an evil hack until full TCP support has been
- * implemented.
- *
- * Somtimes the EDNS0 request gets send with a too-small
- * buffer size. Since glibc doesn't seem to crash when it
- * gets a response biffer then it requested, just bump
- * the buffer size up to 4KiB.
- */
- if (edns0_bufsize < 0x1000) {
- last_label[7] = 0x10;
- last_label[8] = 0x00;
- }
+ DBG("EDNS0 buffer size %u", edns0_bufsize);
+ } else if (!arcount && remain) {
+ DBG("DNS request with %d garbage bytes", remain);
}
debug("query %s", name);
--
2.11.0
5:23 a.m.
New subject: [PATCH 2/3] dnsproxy: Be more strict with incoming DNS requests
Hi Patrik,
Patrik Flykt <patrik.flykt(a)linux.intel.com> writes:
Be more strict with incoming DNS requests. Verify that there is
only one entry in the question section and none in the answer or
name server sections.
Ensure that the question section has a proper IN class and
compute the remaing length and the pointer position with respect
to the end of the question section. If there is an EDNS0 extension,
log its length.
---
src/dnsproxy.c | 59 ++++++++++++++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 24 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index e50f2740..06c53b26 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -2919,26 +2919,33 @@ static struct connman_notifier dnsproxy_notifier = {
static unsigned char opt_edns0_type[2] = { 0x00, 0x29 };
-static int parse_request(unsigned char *buf, int len,
+static int parse_request(unsigned char *buf, size_t len,
char *name, unsigned int size)
{
struct domain_hdr *hdr = (void *) buf;
uint16_t qdcount = ntohs(hdr->qdcount);
+ uint16_t ancount = ntohs(hdr->ancount);
+ uint16_t nscount = ntohs(hdr->nscount);
uint16_t arcount = ntohs(hdr->arcount);
unsigned char *ptr;
- char *last_label = NULL;
unsigned int remain, used = 0;
- if (len < 12)
+ if (len < sizeof(struct domain_hdr) + 5 || hdr->qr ||
+ qdcount != 1 || ancount || nscount) {
+ DBG("Dropped DNS request qr %d with len %zd qdcount %d "
+ "ancount %d nscount %d", hdr->qr, len, qdcount, ancount,
+ nscount);
+
+ return -EINVAL;
+ }
+
+ if (!name || !size)
return -EINVAL;
debug("id 0x%04x qr %d opcode %d qdcount %d arcount %d",
hdr->id, hdr->qr, hdr->opcode,
qdcount, arcount);
- if (hdr->qr != 0 || qdcount != 1)
- return -EINVAL;
-
name[0] = '\0';
ptr = buf + sizeof(struct domain_hdr);
@@ -2948,7 +2955,22 @@ static int parse_request(unsigned char *buf, int len,
uint8_t label_len = *ptr;
if (label_len == 0x00) {
- last_label = (char *) (ptr + 1);
+ uint16_t class;
+
+ if (remain < 5) {
+ DBG("Dropped malformed DNS query");
+ return -EINVAL;
+ }
+
+ class = ptr[3] << 8 | ptr[4];
+ if (class != 1 && class != 255) {
+ DBG("Dropped non-IN DNS class %d", class);
+
+ return -EINVAL;
+ }
+
+ ptr += 5;
+ remain -= 5;
break;
I assume the '5' which appears 4 times is this the length for QTYPE and
QCLASS? Again, I would prefer to add defines for those constants. It
makes simpler to review.
}
@@ -2964,26 +2986,15 @@ static int parse_request(unsigned char *buf, int len,
remain -= label_len + 1;
}
- if (last_label && arcount && remain >= 9 && last_label[4] ==
0 &&
- !memcmp(last_label + 5, opt_edns0_type, 2)) {
+ if (arcount && remain >= 11 && !ptr[0] &&
+ ptr[1] == opt_edns0_type[0] && ptr[2] == opt_edns0_type[1]) {
uint16_t edns0_bufsize;
Is 'remain >= 11' the min length for an EDSN0? The size of
the fixed part of an OPT RR?
- edns0_bufsize = last_label[7] << 8 | last_label[8];
+ edns0_bufsize = ptr[3] << 8 | ptr[4];
- debug("EDNS0 buffer size %u", edns0_bufsize);
-
- /* This is an evil hack until full TCP support has been
- * implemented.
- *
- * Somtimes the EDNS0 request gets send with a too-small
- * buffer size. Since glibc doesn't seem to crash when it
- * gets a response biffer then it requested, just bump
- * the buffer size up to 4KiB.
- */
- if (edns0_bufsize < 0x1000) {
- last_label[7] = 0x10;
- last_label[8] = 0x00;
- }
+ DBG("EDNS0 buffer size %u", edns0_bufsize);
+ } else if (!arcount && remain) {
+ DBG("DNS request with %d garbage bytes", remain);
}
debug("query %s", name);
The rest looks good.
Thanks,
Daniel
ps: you made me start reading those rfc :)
4:12 a.m.
New subject: [PATCH 3/3] backtrace: Add checks for backtrace consistency
If newlines cannot be found, stop processing the backtrace as
something is wrong with it.
---
src/backtrace.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/backtrace.c b/src/backtrace.c
index 6a66c0ac..e8d7f432 100644
--- a/src/backtrace.c
+++ b/src/backtrace.c
@@ -111,6 +111,11 @@ void print_backtrace(const char* program_path, const char*
program_exec,
buf[len] = '\0';
pos = strchr(buf, '\n');
+ if (!pos) {
+ connman_error("Error in backtrace format");
+ break;
+ }
+
*pos++ = '\0';
if (strcmp(buf, "??") == 0) {
@@ -120,6 +125,11 @@ void print_backtrace(const char* program_path, const char*
program_exec,
}
ptr = strchr(pos, '\n');
+ if (!ptr) {
+ connman_error("Error in backtrace format");
+ break;
+ }
+
*ptr++ = '\0';
if (strncmp(pos, program_path, pathlen) == 0)
--
2.11.0
1252
days inactive
1252
days old
8 comments
2 participants
participants (2)
-
Daniel Wagner -
Patrik Flykt