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

ftp_range: avoid integer overflow when figuring out byte range #2205

Closed
wants to merge 3 commits into from

Conversation

cmeister2
Copy link
Contributor

@cmeister2 cmeister2 commented Dec 31, 2017

When trying to bump the value with one and the value is already at max,
it causes an integer overflow.

Detected by oss-fuzz:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=4853

@cmeister2
Copy link
Contributor Author

cmeister2 commented Jan 2, 2018

Travis issues are CI related and not code related.

lib/ftp.c Outdated
data->req.maxdownload = (to - from) + 1; /* include last byte */
totalsize = to-from;
if(totalsize == CURL_OFF_T_MAX)
/* this is too big to increase, so bail out */
Copy link
Member

@jay jay Jan 4, 2018

Choose a reason for hiding this comment

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

I would get rid of this comment it's implied. also you can declare totalsize in this block. also since we're here this could use a check to make sure from <= to

Copy link
Contributor Author

@cmeister2 cmeister2 Jan 4, 2018

Choose a reason for hiding this comment

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

This code is basically a copy of the code from https://github.com/curl/curl/blob/master/lib/file.c#L166 that @bagder wrote; should we be updating both places?

Copy link
Member

@bagder bagder Jan 4, 2018

Choose a reason for hiding this comment

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

We could, but I'd rather not lump a change of file.c into this fix as it isn't really related to this FTP range integer overflow...

Copy link
Contributor Author

@cmeister2 cmeister2 Jan 15, 2018

Choose a reason for hiding this comment

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

The reason I ask is that you made a change to file_range to fix a bug, file_range has the comment

 /*
  Check if this is a range download, and if so, set the internal variables
  properly. This code is copied from the FTP implementation and might as
  well be factored out.
 */

and the ftp_range function still has the issue. So my assumption is that any fixes (or markups) that need to be made in one place should be made in the other as well.

Copy link
Member

@bagder bagder Jan 15, 2018

Choose a reason for hiding this comment

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

Let's turn file_range() into Curl_range() and make both the FILE and FTP code use it.

The only difference I can spot is that ftp_range() sets ftpc->dont_check = TRUE when a range has been set, so we should make sure to set that separately.

What do you think?

Copy link
Contributor Author

@cmeister2 cmeister2 Jan 15, 2018

Choose a reason for hiding this comment

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

Happy with that. Do you want to run with that or shall I?

Copy link
Member

@bagder bagder Jan 15, 2018

Choose a reason for hiding this comment

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

Please go ahead, I have a few other tasks to work on anyway! =)

lib/curl_range.c Outdated
#if !defined(CURL_DISABLE_FTP) && !defined(CURL_DISABLE_FILE)

/*
* Curl_memrchr()
Copy link
Contributor Author

@cmeister2 cmeister2 Jan 29, 2018

Choose a reason for hiding this comment

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

Sigh. todo: remove this

lib/curl_range.c Outdated

/*
Check if this is a range download, and if so, set the internal variables
properly. This code is copied from the FTP implementation and might as
Copy link
Contributor Author

@cmeister2 cmeister2 Jan 29, 2018

Choose a reason for hiding this comment

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

And update this description...

@cmeister2
Copy link
Contributor Author

cmeister2 commented Jan 29, 2018

Build looks fine apart from the MacOS failures which are known about.

lib/file.c Outdated
@@ -514,7 +456,7 @@ static CURLcode file_do(struct connectdata *conn, bool *done)
}

/* Check whether file range has been specified */
file_range(conn);
Curl_range(conn);
Copy link
Member

@bagder bagder Jan 30, 2018

Choose a reason for hiding this comment

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

check return code?

bagder
bagder approved these changes Jan 30, 2018
@bagder
Copy link
Member

bagder commented Jan 30, 2018

Hm, I'm not sure how github interpreted that but I think there should be a check of the return code but otherwise the change looks good!

@cmeister2
Copy link
Contributor Author

cmeister2 commented Jan 30, 2018

https://github.com/curl/curl/blob/master/lib/file.c#L517 in master doesn't do this check; is this something new that should be done?

@bagder
Copy link
Member

bagder commented Jan 30, 2018

It's not new, but that's a flaw we can just as well correct while fixing up this code I think.

@cmeister2
Copy link
Contributor Author

cmeister2 commented Jan 30, 2018

What's the desired behaviour on checking the return code? Anything other than CURLE_OK is a failure and should stop the transfer?

@bagder
Copy link
Member

bagder commented Jan 30, 2018

Right, the common pattern is to just:

   result = Curl_range(....);
   if(result)
     return result;

@cmeister2
Copy link
Contributor Author

cmeister2 commented Jan 30, 2018

Ok, done - hope that passes the tests!

bagder
bagder approved these changes Jan 30, 2018
@bagder
Copy link
Member

bagder commented Jan 30, 2018

thanks!

@bagder bagder closed this in e04417d Jan 30, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants