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.

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

Bugfix/snmp engine #1355

Merged
merged 3 commits into from Oct 14, 2020
Merged

Conversation

mjurczak
Copy link
Contributor

Initial work to fix most critical issues described in SNMP agent implementation:
#1354
#1353
#1352
#1351

The proposed changes require testing and do not cover all out-of-bounds conditions that may occur when processing incoming SNMP requests.

I think it would also be great if there was an automated method of functional validation of the SNMP agent implementation.

Implemented measures to verify if the number of requested variables does not overflow the buffer.
The oid_len value is passed down to OID parsing function that uses the value to check if a buffer overflow condition occured.
@Yagoor
Copy link
Contributor

Yagoor commented Aug 19, 2020

Hi mjurczak,
I implemented the SNMP protocol last year.

I believe you have some tests scenarios that you used to catch these bugs.
Could you add them to the unit test?
The unit test basically only validates the protocol, meaning that it makes sure it is compliant, but it doesn't really stresses it.

@mjurczak
Copy link
Contributor Author

Hi Yagoor,

Some sample inputs are provided in issue describtions. I will provide you with more samples that cause similar porcessing issues.

I will also try to figure out which issues can be verified by UTs. For instance - if the number of requested OIDs exceeds supported value, the parser should be expected to return an error I think.
On the other hand, writes and reads beyond buffer boundaries and certain class of stack overwrite issues may either pass unnoticed in UTs or cause UT framework crash.

Most of the described issues were discovered by means of fuzzing. It would be my pleasure to post a sample base app and a short description on how to run fuzzer on the SNMP agent.

Kind regards.

@Yagoor
Copy link
Contributor

Yagoor commented Aug 21, 2020

Hi mjurczak,

Thank you,
I'll use your branch to add unit tests that implement the scenarios that you describe.
I understand that the beyond buffer boundaries is trick to catch, but I'll give some thought if there is a way to make sure the test fails.

Best Regards,

Copy link
Contributor

@Yagoor Yagoor left a comment

Choose a reason for hiding this comment

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

Hi @g-oikonomou.
I took I look into his changes and they all seems pretty neat.

However in order to prevent these issues and making sure they remain fixed I suggest adding the test cases described in the issues he linked to this PR.
Unfortunately, I've been pretty busy at work these days.

@mjurczak, would you be able to create the unit tests? I have an idea on how to do it.
If you don't have time I believe a good approach would be to merge this PR without the unit tests and create an issue and assign it to me and in the future I'll create the unit tests and maybe others. Having the issue will keep this under my radar and I won't forget it.

@g-oikonomou
Copy link
Member

If this is stand-alone and can be merged without further work I can for sure merge this. Thanks for reviewing!

@g-oikonomou
Copy link
Member

@mjurczak @Yagoor are we ready here? Can I merge this?

@g-oikonomou g-oikonomou self-assigned this Sep 15, 2020
@g-oikonomou g-oikonomou added bug/vulnerability pr/bugfix Used for PRs that fix a bug of any severity labels Sep 15, 2020
@g-oikonomou g-oikonomou added this to the Version 4.6 milestone Sep 15, 2020
@Yagoor
Copy link
Contributor

Yagoor commented Sep 19, 2020

HI @g-oikonomou. I'm working on more improvements in the protocol implementation.
After his points, I reviewed the code and notice more vulnerabilities (corner cases).

I see that you added it to version 4.6 milestones. I'll try to get it done for it.
I'm implementing several unit tests in the engine to make sure it is not only compliant with the protocol but that is also handles corner cases.

@Yagoor Yagoor mentioned this pull request Sep 27, 2020
Copy link
Member

@g-oikonomou g-oikonomou left a comment

Choose a reason for hiding this comment

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

Hi @mjurczak thank you for this, I finally managed to make some time to review. Looks good to me. Please if you can fix the code style as per the comments, then I am keen to merge this one. Let me know if you have no time and I'll apply the fixes.

@@ -128,6 +128,11 @@ snmp_ber_encode_null(unsigned char *out, uint32_t *out_len, uint8_t type)
unsigned char *
snmp_ber_decode_type(unsigned char *buff, uint32_t *buff_len, uint8_t *type)
{
if(*buff_len == 0)
{
Copy link
Member

Choose a reason for hiding this comment

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

Style - opening brace in the same line with the if, like so:

if(*buff_len == 0) {

@@ -137,6 +142,11 @@ snmp_ber_decode_type(unsigned char *buff, uint32_t *buff_len, uint8_t *type)
unsigned char *
snmp_ber_decode_length(unsigned char *buff, uint32_t *buff_len, uint8_t *length)
{
if(*buff_len == 0)
{
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

/*
* Sanity check
* It will not fit in the uint32_t
*/
return NULL;
}

if(*buff_len < len)
{
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@@ -150,7 +160,8 @@ snmp_ber_decode_integer(unsigned char *buf, uint32_t *buff_len, uint32_t *num)

buf = snmp_ber_decode_type(buf, buff_len, &type);

if(type != BER_DATA_TYPE_INTEGER) {
if(buf == NULL ||
type != BER_DATA_TYPE_INTEGER) {
Copy link
Member

Choose a reason for hiding this comment

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

You could keep those in the same line, but you don't have to if you cannot be bothered

/*
* Sanity check
* It will not fit in the uint32_t
*/
return NULL;
}

if(*buff_len < len)
{
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@@ -222,7 +246,8 @@ snmp_ber_decode_string_len_buffer(unsigned char *buf, uint32_t *buff_len, const

buf = snmp_ber_decode_type(buf, buff_len, &type);

if(type != BER_DATA_TYPE_OCTET_STRING) {
if(buf == NULL ||
type != BER_DATA_TYPE_OCTET_STRING) {
Copy link
Member

Choose a reason for hiding this comment

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

Same line - optional

@@ -196,14 +214,20 @@ snmp_ber_decode_unsigned_integer(unsigned char *buf, uint32_t *buff_len, uint8_t

buf = snmp_ber_decode_length(buf, buff_len, &len);

if(len > 4) {
if(buf == NULL ||
len > 4) {
Copy link
Member

Choose a reason for hiding this comment

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

Same line - optional

@@ -186,7 +203,8 @@ snmp_ber_decode_unsigned_integer(unsigned char *buf, uint32_t *buff_len, uint8_t

buf = snmp_ber_decode_type(buf, buff_len, &type);

if(type != expected_type) {
if(buf == NULL ||
type != expected_type) {
Copy link
Member

Choose a reason for hiding this comment

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

Same line - optional

@@ -160,14 +171,20 @@ snmp_ber_decode_integer(unsigned char *buf, uint32_t *buff_len, uint32_t *num)

buf = snmp_ber_decode_length(buf, buff_len, &len);

if(len > 4) {
if(buf == NULL ||
len > 4) {
Copy link
Member

Choose a reason for hiding this comment

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

Same line - optional

@@ -224,6 +224,10 @@ snmp_message_decode(uint8_t *buf, uint32_t buf_len, snmp_header_t *header,
}

for(i = 0; buf_len > 0; ++i) {
if (i >= *varbind_num)
{
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Member

Choose a reason for hiding this comment

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

Also no space between if and opening paren

@g-oikonomou g-oikonomou merged commit 35e5ac8 into contiki-ng:develop Oct 14, 2020
1 check passed
@g-oikonomou g-oikonomou mentioned this pull request Oct 14, 2020
@g-oikonomou
Copy link
Member

Style errors fixed in #1414 instead of blocking this one

@Scepticz
Copy link
Contributor

The merge of this pull request fixes (among other issues) the issue of a previously reserved CVE:
CVEID: 2020-12141
PRODUCT: Contiki-NG
VERSION: the issue affects versions 4.5 and earlier
PROBLEM TYPE: Denial of Service / Information Disclosure due to OOB Read
DESCRIPTION: An out-of-bounds read in the SNMP stack in Contiki-NG 4.5 and earlier allows an attacker to cause a denial of service and potentially disclose information via crafted SNMP packets to snmp_ber_decode_string_len_buffer in os/net/app-layer/snmp/snmp-ber.c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/vulnerability pr/bugfix Used for PRs that fix a bug of any severity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants