Skip to content
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

nanocoap: defuse footgun in coap_get_uri_query() #20195

Merged
merged 3 commits into from
Dec 19, 2023
Merged

nanocoap: defuse footgun in coap_get_uri_query() #20195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f56edf5
nanocoap: defuse footgun in coap_get_uri_query()
benpicco Dec 19, 2023
a1c3d5a
nanocoap: rename coap_get_uri_query() -> coap_get_uri_query_string()
benpicco Dec 19, 2023
d6ca03a
tests/unittests: coap_get_uri_query() -> coap_get_uri_query_string()
benpicco Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions sys/include/net/nanocoap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,18 +847,18 @@
* This function decodes the pkt's URI_QUERY option into a "&"-separated and
* '\0'-terminated string.
*
* Caller must ensure @p target can hold at least CONFIG_NANOCOAP_URI_MAX bytes!
*
* @param[in] pkt pkt to work on
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param[in] pkt pkt to work on
* @note This function is mainly intended to get easy debug output.
* It is needlessly complex and error prone for parsing parameters out of the query,
* which is better done by @ref coap_opt_get_opaque with increasing values of @p opt_num,
* until that returns `-ENOENT`.
* @param[in] pkt pkt to work on

Part of the footgun (IMO the bigger footgun even) is that this function drives people toward assembling and later parsing this string, which inevitably leads to trouble once the first & appears in a query parameter (and if that were fixed, the string would go through percent encoding and then back again).

Copy link
Member

Choose a reason for hiding this comment

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

Scratch that -- we'll still need such a note, but #20197 adds something that would be pointed to instead.

* @param[out] target buffer for target URI
* @param[in] max_len size of @p target in bytes
*
* @returns -ENOSPC if URI option is larger than CONFIG_NANOCOAP_URI_MAX
* @returns -ENOSPC if URI option is larger than @p max_len
* @returns nr of bytes written to @p target (including '\0')
*/
static inline ssize_t coap_get_uri_query(coap_pkt_t *pkt, uint8_t *target)
static inline ssize_t coap_get_uri_query_string(coap_pkt_t *pkt, char *target,
Copy link
Member

Choose a reason for hiding this comment

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

Changing this to char * is also a huge improvement. Since callers have to change the function call anyway, I agree that this quality of life improvement can also be added just as well.

size_t max_len)
{
return coap_opt_get_string(pkt, COAP_OPT_URI_QUERY, target,
CONFIG_NANOCOAP_URI_MAX, '&');
return coap_opt_get_string(pkt, COAP_OPT_URI_QUERY,
(uint8_t *)target, max_len, '&');
}

/**
Expand Down Expand Up @@ -1826,7 +1826,7 @@
*
* @returns amount of bytes written to @p buf
*/
size_t coap_put_option(uint8_t *buf, uint16_t lastonum, uint16_t onum, const void *odata, size_t olen);

Check warning on line 1829 in sys/include/net/nanocoap.h

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters

/**
* @brief Insert block1 option into buffer
Expand Down Expand Up @@ -2214,4 +2214,4 @@
}
#endif
#endif /* NET_NANOCOAP_H */
/** @} */

Check warning on line 2217 in sys/include/net/nanocoap.h

View workflow job for this annotation

GitHub Actions / static-tests 

source file is too long
36 changes: 18 additions & 18 deletions tests/unittests/tests-nanocoap/tests-nanocoap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ static void test_nanocoap__hdr_2(void)
uint16_t msgid = 0xABCD;
char path[] = "/test/abcd/efgh?foo=bar&baz=blub";
unsigned char path_tmp[64] = {0};
unsigned char query_tmp[64] = {0};
char query_tmp[64] = {0};

uint8_t *pktpos = &buf[0];
uint16_t lastonum = 0;
Expand All @@ -83,7 +83,7 @@ static void test_nanocoap__hdr_2(void)
TEST_ASSERT_EQUAL_INT(sizeof("/test/abcd/efgh"), res);
TEST_ASSERT_EQUAL_STRING("/test/abcd/efgh", (char *)path_tmp);

res = coap_get_uri_query(&pkt, query_tmp);
res = coap_get_uri_query_string(&pkt, query_tmp, sizeof(query_tmp));
TEST_ASSERT_EQUAL_INT(sizeof("&foo=bar&baz=blub"), res);
TEST_ASSERT_EQUAL_STRING("&foo=bar&baz=blub", (char *)query_tmp);
}
Expand Down Expand Up @@ -321,14 +321,14 @@ static void test_nanocoap__get_query(void)
TEST_ASSERT_EQUAL_STRING((char *)path, (char *)uri);

char query[10] = {0};
coap_get_uri_query(&pkt, (uint8_t *)&query[0]);
/* skip initial '&' from coap_get_uri_query() */
coap_get_uri_query_string(&pkt, query, sizeof(query));
/* skip initial '&' from coap_get_uri_query_string() */
TEST_ASSERT_EQUAL_STRING((char *)qs, &query[1]);

/* overwrite query to test buffer-based put */
coap_opt_put_uri_query(query_pos, COAP_OPT_URI_PATH, qs);
coap_get_uri_query(&pkt, (uint8_t *)&query[0]);
/* skip initial '&' from coap_get_uri_query() */
coap_get_uri_query_string(&pkt, query, sizeof(query));
/* skip initial '&' from coap_get_uri_query_string() */
TEST_ASSERT_EQUAL_STRING((char *)qs, &query[1]);
}

Expand Down Expand Up @@ -359,14 +359,14 @@ static void test_nanocoap__get_multi_query(void)
TEST_ASSERT_EQUAL_INT(2, optlen);

char query[20] = {0};
coap_get_uri_query(&pkt, (uint8_t *)&query[0]);
/* skip initial '&' from coap_get_uri_query() */
coap_get_uri_query_string(&pkt, query, sizeof(query));
/* skip initial '&' from coap_get_uri_query_string() */
TEST_ASSERT_EQUAL_STRING((char *)qs, &query[1]);

/* overwrite query to test buffer-based put */
coap_opt_put_uri_query(query_pos, COAP_OPT_URI_PATH, qs);
coap_get_uri_query(&pkt, (uint8_t *)&query[0]);
/* skip initial '&' from coap_get_uri_query() */
coap_get_uri_query_string(&pkt, query, sizeof(query));
/* skip initial '&' from coap_get_uri_query_string() */
TEST_ASSERT_EQUAL_STRING((char *)qs, &query[1]);
}
/*
Expand Down Expand Up @@ -399,24 +399,24 @@ static void test_nanocoap__add_uri_query2(void)
char query[20] = {0};
len = coap_opt_add_uri_query2(&pkt, keys, key1_len, vals, val1_len);
TEST_ASSERT_EQUAL_INT(query1_opt_len, len);
coap_get_uri_query(&pkt, (uint8_t *)&query[0]);
/* skip initial '&' from coap_get_uri_query() */
coap_get_uri_query_string(&pkt, query, sizeof(query));
/* skip initial '&' from coap_get_uri_query_string() */
TEST_ASSERT_EQUAL_STRING((char *)qs1, &query[1]);

/* includes key only */
memset(query, 0, 20);
len = coap_opt_add_uri_query2(&pkt, &keys[2], key2_len, NULL, 0);
TEST_ASSERT_EQUAL_INT(query2_opt_len, len);
coap_get_uri_query(&pkt, (uint8_t *)&query[0]);
/* skip initial '&' from coap_get_uri_query() */
coap_get_uri_query_string(&pkt, query, sizeof(query));
/* skip initial '&' from coap_get_uri_query_string() */
TEST_ASSERT_EQUAL_STRING((char *)qs2, &query[1]);

/* includes key only; value not NULL but zero length */
memset(query, 0, 20);
len = coap_opt_add_uri_query2(&pkt, &keys[2], key2_len, &vals[3], 0);
TEST_ASSERT_EQUAL_INT(query3_opt_len, len);
coap_get_uri_query(&pkt, (uint8_t *)&query[0]);
/* skip initial '&' from coap_get_uri_query() */
coap_get_uri_query_string(&pkt, query, sizeof(query));
/* skip initial '&' from coap_get_uri_query_string() */
TEST_ASSERT_EQUAL_STRING((char *)qs3, &query[1]);

/* fails an assert, so only run when disabled */
Expand All @@ -428,8 +428,8 @@ static void test_nanocoap__add_uri_query2(void)
memset(query, 0, 20);
len = coap_opt_add_uri_query2(&pkt, &keys[2], key2_len, NULL, 1);
TEST_ASSERT_EQUAL_INT(query4_opt_len, len);
coap_get_uri_query(&pkt, (uint8_t *)&query[0]);
/* skip initial '&' from coap_get_uri_query() */
coap_get_uri_query_string(&pkt, query, sizeof(query));
/* skip initial '&' from coap_get_uri_query_string() */
TEST_ASSERT_EQUAL_STRING((char *)qs4, &query[1]);
#endif
}
Expand Down
Loading