Headline

CVE-2019-13115: Simplified _libssh2_check_length by willco007 · Pull Request #350 · libssh2/libssh2

In libssh2 before 1.9.0, kex_method_diffie_hellman_group_exchange_sha256_key_exchange in kex.c has an integer overflow that could lead to an out-of-bounds read in the way packets are read from the server. A remote attacker who compromises a SSH server may be able to disclose sensitive information or cause a denial of service condition on the client system when a user connects to the server. This is related to an _libssh2_check_length mistake, and is different from the various issues fixed in 1.8.1, such as CVE-2019-3855.

5 years ago

CVE

Open in Source

#dos #git

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

Conversation 6 Commits 2 Checks 0 Files changed

Conversation

@willco007

misc.c : _libssh2_check_length()

Removed cast and one-lined check.

Credit : Yuriy M. Kaminskiy

@willco007

misc.c : _libssh2_check_length()

Removed cast and one-lined check.

Credit : Yuriy M. Kaminskiy

kdudka

return 0;

return ((int)(buf->dataptr - buf->data) <= (int)(buf->len - len)) ? 1 : 0;

return (len <= (size_t)((buf->data + buf->len) - buf->dataptr));

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.

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.

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));

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.

@willco007

Updated suggested patch to protect against incorrect usage which could cause a wrap-around value to return success.

kdudka