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

vauth/cleartext: fix integer overflow check #2408

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Mar 20, 2018

Make the integer overflow check not rely on the undefined behavior that
a size_t wraps around on overflow.

Detected by lgtm.com

Make the integer overflow check not rely on the undefined behavior that
a size_t wraps around on overflow.

Detected by lgtm.com
@bagder bagder closed this in c136657 Mar 20, 2018
@bagder bagder deleted the bagder/cleartext-fix-overflow-check branch March 20, 2018 18:26
@jay
Copy link
Member

jay commented Mar 20, 2018

I use that overflow check pattern all the time, how is that undefined? size_t should always be unsigned unless gcc in early 90s maybe where they didn't follow the standard exactly. I think that checker is too sensitive. What about a username and password length check some small value like 1k instead of some chunk of size_t max

@bagder
Copy link
Member Author

bagder commented Mar 20, 2018

Ah yes, I was a bit "blinded" by the warning so I didn't think properly. It actually shouldn't be undefined, no...

What about a username and password length check some small value like 1k instead of some chunk of size_t max

Yeah, I think that would make sense and would actually probably help to detect errors earlier and better...

@jay
Copy link
Member

jay commented Mar 23, 2018

ok how about this

diff --git a/lib/vauth/cleartext.c b/lib/vauth/cleartext.c
index 5d61ce6..b9a9be0 100644
--- a/lib/vauth/cleartext.c
+++ b/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 > 1024 || plen > 1024)
     return CURLE_OUT_OF_MEMORY;
   plainlen = 2 * ulen + plen + 2;
 

@lock lock bot locked as resolved and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants