nanocoap: fix server-side option_count overflow #10754

kaspar030 · 2019-01-11T10:36:02Z

Contribution description

nanocoap limits the number of options it parses, but didn't check if that limit was reached when parsing a packet. (See report in #10753).

This PR provides a fix. It also adds a unittest that exposes the flaw.

Fixes #10753.

Testing procedure

Run unittests without uppermost commit. The nanocoap test should crash on native. Run again with topmost commit. Tests should succeed.

Issues/PRs references

See #10753.

kb2ma · 2019-01-11T11:15:43Z

Just took a first look. The fix looks appropriate. Need to do a little more testing.

In the unit test, how do I know that the printed pointer values indicate a good test? Just that they are close to each other? Is it possible to be more specific? It's odd to print values as part of a unit test. I would think it makes more sense to compare the values within the test, and fail the test if the difference is outside of some range.

kaspar030 · 2019-01-11T11:26:41Z

In the unit test, how do I know that the printed pointer values indicate a good test? Just that they are close to each other? Is it possible to be more specific? It's odd to print values as part of a unit test. I would think it makes more sense to compare the values within the test, and fail the test if the difference is outside of some range.

When I wrote the test, I wasn't sure that it would (without the fix) crash.
The coap options array is the last field in coap_pkt_t. My logic was that if I have pkt on the stack and directly after it another value (guard_value), if coap_parse() exceeded the options array, it would overwrite the guard value. But without getting an address of both of the variables, the compiler might just keep them in registers. Thus the printf...
While writing this I realized that any overflow would go to stack space before (or above) pkt, not after (or below), on all our platforms. So I think the guard_value declaration must be moved before pkt.

Does this make sense at all or does anyone have a better idea on how to detect an overflow of the options array?

kb2ma · 2019-01-11T12:08:04Z

Sorry, I have no insights. In keeping with the current approach, you could put a guard value on both sides to be safe.

Another approach is to set the maximum number of options (NANOCOAP_NOPTS_MAX) to a small value and try a functional test. That's what I'm working on.

kb2ma · 2019-01-11T13:35:20Z

You also could craft an array with NANOCOAP_NOPTS_MAX options, parse it, and expect success. Then add another option to it and expect -ENOMEM. That should be sufficient for this test. No need to get into the low level overflow detection.

kb2ma

Need to fix option count comparison.

kb2ma · 2019-01-11T14:20:59Z

sys/net/application_layer/nanocoap/nanocoap.c

@@ -111,6 +111,9 @@ int coap_parse(coap_pkt_t *pkt, uint8_t *buf, size_t len)
 DEBUG("optpos option_nr=%u %u\n", (unsigned)option_nr, (unsigned)optpos->offset);
 optpos++;
 option_count++;
+ if (option_count >= NANOCOAP_NOPTS_MAX) {


It should be OK to include NANOCOAP_NOPTS_MAX options. This test should be moved above addition to the array, probably line 109. The test then should be:

if (option_count == NANOCOAP_NOPTS_MAX) {

Yes. The new test seems to be working, it exposed that using "==" here caused to write one behind the options array. So I moved the check to the beginning of the if case.

kaspar030 · 2019-01-12T09:50:47Z

Then add another option to it and expect -ENOMEM. That should be sufficient for this test. No need to get into the low level overflow detection.

I think I found a way. At least on native it works as expected.

kb2ma

I agree the verification in coap_parse() is in the right place now. See my suggested alternative unit test in kapar030/#41. We're getting closer...

sys/net/application_layer/nanocoap/nanocoap.c

kb2ma · 2019-01-12T12:51:03Z

tests/unittests/tests-nanocoap/tests-nanocoap.c

@@ -433,6 +434,40 @@ static void test_nanocoap__server_reply_simple_con(void)
 TEST_ASSERT_EQUAL_INT(COAP_TYPE_ACK, coap_get_type(&pkt));
 }

+static void test_nanocoap__server_option_count_overflow_check(void)


I kind of understand what you are doing to test the overflow. Why are there 42 options in guard data? How does the unit test work? I think it's important for others to understand this without a lot of thought/investigation.

I've added some comments.

kaspar030 · 2019-01-12T15:28:40Z

See my suggested alternative unit test in kapar030/#41.

I have cherry-picked the commit. IMO more test code doesn't hurt, even if they test the same thing.

kb2ma · 2019-01-13T11:13:36Z

I agree it's OK to include both tests since they verify from different directions. These tests run OK for me on native and samr21-xpro. I verified that allowing coap_parse() to write an extra option makes the tests fail.

Thanks for adding the comments in the test. The only remaining question for me is why are there 42 options in guard_data instead of only one or two. When I allowed the parse to overwrite the guard data, I found the overwrite occurred in the first few bytes.

kaspar030 · 2019-01-14T10:49:34Z

The only remaining question for me is why are there 42 options in guard_data instead of only one or two. When I allowed the parse to overwrite the guard data, I found the overwrite occurred in the first few bytes.

@nmeum provided this packet in his original report, and I was lazy enough to take it as-is. The number just needs to be higher than NANOCOAP_NOPT_MAX. (42 seemed to be the right answer after thinking for 7.5 million years.) I've added a note stating that 42 is basically randomly chosen.

kb2ma · 2019-01-14T11:19:38Z

OK, good enough. ACK, please squash.

kaspar030 · 2019-01-14T12:26:30Z

Thanks @kb2ma!

jia200x · 2019-01-14T13:49:44Z

@kaspar030 could you provide a backport to the 2018.10 branch?

kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: security Area: Security-related libraries and subsystems Area: CoAP Area: Constrained Application Protocol implementations labels Jan 11, 2019

kaspar030 requested review from miri64 and kb2ma January 11, 2019 10:36

kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jan 11, 2019

kaspar030 mentioned this pull request Jan 11, 2019

nanocoap: options buffer overflow #10753

Closed

kb2ma requested changes Jan 11, 2019

View reviewed changes

kb2ma mentioned this pull request Jan 12, 2019

tests/nanocoap: add test for option parse overflow kaspar030/RIOT#41

Closed

kb2ma requested changes Jan 12, 2019

View reviewed changes

kaspar030 removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Jan 12, 2019

kb2ma added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Jan 13, 2019

kb2ma approved these changes Jan 14, 2019

View reviewed changes

kaspar030 and others added 3 commits January 14, 2019 12:30

tests/unittests/nanocoap: add test case for option_count overflow

dd53bb8

sys/net/nanocoap: fix possible option_count overflow

4b1b0aa

tests/nanocoap: add test for option parse overflow

1058a8d

kaspar030 force-pushed the fix_nanocoap_optlen branch from 8b86050 to 1058a8d Compare January 14, 2019 11:30

kaspar030 merged commit 9767dd3 into RIOT-OS:master Jan 14, 2019

kaspar030 deleted the fix_nanocoap_optlen branch January 22, 2019 10:22

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nanocoap: fix server-side option_count overflow #10754

nanocoap: fix server-side option_count overflow #10754

kaspar030 commented Jan 11, 2019

kb2ma commented Jan 11, 2019

kaspar030 commented Jan 11, 2019

kb2ma commented Jan 11, 2019

kb2ma commented Jan 11, 2019

kb2ma left a comment

kb2ma Jan 11, 2019 •

edited

Loading

kaspar030 Jan 12, 2019

kaspar030 commented Jan 12, 2019

kb2ma left a comment

kb2ma Jan 12, 2019

kaspar030 Jan 12, 2019

kaspar030 commented Jan 12, 2019

kb2ma commented Jan 13, 2019

kaspar030 commented Jan 14, 2019

kb2ma commented Jan 14, 2019

kaspar030 commented Jan 14, 2019

jia200x commented Jan 14, 2019

nanocoap: fix server-side option_count overflow #10754

nanocoap: fix server-side option_count overflow #10754

Conversation

kaspar030 commented Jan 11, 2019

Contribution description

Testing procedure

Issues/PRs references

kb2ma commented Jan 11, 2019

kaspar030 commented Jan 11, 2019

kb2ma commented Jan 11, 2019

kb2ma commented Jan 11, 2019

kb2ma left a comment

Choose a reason for hiding this comment

kb2ma Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

kaspar030 Jan 12, 2019

Choose a reason for hiding this comment

kaspar030 commented Jan 12, 2019

kb2ma left a comment

Choose a reason for hiding this comment

kb2ma Jan 12, 2019

Choose a reason for hiding this comment

kaspar030 Jan 12, 2019

Choose a reason for hiding this comment

kaspar030 commented Jan 12, 2019

kb2ma commented Jan 13, 2019

kaspar030 commented Jan 14, 2019

kb2ma commented Jan 14, 2019

kaspar030 commented Jan 14, 2019

jia200x commented Jan 14, 2019

kb2ma Jan 11, 2019 •

edited

Loading