Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: Domain name setting error and fix (Daniel Wagner)
2. Re: [PATCH 1/3] dnsproxy: Send a short response on error
(Daniel Wagner)
3. Re: [PATCH 1/3] dnsproxy: Send a short response on error
(Patrik Flykt)
4. Re: [PATCH 2/3] dnsproxy: Be more strict with incoming DNS
requests (Daniel Wagner)
5. Re: [PATCH 3/3] backtrace: Add checks for backtrace
consistency (Daniel Wagner)
6. Re: Domain name setting error and fix (Patrik Flykt)
7. Re: [PATCH 2/3] dnsproxy: Be more strict with incoming DNS
requests (Patrik Flykt)
8. Re: Domain name setting error and fix (Daniel Wagner)
9. [PATCH v2 2/2] dnsproxy: Be more strict with incoming DNS
requests (Patrik Flykt)
----------------------------------------------------------------------
Message: 1
Date: Fri, 25 Aug 2017 11:40:37 +0200
From: Daniel Wagner <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: Neven Sajko <[email protected]>, [email protected]
Subject: Re: Domain name setting error and fix
Message-ID: <[email protected]>
Content-Type: text/plain
Patrik Flykt <[email protected]> writes:
> On Thu, 2017-08-24 at 20:32 +0200, Daniel Wagner wrote:
>> @Patrik: you have added the capabilities initially:
>>
>> 36aaa77f88d0 ("connman.service: Restrict capabilities")
>>
>> Any strong feelings, arguments against removing the
>> CapabilityBoundingSet?
>
> CapabilityBoundingSet is preferred in order to keep some kind of lid on
> ConnMan, although CAP_SYS_ADMIN is reaching quite far anyway. Its not
> nice to add CAP_SYS_ADMIN, but there doesn't seem to be any other way
> to do it either. And now that I look at it, don't we fork when writing
> a backtrace, which was restricted by a capability I can't remember...
IIRC, there were same attempts to let ConnMan run as normal user but
there was no feedback on this attempt.
Anyway, from reading the LWN article on CAP_SYS_ADMIN [1] I would say we
don't have to worry trying to figure out which capabilities ConnMan needs
as soon we add CAP_SYS_ADMIN:
"""
To summarize: CAP_SYS_ADMIN has become the new root. If the goal of
capabilities is to limit the power of privileged programs to be less
than root, then once we give a program CAP_SYS_ADMIN the game is more or
less over. That is the manifest problem revealed from the above
analysis. However, if we look further, there is evidence of an
additional problem, one that lies in the Linux development model.
"""
Thanks,
Daniel
[1] https://lwn.net/Articles/486306/
------------------------------
Message: 2
Date: Fri, 25 Aug 2017 11:52:07 +0200
From: Daniel Wagner <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 1/3] dnsproxy: Send a short response on error
Message-ID: <[email protected]>
Content-Type: text/plain
Hi Patrik,
Patrik Flykt <[email protected]> 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
------------------------------
Message: 3
Date: Fri, 25 Aug 2017 13:18:03 +0300
From: Patrik Flykt <[email protected]>
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 1/3] dnsproxy: Send a short response on error
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
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
------------------------------
Message: 4
Date: Fri, 25 Aug 2017 12:23:59 +0200
From: Daniel Wagner <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 2/3] dnsproxy: Be more strict with incoming DNS
requests
Message-ID: <[email protected]>
Content-Type: text/plain
Hi Patrik,
Patrik Flykt <[email protected]> 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 :)
------------------------------
Message: 5
Date: Fri, 25 Aug 2017 12:24:51 +0200
From: Daniel Wagner <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 3/3] backtrace: Add checks for backtrace
consistency
Message-ID: <[email protected]>
Content-Type: text/plain
Hi Patrik,
Patrik Flykt <[email protected]> writes:
> If newlines cannot be found, stop processing the backtrace as
> something is wrong with it.
Patch applied.
Thanks,
Daniel
------------------------------
Message: 6
Date: Fri, 25 Aug 2017 13:25:31 +0300
From: Patrik Flykt <[email protected]>
To: Daniel Wagner <[email protected]>
Cc: Neven Sajko <[email protected]>, [email protected]
Subject: Re: Domain name setting error and fix
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Fri, 2017-08-25 at 11:40 +0200, Daniel Wagner wrote:
> IIRC, there were same attempts to let ConnMan run as normal user but
> there was no feedback on this attempt.
Never heard back from that, either.
> Anyway, from reading the LWN article on CAP_SYS_ADMIN [1] I would say
> we don't have to worry trying to figure out which capabilities
> ConnMan needs as soon we add CAP_SYS_ADMIN:
>
> """
> To summarize: CAP_SYS_ADMIN has become the new root. If the goal of
> capabilities is to limit the power of privileged programs to be less
> than root, then once we give a program CAP_SYS_ADMIN the game is more
> or less over. That is the manifest problem revealed from the above
> analysis. However, if we look further, there is evidence of an
> additional problem, one that lies in the Linux development model.
> """
Indeed... Do we need to set the domainname in the first place? Or
hostname? Right now I don't remember which issues setting the
domainname were supposed to solve. Setting the hostname probably fails
as well(?), but keeping the same hostname makes me feel at home
independent of network :-)
Cheers,
Patrik
------------------------------
Message: 7
Date: Fri, 25 Aug 2017 13:26:15 +0300
From: Patrik Flykt <[email protected]>
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 2/3] dnsproxy: Be more strict with incoming DNS
requests
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Fri, 2017-08-25 at 12:23 +0200, Daniel Wagner wrote:
> ps: you made me start reading those rfc :)
They're quite awful, I must say...
Patrik
------------------------------
Message: 8
Date: Fri, 25 Aug 2017 12:50:04 +0200
From: Daniel Wagner <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: Neven Sajko <[email protected]>, [email protected]
Subject: Re: Domain name setting error and fix
Message-ID: <[email protected]>
Content-Type: text/plain
Patrik Flykt <[email protected]> writes:
> On Fri, 2017-08-25 at 11:40 +0200, Daniel Wagner wrote:
>> """
>> To summarize: CAP_SYS_ADMIN has become the new root. If the goal of
>> capabilities is to limit the power of privileged programs to be less
>> than root, then once we give a program CAP_SYS_ADMIN the game is more
>> or less over. That is the manifest problem revealed from the above
>> analysis. However, if we look further, there is evidence of an
>> additional problem, one that lies in the Linux development model.
>> """
>
> Indeed... Do we need to set the domainname in the first place? Or
> hostname? Right now I don't remember which issues setting the
> domainname were supposed to solve. Setting the hostname probably fails
> as well(?), but keeping the same hostname makes me feel at home
> independent of network :-)
IIRC it was added to allow to configure embedded devices via DNS. I know
the feeling of hostname changing. Luckely there is
AllowHostnameUpdates=false :)
So do we agree on dropping the capabilities attempt?
------------------------------
Message: 9
Date: Fri, 25 Aug 2017 14:27:09 +0300
From: Patrik Flykt <[email protected]>
To: [email protected]
Subject: [PATCH v2 2/2] dnsproxy: Be more strict with incoming DNS
requests
Message-ID: <[email protected]>
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 | 67 +++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 41 insertions(+), 26 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index ef91a7cb..8b2827a5 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -79,6 +79,11 @@ struct domain_hdr {
#error "Unknown byte order"
#endif
+struct qtype_qclass {
+ uint16_t qtype;
+ uint16_t qclass;
+} __attribute__ ((packed));
+
struct partial_reply {
uint16_t len;
uint16_t received;
@@ -2918,26 +2923,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(*hdr) + sizeof(struct qtype_qclass) ||
+ 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);
@@ -2947,7 +2959,23 @@ static int parse_request(unsigned char *buf, int len,
uint8_t label_len = *ptr;
if (label_len == 0x00) {
- last_label = (char *) (ptr + 1);
+ uint8_t class;
+ struct qtype_qclass *q =
+ (struct qtype_qclass *)(ptr + 1);
+
+ if (remain < sizeof(*q)) {
+ DBG("Dropped malformed DNS query");
+ return -EINVAL;
+ }
+
+ class = ntohs(q->qclass);
+ if (class != 1 && class != 255) {
+ DBG("Dropped non-IN DNS class %d", class);
+ return -EINVAL;
+ }
+
+ ptr += sizeof(*q) + 1;
+ remain -= (sizeof(*q) + 1);
break;
}
@@ -2963,26 +2991,13 @@ 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)) {
- uint16_t edns0_bufsize;
+ if (arcount && remain >= sizeof(struct domain_rr) + 1 && !ptr[0] &&
+ ptr[1] == opt_edns0_type[0] && ptr[2] == opt_edns0_type[1]) {
+ struct domain_rr *edns0 = (struct domain_rr *)(ptr + 1);
- edns0_bufsize = last_label[7] << 8 | last_label[8];
-
- 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", ntohs(edns0->class));
+ } else if (!arcount && remain) {
+ DBG("DNS request with %d garbage bytes", remain);
}
debug("query %s", name);
--
2.11.0
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 22, Issue 20
***************************************
