fluent-bit
fluent-bit copied to clipboard
fluent
pack: Fix read Stack-buffer-overflow READ in msgpack_sbuffer_write
This PR fixes Stack-buffer-overflow in msgpack_sbuffer_write revealed by fuzzing: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=45208
The root cause is that snprintf call in flb_pack_msgpack_to_json_format returns the number of characters that would have been written if the buffer had been sufficiently large. It leads to reading out of the boundaries of the buffer later.
Signed-off-by: Aleks L [email protected]
Enter [N/A] in the box, if an item is not applicable to your change.
Testing Before we can approve your change; please submit the following in a comment:
- [ ] Example configuration file for the change
- [ ] Debug log output from testing the change
- [ ] Attached Valgrind output that shows no leaks or memory corruption was found
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
- [ ] Attached local packaging test output showing all targets (including any new ones) build.
Documentation
- [ ] Documentation required for this feature
Backporting
- [ ] Backport to latest stable release.
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Hopefully now everything is correct. It truncates the time, but it is as it was, I was afraid to break anything. Let me know if you think it is better to make the buffer bigger.
Sorry, missed that request.
It is not about memory leaks (unless you meant leaking stack variables to the packaged msg), but about reading stack buffer beyond bounds.
The char time_formatted[32]; is first filled with 19 characters like 1970-01-01 00:00:00.
Then snprintf returns 18 - how much it would need to print microseconds. It is added to 19: s += len; and gives 37 which is 5 bytes bigger than the size of the time_formatted buffer. So the msgpack_pack_str_body(&tmp_pck, time_formatted, s); call results in reading beyond the buffer bounds.
Reading the documentation I think you are doing minus one for no reason:
Provided that the result string, including the terminating null byte, does not exceed max bytes, strftime() returns the number of bytes (excluding the terminating null byte) placed in the arrays.
and
The functions snprintf() and vsnprintf() write at most size bytes (including the terminating null byte ('\0')) to str.
So I have removed that and increased the buffer to fit microseconds. It should always fit now, but I also added the return if it would be truncated.
Then snprintf returns 18 - how much it would need to print microseconds.
How does it return 18 ?
tms.tm.tv_sec is -100000000. Then (uint64_t) tms.tm.tv_nsec / 1000 gives 18446744073609551 which requires 18 bytes to format with '\0'.
While you are here... May I ask why https://github.com/fluent/fluent-bit/pull/5727 is not merged yet?
Regarding the truncation, something similar happened previously: https://github.com/fluent/fluent-bit/pull/2798
It might be nice to take a quick skim through all uses of snprintf
Looks good to me, thanks for extracting the functionality and fixing the remaining block in the process!
