8000 fix heap-buffer-overflow by zhangtaoXT5 · Pull Request #957 · redis/hiredis · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix heap-buffer-overflow #957

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

Merged
merged 5 commits into from
Sep 1, 2022
Merged

Conversation

zhangtaoXT5
Copy link
Contributor

fix heap-buffer-overflow in #956

8000
@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.

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.

@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
0