Permalink
Browse files

Curl_auth_create_plain_message: fix too-large-input-check

CVE-2018-16839
Reported-by: Harry Sintonen
Bug: https://curl.haxx.se/docs/CVE-2018-16839.html
  • Loading branch information...
bagder committed Sep 28, 2018
1 parent 81d135d commit f3a24d7916b9173c69a3e0ee790102993833d6c5
Showing with 1 addition and 1 deletion.
  1. +1 −1 lib/vauth/cleartext.c
@@ -74,7 +74,7 @@ CURLcode Curl_auth_create_plain_message(struct Curl_easy *data,
plen = strlen(passwdp);
/* Compute binary message length. Check for overflows. */
if((ulen > SIZE_T_MAX/2) || (plen > (SIZE_T_MAX/2 - 2)))
if((ulen > SIZE_T_MAX/4) || (plen > (SIZE_T_MAX/2 - 2)))
return CURLE_OUT_OF_MEMORY;
plainlen = 2 * ulen + plen + 2;

3 comments on commit f3a24d7

@thoger

This comment has been minimized.

Contributor

thoger replied Nov 14, 2018

Is there a reason why this integer overflow check is written the way it is? It defines limits that are lower than what they really need to be. For example, if you have 10 character user name, you can still have 3GB password without triggering integer overflow, or you can have 1.5GB username and 4 character password.

In a case like this, I would rather expect a check like this:

if ((plen > SIZE_T_MAX - 2)  ||  (ulen > (SIZE_T_MAX - 2 - plen)/2))

Also a note regarding this advisory wording:

On systems with a 32 bit size_t, the math to calculate the buffer size triggers an integer overflow when the user name length exceeds 2GB (2^31 bytes).

That's likely a re-use of a wording from an older advisory that is not applicable here. Even prior to this fix, user name lengths exceeding 2GB were rejected, and the fix further lowers the limit to 1GB.

@bagder

This comment has been minimized.

Member

bagder replied Nov 15, 2018

Is there a reason why this integer overflow check is written the way it is? It defines limits that are lower than what they really need to be.

I just went with a simple fix. It could certainly be improved, but I didn't see any real point in reworking the check as surely a 1GB user name length and 2GB password should be enough for 32bit systems?

That's likely a re-use of a wording from an older advisory that is not applicable here

Very true. I'll adjust that wording!

@thoger

This comment has been minimized.

Contributor

thoger replied Nov 15, 2018

I agree that 1GB/2GB are ways from what is going to be needed on any system, 32bit or not. I expect the limit based on the real world requirements is going to be closer to the old default of MAX_CURL_USER_LENGTH / MAX_CURL_PASSWORD_LENGTH.

I'm also wondering if any PoC was provided for this, or a hint on a path to trigger this issue? To trigger the overflow, this requires input that is longer than SIZE_T_MAX/2 (user name at or slightly below SIZE_T_MAX/2, and few bytes for password). The user name and password passed to Curl_auth_create_plain_message comes form conndata struct. Setting them there seems to be set_login()'s job. However, set_login() copies / strdups both of them, so at the end of that function user name and password needs to be in memory in two copies, which is not possible if single copy requires more than SIZE_T_MAX/2 of space. Also what's passed to set_login() already is a copy of data also stored elsewhere in the memory (with copy created in parseurlandfillconn() or override_login()).

So it does not seem triggerable when using libcurl APIs.

Please sign in to comment.