[riot-notifications] [RIOT-OS/RIOT] sock_dns: fix out-of-bound errors (#10740)

Marian Buschsieweke notifications at github.com
Wed Jan 9 22:35:32 CET 2019


maribu commented on this pull request.

My comments in the code are not relevant to this PR. (They are minor enough that they could be addressed in this PR easily as well, but with the urgency of this PR it might be better to make a new PR for that. I could also propose such a PR, if you want.)

So back to this PR: I do approve with the code, but I cannot test as I'm missing the hardware.

> @@ -95,24 +103,27 @@ static int _parse_dns_reply(uint8_t *buf, size_t len, void* addr_out, int family
 

https://github.com/RIOT-OS/RIOT/blob/d99d72c583789bba3d8696274358af2d52d19ac9/sys/net/application_layer/dns/dns.c#L91-L93

Not related to this PR, but `buf` has an alignment requirement of 1 byte, while `sock_dns_hdr_t` has an alignment requirement of 2 bytes. Also `__attribute__((packed))` should be added in `sock_dns_hdr_t` when the memory layout of that `struct` has to be predictable. (Even though I'm 99.999% sure that without that attribute no padding bytes will be added for any currently supported platform I'm personally in favor of using the `packed` attribute whenever accessing memory as both byte array and structure, as e.g. future changes to the struct or later added platforms could have issues here.)

The parameter `buf` comes from here:
https://github.com/RIOT-OS/RIOT/blob/d99d72c583789bba3d8696274358af2d52d19ac9/sys/net/application_layer/dns/dns.c#L186-L190

And is allocated here:
https://github.com/RIOT-OS/RIOT/blob/d99d72c583789bba3d8696274358af2d52d19ac9/sys/net/application_layer/dns/dns.c#L133-L136

`SOCK_DNS_QUERYBUF_LEN` is currently 80 on ARM, so `reply_buf` luckily has the correct alignment as required by the cast. But this really seems to be pure luck rather then indention here.

------
Update 1: Added permalink to the code I'm referring to at the beginning of this comment, as GitHub chose to not show the code above this comment.

> @@ -73,17 +73,25 @@ static unsigned _get_short(uint8_t *buf)
     return _tmp;
 }
 
-static size_t _skip_hostname(uint8_t *buf)
+static int _skip_hostname(const uint8_t *buf, size_t len, uint8_t *bufpos)

Even though under the hood most likely `ssize_t` will be the same type as `int`, `ssize_t` makes it clearer what the intent is. Thus, +1

>          bufpos += 4;    /* skip type and class of query */
     }
 
     for (unsigned n = 0; n < ntohs(hdr->ancount); n++) {
-        bufpos += _skip_hostname(bufpos);
+        int tmp = _skip_hostname(buf, len, bufpos);
+        if (tmp < 0) {
+            return tmp;
+        }
+        bufpos += tmp;
         uint16_t _type = ntohs(_get_short(bufpos));

Again not related to the PR, but `ntohs` and `_get_short` are used quite often together that creating a new function like

```C
static inline uint16_t _get_short_nbo(uint8_t *buf) {
    return (((uint16_t)buf[0]) << 8) | buf[1];
}
```

would make the code more readable and will likely be faster that memcpy + ntohs.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/10740#pullrequestreview-190923775
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190109/1f95758d/attachment.html>


More information about the notifications mailing list