sock_dns: fix out-of-bound errors by miri64 · Pull Request #10740 · RIOT-OS/RIOT

miri64 · 2019-01-09T18:51:25Z

Contribution description

Testing procedure

I compiled tests/gnrc_sock_dns with the following patch:

diff --git a/tests/gnrc_sock_dns/Makefile b/tests/gnrc_sock_dns/Makefile
index 4ef2c75..0b3824e 100644
--- a/tests/gnrc_sock_dns/Makefile
+++ b/tests/gnrc_sock_dns/Makefile
@@ -11,7 +11,11 @@ USEMODULE += sock_dns
 USEMODULE += gnrc_sock_udp
 USEMODULE += gnrc_ipv6_default
 USEMODULE += gnrc_ipv6_nib_dns
-USEMODULE += gnrc_netdev_default
+USEMODULE += ethos
+
+# ethos baudrate can be configured from make command
+ETHOS_BAUDRATE ?= 115200
+CFLAGS += -DETHOS_BAUDRATE=$(ETHOS_BAUDRATE) -DUSE_ETHOS_FOR_STDIO
 USEMODULE += auto_init_gnrc_netif
 
 USEMODULE += shell_commands
diff --git a/tests/gnrc_sock_dns/main.c b/tests/gnrc_sock_dns/main.c
index 52700fe..b8c2ad5 100644
--- a/tests/gnrc_sock_dns/main.c
+++ b/tests/gnrc_sock_dns/main.c
@@ -40,7 +40,7 @@ int main(void)
     uint8_t addr[16] = {0};
 
     puts("waiting for router advertisement...");
-    xtimer_usleep(1U*1000000);
+    xtimer_usleep(5U*1000000);
 
     /* print network addresses */
     puts("Configured network interfaces:");

flashed a samr21-xpro (alternatively pba-d-01-kw2x as described in #10739 is also possible), and connected an ethos terminal to the node

sudo ./dist/tools/ethos/start_network.sh /dev/ttyACM0 tap0 affe:abe::/48

I reset the node to get its link-local address for later.

Then I started a RADVD with the following config:

interface tap0 {
        AdvSendAdvert on;
        MinRtrAdvInterval 3;
        MaxRtrAdvInterval 10;
        prefix affe:abe::/64 {
                AdvOnLink off;
                AdvAutonomous on;
                AdvRouterAddr on;
        };
        RDNSS <a global address on your host> {
            AdvRDNSSLifetime 60;
        };
};

and reconfigured the correct route to the host (the preconfigured route by start_network.sh does not work, since uhcpc is not available on the node):

sudo ip route del affe:abe::/64
sudo ip route add affe:abe::/64 via "<link-local address of node>" dev tap0

Then I start the exploit script described in #10739 (or provided by https://github.com/beduino-project/exploit-riot-dns) and reset the node again to start the test. Without this PR the node will crash (note that the exploit described in #10739 does not work but only crashes the node since due to the usage of ethos the binary is different), withit it will just report error resolving example.org.

I also tested expected operation as described in the application's README.

Issues/PRs references

Fixes #10739.

miri64 · 2019-01-09T18:55:39Z

Fixed some typos and unrelated changes that snuck in. Should be good for review now.

sys/net/application_layer/dns/dns.c

kaspar030 · 2019-01-09T21:07:27Z

How I sometimes wish C had exceptions.

miri64 · 2019-01-09T21:13:27Z

How I sometimes wish C had exceptions.

Those don't make your life any better and bloat the code (even Rust does something else)

maribu

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.

sys/net/application_layer/dns/dns.c

miri64 · 2019-01-09T21:14:32Z

Addressed newest comments

miri64 · 2019-01-09T21:52:25Z

Makefile.dep


 ifneq (,$(filter sock_dns,$(USEMODULE)))
  USEMODULE += sock_util
+  USEMODULE += posix


Though this dependency is also pulled in by sock_util, I think it is cleaner to add this dependency due to https://github.com/RIOT-OS/RIOT/pull/10740/files#diff-0f2b4bcad3760716a76eab7708ea0e28R18 (it just adds the posix include path).

miri64 · 2019-01-09T22:00:38Z

@kaspar030 backport to 2018.10?

kaspar030 · 2019-01-09T22:07:03Z

@kaspar030 backport to 2018.10?

Yeah, I seem to remember that we intent to provide security fixes for the current release?

sys/net/application_layer/dns/dns.c

pyropeter · 2019-01-09T22:12:35Z

sys/net/application_layer/dns/dns.c

+             * of loop */
            continue;
        }
+        if ((addrlen != INADDRSZ) || (addrlen != IN6ADDRSZ)) {


This is always true. Should be &&.

Nope, see newest version ;-)

I see, that is even better.

Ah wait, that doesn't work with AF_UNSPEC..

kaspar030 · 2019-01-09T23:03:45Z

sys/net/application_layer/dns/dns.c

                ! ((_type == DNS_TYPE_A) || ((_type == DNS_TYPE_AAAA))
                    )) {
-            if (((bufpos - pos) + addrlen) < (bufpos - pos)) {
+            if (addrlen < len) {


The comparison needs to be the other way around.

Need to go to bed -.-... Fixed.

kaspar030 · 2019-01-09T23:05:07Z

sys/net/application_layer/dns/dns.c

                ((_type == DNS_TYPE_AAAA) && (family == AF_INET)) ||
                ! ((_type == DNS_TYPE_A) || ((_type == DNS_TYPE_AAAA))
                    )) {
+            if (addrlen < len) {


Suggested change

if (addrlen < len) {

if (addrlen > len) {

kaspar030 · 2019-01-10T09:45:03Z

I think this can be squashed?

miri64 · 2019-01-10T09:52:56Z

Squashed on top of original merge base.

miri64 · 2019-01-10T10:30:42Z

Fixed and squashed whitespace error reported by Murdock

miri64 · 2019-01-10T10:31:01Z

(here is the diff https://github.com/RIOT-OS/RIOT/compare/b1de000aef472e9508c0dd6b7a00ec9cd4124a34..1a61b8aadf18d437bb85660506aeb104313e79d0)

miri64 · 2019-01-10T11:01:40Z

@riot-ci is happy. @nmeum @pyropeter @kaspar030 @maribu are you?

maribu

ACK from my side :-)

maribu

Sorry, one thing just found :-/

sys/net/application_layer/dns/dns.c

kaspar030 · 2019-01-10T12:08:06Z

I just tried the test on native, it returns "error resolving example.org".

dnsmasq seems to have gotten the request:

[kaspar@ng riot (minimize)]$ sudo dnsmasq -d -2 -z -i riot0 -q --no-resolv \                                                                 
>         --dhcp-range=::1,constructor:riot0,ra-only \
>         --listen-address 2001:db8::1 \
>         --host-record=example.org,10.0.0.1,2001:db8::1
dnsmasq: started, version 2.80 cachesize 150
dnsmasq: compile time options: IPv6 GNU-getopt DBus i18n IDN2 DHCP DHCPv6 no-Lua TFTP conntrack ipset auth DNSSEC loop-detect inotify dumpfile
dnsmasq: warning: no upstream servers configured
dnsmasq-dhcp: router advertisement on riot0
dnsmasq-dhcp: router advertisement on 2001:db8::, constructed for riot0                                                                      
dnsmasq-dhcp: RTR-ADVERT(riot0) 2001:db8::
dnsmasq: read /etc/hosts - 0 addresses
dnsmasq-dhcp: RTR-SOLICIT(riot0)
dnsmasq-dhcp: RTR-ADVERT(riot0) 2001:db8::
dnsmasq: config example.org is 2001:db8::1
dnsmasq: config example.org is 10.0.0.1
dnsmasq: config example.org is 2001:db8::1
dnsmasq: config example.org is 10.0.0.1

kaspar030 · 2019-01-10T12:11:08Z

dns.c line 124 returns -EMULTIHOP

kaspar030 · 2019-01-10T12:15:38Z

dns.c line 124 returns -EMULTIHOP

sorry, -EBADMSG

kaspar030 · 2019-01-10T13:01:50Z

sys/net/application_layer/dns/dns.c

+        bufpos += RR_TTL_LENGTH; /* skip ttl */

        unsigned addrlen = ntohs(_get_short(bufpos));
-        bufpos += 2;


this got dropped

miri64 · 2019-01-10T13:10:58Z

Nope, it got moved to line 158. However, I found the bug (the return in l 101 is wrong as it does not return the length of the hostname, but the offset of the end of the hostname).

…

miri64 · 2019-01-10T13:13:04Z

I just tried the test on native, it returns "error resolving example.org".

Please try again with 4c507e9

kaspar030 · 2019-01-10T17:02:42Z

Please try again with 4c507e9

works now. pls squash!

Fixes RIOT-OS#10739

miri64 · 2019-01-10T17:09:21Z

Squashed. Diff of force push should show no difference ;)

kaspar030

ACK.

kaspar030 · 2019-01-10T17:32:54Z

Thanks everyone involved, and thanks @miri64 for dealing with the mess I created!

Lessons I learned:

sure it's fun to quickly hack together something like a DNS client. But just because it is working doesn't mean that it is safe. always double check when sharing it .
RIOT's review process needs a security tag. The buffer overflows in this code were so obvious...
even community members don't know about security@riot-os.org. Maybe we should add a big(er) note in the issue template?

miri64 · 2019-01-10T18:30:42Z

Thanks everyone involved, and thanks @miri64 for dealing with the mess I created!

Welp, I merged the mess IIRC, so I was as obligated as you ;-).

miri64 · 2019-01-10T18:33:36Z

sure it's fun to quickly hack together something like a DNS client. But just because it is working doesn't mean that it is safe. always double check when sharing it .

Failure tests, failure tests, failure tests. If I find some time tomorrow I will provide a PR for that (in a similar way as #10382 and #10392) before I tackle @maribu's suggestions in #10740 (review).

miri64 · 2019-01-11T18:43:48Z

Backport provided in #10757

miri64 · 2019-01-11T18:43:57Z

(forgot about that)

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 9, 2019

miri64 requested a review from kaspar030 January 9, 2019 18:51

miri64 mentioned this pull request Jan 9, 2019

sock_dns: Security issues (including remote code execution) #10739

Closed

miri64 force-pushed the sock_dns/bug/i10739 branch 2 times, most recently from b7a6187 to c05257e Compare January 9, 2019 18:55

pyropeter reviewed Jan 9, 2019

View reviewed changes

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved

sys/net/application_layer/dns/dns.c Show resolved Hide resolved

pyropeter reviewed Jan 9, 2019

View reviewed changes

sys/net/application_layer/dns/dns.c Show resolved Hide resolved

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved

kaspar030 reviewed Jan 9, 2019

View reviewed changes

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved

kaspar030 reviewed Jan 9, 2019

View reviewed changes

sys/net/application_layer/dns/dns.c Show resolved Hide resolved

kaspar030 reviewed Jan 9, 2019

View reviewed changes

sys/net/application_layer/dns/dns.c Show resolved Hide resolved

maribu reviewed Jan 9, 2019

View reviewed changes

sys/net/application_layer/dns/dns.c Show resolved Hide resolved

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved

sys/net/application_layer/dns/dns.c Show resolved Hide resolved

miri64 commented Jan 9, 2019

View reviewed changes

kaspar030 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jan 9, 2019

pyropeter reviewed Jan 9, 2019

View reviewed changes

kaspar030 reviewed Jan 9, 2019

View reviewed changes

miri64 force-pushed the sock_dns/bug/i10739 branch from 42d7cf5 to b1de000 Compare January 10, 2019 09:52

miri64 force-pushed the sock_dns/bug/i10739 branch from b1de000 to 1a61b8a Compare January 10, 2019 10:30

maribu approved these changes Jan 10, 2019

View reviewed changes

maribu reviewed Jan 10, 2019

View reviewed changes

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved

miri64 force-pushed the sock_dns/bug/i10739 branch from 1a61b8a to 2c6af6e Compare January 10, 2019 11:39

kaspar030 reviewed Jan 10, 2019

View reviewed changes

maribu mentioned this pull request Jan 10, 2019

makefiles: Add -fwrapv to CFLAGS #10748

Merged

miri64 and others added 3 commits January 10, 2019 18:07

sock_dns: fix out-of-bound errors

2840b38

Fixes RIOT-OS#10739

sock_dns: remove some magic numbers

8ad5e44

sock_dns: remove out-of-place puts() call

894ad29

miri64 force-pushed the sock_dns/bug/i10739 branch from 4c507e9 to 894ad29 Compare January 10, 2019 17:07

kaspar030 approved these changes Jan 10, 2019

View reviewed changes

kaspar030 merged commit 11e4309 into RIOT-OS:master Jan 10, 2019

miri64 deleted the sock_dns/bug/i10739 branch January 10, 2019 17:38

aabadie added this to the Release 2019.01 milestone Jan 11, 2019

miri64 mentioned this pull request Jan 11, 2019

sock_dns: fix out-of-bound errors [backport 2018.10] #10757

Merged

This was referenced Jan 29, 2019

sock_dns: correctly report too short messages #10896

Merged

sock_dns: correctly report too short messages [backport 2019.01] #10900

Merged

nmeum mentioned this pull request Oct 30, 2020

sock_dns: Fix incorrect buffer bounds check #15345

Merged

Conversation

miri64 commented Jan 9, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Jan 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaspar030 commented Jan 9, 2019

Uh oh!

miri64 commented Jan 9, 2019

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miri64 commented Jan 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 9, 2019

Uh oh!

kaspar030 commented Jan 9, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Jan 10, 2019

Uh oh!

miri64 commented Jan 10, 2019

Uh oh!

miri64 commented Jan 10, 2019

Uh oh!

miri64 commented Jan 10, 2019

Uh oh!

miri64 commented Jan 10, 2019

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaspar030 commented Jan 10, 2019

Uh oh!

kaspar030 commented Jan 10, 2019

Uh oh!

kaspar030 commented Jan 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 10, 2019 via email

Uh oh!

miri64 commented Jan 10, 2019 via email

Uh oh!

kaspar030 commented Jan 10, 2019

Uh oh!

miri64 commented Jan 10, 2019