Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jan 29, 2019
Merged

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

merged 3 commits into from
Jan 29, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jan 11, 2019

Backport of #10740

Contribution description

Fixes #10739

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 miri64 added Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jan 11, 2019
@miri64 miri64 requested a review from kaspar030 January 11, 2019 18:43
@miri64 miri64 mentioned this pull request Jan 11, 2019
Merged
@miri64 miri64 requested review from jia200x and maribu January 11, 2019 18:44
@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jan 12, 2019
@maribu
Copy link
Member

maribu commented Jan 12, 2019

The code is identical to #10740, so only testing should be required.

@kaspar030
Copy link
Contributor

tests/gnrc_sock_dns still working. ACK.

kaspar030
kaspar030 previously approved these changes Jan 12, 2019
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

@kaspar030
Copy link
Contributor

We have a problem, though: the release branch doesn't build with our current docker image, thus CI doesn't pass.

@kaspar030 kaspar030 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 12, 2019
@miri64
Copy link
Member Author

miri64 commented Jan 12, 2019

Dafuq?

@kaspar030
Copy link
Contributor

Dafuq?

Well, that's how interpret the unrelated Murdock errors.

This was referenced Jan 12, 2019
Closed
Merged
@miri64
Copy link
Member Author

miri64 commented Jan 14, 2019

#10759 was merged. Please rebase.

@jia200x jia200x mentioned this pull request Jan 14, 2019
Merged
miri64 and others added 3 commits January 16, 2019 07:54
@miri64 miri64 force-pushed the backport/2018.10/sock_dns/bug/i10739 branch from 520a9e8 to a8af1cc Compare January 16, 2019 06:55
@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

Rebased to current 2018.10-branch.

@jia200x
Copy link
Member

jia200x commented Jan 17, 2019

@kaspar030 Re-ACK?

@miri64
Copy link
Member Author

miri64 commented Jan 17, 2019

As suspected in #10765 Travis still isn't able to build.

@miri64
Copy link
Member Author

miri64 commented Jan 18, 2019

@jia200x could you review?

@jia200x
Copy link
Member

jia200x commented Jan 21, 2019

yes, sure

@miri64 miri64 mentioned this pull request Jan 25, 2019
Merged
4 tasks
@miri64
Copy link
Member Author

miri64 commented Jan 29, 2019

Should we close this, now that 2019.01 is imminent?

@jia200x
Copy link
Member

jia200x commented Jan 29, 2019

Should we close this, now that 2019.01 is imminent?

If we can ignore Travis output, I can test it now and merge

@miri64
Copy link
Member Author

miri64 commented Jan 29, 2019

If we can ignore Travis output, I can test it now and merge

We basically have to, since we would have to backport #10765 for that. Since Travis is performing the same checks as Murdock does, this seems unnecessary to me.

@jia200x
Copy link
Member

jia200x commented Jan 29, 2019

I was not able to reproduce the crash on my machine, but I think it's because of my local setup. The fix is evident and the patch is the same as in #10740. So, let's merge it

jia200x
jia200x approved these changes Jan 29, 2019
View reviewed changes
Copy link
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@jia200x jia200x merged commit 8aaf974 into RIOT-OS:2018.10-branch Jan 29, 2019
@miri64 miri64 deleted the backport/2018.10/sock_dns/bug/i10739 branch January 29, 2019 20:13
@miri64
Copy link
Member Author

miri64 commented Jan 29, 2019

So now we can publish 2018.10.1 just before 2019.01 ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jia200x jia200x jia200x approved these changes

@kaspar030 kaspar030 kaspar030 left review comments

@maribu maribu Awaiting requested review from maribu

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@miri64 @maribu @kaspar030 @jia200x