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

Fix out of bounds memory read in onig_node_str_cat #164

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sashashura
Copy link

This PR fixes out of bounds memory read in onig_node_str_cat revealed by fuzzing fluent-bit:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=46049

The root cause is that a call to enclen inside of PFETCH macro when called in fetch_token results in a call to onigenc_mbclen_approximate.
When the value of p passed to the function is \xec even though it is the last byte in multibyte sequince (the next byte is unexpected string terminator \0) the onigenc_mbclen_approximate returns it's size as 4. The size is added to the overall string length and results in reading past the end of the string.

@sashashura
Copy link
Author

@k-takata?

@DavidKorczynski
Copy link

Given #165 and this PR, does it make sense to create a fork that accepts security patches and then use that fork?

@sashashura
Copy link
Author

I have contemplated making local temporary changes in https://github.com/fluent/fluent-bit/tree/master/lib/onigmo. Making a temporary fork for security patches only until this repository gets back to life is an option if there are more dependent repositories willing to adopt it. In any case fluent-bit maintainers have to be convinced to make a change (either local patch or reference another fork), but they hesitate to accept security patches even in their code so far.

sashashura added a commit to sashashura/fluent-bit that referenced this pull request Sep 5, 2022
Temporary fix until k-takata/Onigmo#164 is merged
sashashura added a commit to sashashura/fluent-bit that referenced this pull request Sep 5, 2022
Temporary fix until k-takata/Onigmo#164 is merged
Signed-off-by: sashashura <[email protected]>
sashashura added a commit to sashashura/fluent-bit that referenced this pull request Sep 5, 2022
Temporary fix until k-takata/Onigmo#164 is merged
Signed-off-by: sashashura <[email protected]>
sashashura added a commit to sashashura/onigmo-1 that referenced this pull request Sep 7, 2022
Temporary fix until k-takata/Onigmo#164 is merged
Signed-off-by: sashashura <[email protected]>
sashashura added a commit to sashashura/onigmo-1 that referenced this pull request Sep 7, 2022
Temporary fix until k-takata/Onigmo#164 is merged
Signed-off-by: sashashura <[email protected]>
edsiper pushed a commit to fluent/onigmo that referenced this pull request Sep 11, 2022
Temporary fix until k-takata/Onigmo#164 is merged
Signed-off-by: sashashura <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants