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
Memory leak when parsing a protobuf message with duplicate fields #615
Comments
|
I agree with you that parsing untrusted input shouldn't leak memory, and the fuzztest included in nanopb tries to verify that also. It appears that your test case is hitting some situation that is not covered by the fuzztest. I'll have to take a look. |
Nanopb would leak memory when all of the following conditions were true: - PB_ENABLE_MALLOC is defined at the compile time - Message definitions contains an oneof field, the oneof contains a static submessage, and the static submessage contains a pointer field. - Data being decoded contains two values for the submessage. The logic in pb_release_union_field would detect that the same submessage occurs twice, and wouldn't release it because keeping the old values is necessary to match the C++ library behavior regarding message merges. But then decode_static_field() would go to memset() the whole submessage to zero, because it unconditionally assumed it to be uninitialized memory. This would normally happen when the contents of the union field is switched to a different oneof item, instead of merging with the same one. This commit changes it so that the field is memset() only when `which_field` contains a different tag. Also the setting of the default values for the submessage was moved to decode_static_field() so that it wouldn't overwrite the values that must be merged. Test cases must still be extended to cover this problem.
|
Looks like the current test cases did not find this because they contained only a fully static fields inside a static submessage inside oneof, and pointer fields inside a pointer submessage inside oneof. And the bug is only triggered by pointer fields inside a static submessage inside oneof. There is a preliminary fix on git now, but test cases must still be expanded to cover this. Something like #143 would be nice to help finding cases like this, but I'm not sure how to go about actually implementing that. |
This also covers the fairly rarely used behavior of protobuf C++ library regarding oneof merges: if an oneof submessage occurs multiple times in a message, their contents are merged together. This behavior was also previously broken in nanopb.
This gives the fuzzer a chance to find bugs like #615 in the future.
|
Thanks for your quick reply. I confirm your patches fix the memory leak in the project that I am fuzzing (I updated Nanopb to I am not familiar enough with the project to help adding what would be necessary to generate For this issue, do you know when a release will include a fix? (In a few weeks/months/...?). Also, is there any plan regarding back-porting the fix to branch |
|
Yeah, I'm working on making a release, you can expect it today or tomorrow. And yeah, I will backport the fix and the test to 0.3 also. |
This also covers the fairly rarely used behavior of protobuf C++ library regarding oneof merges: if an oneof submessage occurs multiple times in a message, their contents are merged together. This behavior was also previously broken in nanopb.
Nanopb would leak memory when all of the following conditions were true: - PB_ENABLE_MALLOC is defined at the compile time - Message definitions contains an oneof field, the oneof contains a static submessage, and the static submessage contains a pointer field. - Data being decoded contains two values for the submessage. The logic in pb_release_union_field would detect that the same submessage occurs twice, and wouldn't release it because keeping the old values is necessary to match the C++ library behavior regarding message merges. But then decode_static_field() would go to memset() the whole submessage to zero, because it unconditionally assumed it to be uninitialized memory. This would normally happen when the contents of the union field is switched to a different oneof item, instead of merging with the same one. This commit changes it so that the field is memset() only when `which_field` contains a different tag.
|
Fix is now released in 0.4.4 and 0.3.9.7. |
Hello,
While fuzzing a project that relies on Nanopb to parse (untrusted) user input, I found a memory leak which is triggered by sending some message where fields are duplicated.
Steps to reproduce the issue
In order to test this memleak on several versions of Nanopb (and several Linux distributions), I have written the following script:
What happens?
On a up-to-date Debian 10 machine, this leads to the following output:
With my program,
0a06 0a020a00 0a00leaks 4 bytes,0a0a 0a020a00 0a020a00 0a00leaks 8 bytes, etc.What should happen?
I believe that parsing untrusted input should not leak allocated memory. You might disagree with this belief, in which case it would be nice to indicate in https://github.com/nanopb/nanopb/security/policy that Nanopb may leak memory when parsing untrusted data which was maliciously crafted.
The text was updated successfully, but these errors were encountered: