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

Simplified _libssh2_check_length #350

Merged
merged 2 commits into from Apr 5, 2019
Merged

Simplified _libssh2_check_length #350

merged 2 commits into from Apr 5, 2019

Conversation

willco007
Copy link
Member

misc.c : _libssh2_check_length()

Removed cast and one-lined check.

Credit : Yuriy M. Kaminskiy

misc.c : _libssh2_check_length()

Removed cast and one-lined check. 

Credit : Yuriy M. Kaminskiy
src/misc.c Outdated
return 0;

return ((int)(buf->dataptr - buf->data) <= (int)(buf->len - len)) ? 1 : 0;
return (len <= (size_t)((buf->data + buf->len) - buf->dataptr));
Copy link
Contributor

Choose a reason for hiding this comment

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

This weakens the check in case buf->dataptr is already behind buf->data + buf->len because when ((buf->data + buf->len) - buf->dataptr) is negative, the conversion to size_t turns it into a big positive number.

Copy link
Member Author

@willco007 willco007 Apr 4, 2019

Choose a reason for hiding this comment

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

It theoretically does, but dataptr should be greater than or equal to data if the API is used correctly (famous last words, I know). Also, in the original case if dataptr was less than data it would be a negative number which would be less than the test and incorrectly return true, which is also bad. Furthermore, the cast to a signed value from unsigned isn't great and could loose precision.

Copy link
Member

Choose a reason for hiding this comment

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

I think the expression could be split up in several parts to become more readable and then the logic is easier to follow and confirm. Something like:

 char *endp = &buf->data[buf->len];
 size_t left = endp - buf->dataptr;
 return (len <= left);

it could even protect against the wrap-around case @kdudka mentioned:

 char *endp = &buf->data[buf->len];
 size_t left = endp - buf->dataptr;
 return ((len <= left) && (left <= buf->len));

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks good to me. I'll go ahead and make the change to @bagder's last suggestion.

Updated suggested patch to protect against incorrect usage which could cause a wrap-around value to return success.
@willco007 willco007 requested a review from kdudka April 4, 2019 21:30
Copy link
Contributor

@kdudka kdudka left a comment

Choose a reason for hiding this comment

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

Looks good.

@willco007 willco007 merged commit ff1b155 into master Apr 5, 2019
@willco007 willco007 deleted the willco007-patch-1 branch April 5, 2019 16:46
@carnil
Copy link

carnil commented Jul 17, 2019

https://blog.semmle.com/libssh2-integer-overflow/ is related to this pull request.

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

4 participants