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
Ill-formed oneof message leads to calling free on an arbitrary pointer #647
Comments
|
Setting pData to NULL when starting to decode a pointer type oneof field sounds reasonable. |
Nanopb would call free() or realloc() on an invalid (attacker controlled) pointer value when all the following conditions are true: - PB_ENABLE_MALLOC is defined at the compile time - Message definition contains an oneof field, and the oneof contains at least one pointer type field and at least one non-pointer type field. - Data being decoded first contains a non-pointer value for the oneof field, and later contains an overwriting pointer value. Depending on message layout, the bug may not be exploitable in all cases, but it is known to be exploitable at least with string and bytes fields. Actual security impact will also depend on the heap implementation used.
Nanopb would call free() or realloc() on an invalid (attacker controlled) pointer value when all the following conditions are true: - PB_ENABLE_MALLOC is defined at the compile time - Message definition contains an oneof field, and the oneof contains at least one pointer type field and at least one non-pointer type field. - Data being decoded first contains a non-pointer value for the oneof field, and later contains an overwriting pointer value. Depending on message layout, the bug may not be exploitable in all cases, but it is known to be exploitable at least with string and bytes fields. Actual security impact will also depend on the heap implementation used.
|
Security advisory: GHSA-7mv5-5mxh-qg88 I'm still a bit surprised why this hasn't been caught by the existing fuzztest. It seems the same issue should occur even with submessages, as used in Yet another bug that could potentially have been found with #143. |
Previously added static_message member of the oneof had the shortcoming that the first member of the submessage was also a pointer. So when it aliased with a pointer to a message, it was still a valid NULL pointer.
|
Fix released in 0.3.9.8 and 0.4.5. |
This affects
0.3.9.7(and probably lower versions, though I haven't checked) withPB_ENABLE_MALLOCenabled.Specially crafted bytes can make
pb_decodeeventually callpb_freeon an arbitrary pointer. Here's the smallest repro case I could make:Running this leads to:
I can repro this on both Linux and Mac.
What I believe happens is:
boolean_valueand result initer.pDatabeing set to1;bytes_value. Because there are no more bytes in the input, decoding the field fails (withend-of-stream). However, the current field is reset to5from1whileiter.pDatais not cleared and is still set to1;pb_decodetries to release the message.pb_release_single_field, thinking that the current field is5, considers the contents ofiter.pDatato refer to a dynamically-allocated array and callspb_freeon it.I'm not sure what the right fix would be -- perhaps
iter.pDatashould be set to null when oneof fields are switched, or perhaps the current field should not be changed until it is successfully parsed?Note that this is a potential security issue. I presume that if the first field was an integer, an arbitrary value could be written to it which would then be interpreted as an address and passed to
free.Note: this was found by OSS-Fuzz on Firestore (note that I have trimmed down the repro case from the original).
The text was updated successfully, but these errors were encountered: