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

Prevent possible integer overflow (hiredis) #6449

Closed
wants to merge 1 commit into from

Conversation

artix75
Copy link
Contributor

@artix75 artix75 commented Oct 11, 2019

Elements count in multi-bulk replies is read into a long long variable inside processAggregateItem.
In file deps/hiredis/read.c, line 423:

long long elements;

...

string2ll(p, len, &elements)

Anyway, later (line 482), its value is copied inside the elements member of a struct redisReadTask:

cur->elements = elements;

Since the elements member was declared as an int, an integer overflow could happen.

Declaring the element as size_t is safe since even if size_t was a 32bit, there's a check at line 442 that prevents writing values bigger than it can accept.

if (elements < -1 || (LLONG_MAX > SIZE_MAX && elements > SIZE_MAX)) { __redisReaderSetError(r,REDIS_ERR_PROTOCOL, "Multi-bulk length out of range"); return REDIS_ERR; }

@itamarhaber
Copy link
Member

Hello @artix75

Thanks for the PR - please note that contribs to hiredis should go upstream: https://github.com/redis/hiredis 🤣

@artix75
Copy link
Contributor Author

artix75 commented Oct 11, 2019

Hello @artix75

Thanks for the PR - please note that contribs to hiredis should go upstream: https://github.com/redis/hiredis 🤣

Yeah, I know 😄

I'm going to fork the hiredis repo and create PR there too.
Thank you! 😉

@artix75
Copy link
Contributor Author

artix75 commented Dec 13, 2019

I made a little mess with another PR, I'm closing this one for now. I will reopen it later, so please ignore this PR, thank you.

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.

None yet

2 participants