snmp_decode_msg and strings

Question

snmp_decode_msg and strings

Opened this issue 6 years ago · 3 comments

Darek,

This is an amazing piece of work! I actually visited Gdansk way before you were born, when the Russians were there.

Anyway, two issues (and I can provide code later):

the varbinds_num is not updated during processing in snmp_decode_msg
you cannot return a string using a pointer to its value in the buffer, as its not null terminated

You were on the right track with mallocing memory for strings. You could move the string back one byte position, then zero out the last byte, but then you'd be modifying the buffer.

Really, this is all I've found! I plan to upload my own framework with your code included, and will reference you then of course.

The README file does not do justice to your project - you should show the snmp_msg_test code - this is the power of what you did not the ber. Many people are looking for this and you are hiding it!!!

Answer 1 · 2018-12-11T22:05:02.000Z

Thanks, David! While trying to fix those two issues, I noticed a possible buffer overflow in the SNMP decode function. I fixed it right away, but I think the overall quality and bug-proofness of this entire SNMP code is simply too low to make it a separate library. It's only good enough for a BER demo and I believe that's why I didn't emphasize it too much in the docs. About the second issue:

you cannot return a string using a pointer to its value in the buffer, as

its not null terminated I assume you're talking about the string varbinds in snmp_decode_msg(). The strings inside the underlying buffer actually get null-terminated manually. Imagine a buffer like this: | snmp msg data | varbind #1 ber type | varbind #1 oid | varbind #1 string | varbind #2 ber type | varbind #2 oid | varbind #2 string | <a spare byte> | As the SNMP code doesn't check for varbind ber types, it can set any of them to 0 and hence make all varbind strings null-terminated. This isn't mentioned in the SNMP docs, but is described in the ber_decode_string_buffer() that SNMP uses. That spare byte which follows the provided buffer isn't documented anywhere and I have no doubt it should be, but first let me take an overall look at the SNMP code - maybe some parts of the API can be rethinked. Let me get back to you in a week. Thanks again, D.

…

On Sun, Dec 9, 2018 at 11:07 PM David Hoerl ***@***.***> wrote: Darek, This is an amazing piece of work! I actually visited Gdansk way before you were born, when the Russians were there. Anyway, two issues (and I can provide code later): - the varbinds_num is not updated during processing in snmp_decode_msg - you cannot return a string using a pointer to its value in the buffer, as its not null terminated You were on the right track with mallocing memory for strings. You could move the string back one byte position, then zero out the last byte, but then you'd be modifying the buffer. Really, this is all I've found! I plan to upload my own framework with your code included, and will reference you then of course. The README file does not do justice to your project - you should show the snmp_msg_test code - this is the power of what you did not the ber. Many people are looking for this and you are hiding it!!! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANpmLEP3WQkSI-3FGTPvL-1zlPkO98Mwks5u3Ym_gaJpZM4ZKWTs> .

Answer 2 · 2018-12-11T23:17:56.000Z

Darek, I’ve made several substantial changes to your code. I have no problem in making a PR if you want to incorporate it in your base, or I can just fork. What I’m doing is making the Strings “C” strings by converting the “Pascal” format (len+chars) to (chars+null). This gets rid of the next char too, and removes the need for an additional byte at the end of the array - but at the cost of a memmove. I also added other defines - COUNTERs and such - that cause issues in returned data. Overall, I am very very impressed with this project! The readme should hype the ability to construct and parse SNMP messages - many on StackOverFlow are looking for this. I plan to make my own open source project soon with your code and some networking code that glued together make a command line app to read a bunch of OIDs at once. I am putting your code very soon into a shipping product!!! David

…

On Dec 11, 2018, at 5:05 PM, Darek Stojaczyk ***@***.***> wrote: Thanks, David! While trying to fix those two issues, I noticed a possible buffer overflow in the SNMP decode function. I fixed it right away, but I think the overall quality and bug-proofness of this entire SNMP code is simply too low to make it a separate library. It's only good enough for a BER demo and I believe that's why I didn't emphasize it too much in the docs. About the second issue: > you cannot return a string using a pointer to its value in the buffer, as its not null terminated I assume you're talking about the string varbinds in snmp_decode_msg(). The strings inside the underlying buffer actually get null-terminated manually. Imagine a buffer like this: | snmp msg data | varbind #1 ber type | varbind #1 oid | varbind #1 string | varbind #2 ber type | varbind #2 oid | varbind #2 string | <a spare byte> | As the SNMP code doesn't check for varbind ber types, it can set any of them to 0 and hence make all varbind strings null-terminated. This isn't mentioned in the SNMP docs, but is described in the ber_decode_string_buffer() that SNMP uses. That spare byte which follows the provided buffer isn't documented anywhere and I have no doubt it should be, but first let me take an overall look at the SNMP code - maybe some parts of the API can be rethinked. Let me get back to you in a week. Thanks again, D. On Sun, Dec 9, 2018 at 11:07 PM David Hoerl ***@***.***> wrote: > Darek, > > This is an amazing piece of work! I actually visited Gdansk way before you > were born, when the Russians were there. > > Anyway, two issues (and I can provide code later): > > - the varbinds_num is not updated during processing in snmp_decode_msg > - you cannot return a string using a pointer to its value in the > buffer, as its not null terminated > > You were on the right track with mallocing memory for strings. You could > move the string back one byte position, then zero out the last byte, but > then you'd be modifying the buffer. > > Really, this is all I've found! I plan to upload my own framework with > your code included, and will reference you then of course. > > The README file does not do justice to your project - you should show the > snmp_msg_test code - this is the power of what you did not the ber. Many > people are looking for this and you are hiding it!!! > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#1>, or mute the thread > <https://github.com/notifications/unsubscribe-auth/ANpmLEP3WQkSI-3FGTPvL-1zlPkO98Mwks5u3Ym_gaJpZM4ZKWTs> > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

Answer 3 · 2018-12-23T19:55:34.000Z

I just uploaded my project, which is a superset of what you did. It has slightly modified per/snmp files to address a couple of bugs and add more SNMP_DATA_T_ types.

you can diff the four .c and .h files to see the changes: https://github.com/dhoerl/cber

Again, you did 99% of the work and I still remained very impressed with your work!!!