Skip to content

Commit

Permalink
Curl_auth_create_plain_message: fix too-large-input-check
Browse files Browse the repository at this point in the history 
CVE-2018-16839
Reported-by: Harry Sintonen
Bug: https://curl.haxx.se/docs/CVE-2018-16839.html
  • Loading branch information
@bagder
bagder committed Oct 29, 2018
1 parent 81d135d commit f3a24d7
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion lib/vauth/cleartext.c
View file
Expand Up @@ -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;

Expand Down

3 comments on commit f3a24d7

@thoger
Copy link
Contributor

@thoger thoger commented on f3a24d7 Nov 14, 2018

Choose a reason for hiding this comment

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

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
Copy link
Member Author

@bagder bagder commented on f3a24d7 Nov 15, 2018

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@thoger thoger commented on f3a24d7 Nov 15, 2018

Choose a reason for hiding this comment

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

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.