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 heap-buffer-overflow #957

Merged
merged 5 commits into from
Sep 1, 2022
Merged

Conversation

zhangtaoXT5
Copy link
Contributor

fix heap-buffer-overflow in #956

@michael-grunder
Copy link
Collaborator

michael-grunder commented Aug 29, 2022

Hi,

I'm going through PRs in preparation for a new release. I see what you're fixing here, but can you provide an example format string where we would currently read past the end of the buffer?

I think a regression test may be worthwhile.

hiredis.c Outdated Show resolved Hide resolved
Whitespace

Co-authored-by: Kristján Valur Jónsson <sweskman@gmail.com>
@bjosv
Copy link
Contributor

bjosv commented Sep 1, 2022

I ran the fuzzing tests described in the issue to be able to propose a testcase.
I have pinpointed the problem to the format specifier parser and will push an alternative fix for this, together with the testcases.

@michael-grunder
Copy link
Collaborator

Closing in favor of #1097. Thanks for finding and reporting the issue though!

@kristjanvalur
Copy link
Contributor

kristjanvalur commented Sep 1, 2022

I contest closing this. Incrementing a zero terminated string twice, without checking that there is indeed a zero in between increments, is dangerous. One should always check for zeroes, even if one strongly believes that the string is correctly constructed.

It is prudent, from a security perspective, to leave no opening for reading past the end of a string. Even if one has good faith that the string is well formed.

hiredis.c Outdated
@@ -477,6 +477,8 @@ int redisvFormatCommand(char **target, const char *format, va_list ap) {

touched = 1;
c++;
if (*c == '\0')
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change this to break to make the intention more explicit.

@gzur
Copy link

gzur commented Sep 1, 2022

I am a professional programmer and I approve of this message.

@michael-grunder
Copy link
Collaborator

@kristjanvalur I see your point. It certainly doesn't hurt anything to apply both fixes.

hiredis.c Outdated Show resolved Hide resolved
hiredis.c Outdated Show resolved Hide resolved
hiredis.c Outdated Show resolved Hide resolved
@michael-grunder michael-grunder merged commit bc8d837 into redis:master Sep 1, 2022
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.

5 participants