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

vtls: fix potential version string overflow #3863

Closed

Conversation

@danielgustafsson
Copy link
Member

commented May 10, 2019

In Curl_multissl_version() it was possible to overflow the passed in buffer if the generated version string exceeded the size of the buffer. Fix by inverting the logic, and also make sure to not exceed the local buffer during the string generation.

Reported-by: nevv on HackerOne/curl

@bagder
bagder approved these changes May 10, 2019
@@ -1243,7 +1243,7 @@ static size_t Curl_multissl_version(char *buffer, size_t size)

selected = current;

for(i = 0; available_backends[i]; i++) {
for(i = 0; available_backends[i] && p < backends + sizeof(backends); i++) {

This comment has been minimized.

Copy link
@jay

jay May 10, 2019

Member

You'll have to account for each time p is incremented. How about

  if(current != selected) {
    char *p = backends;
    char *end = backends + sizeof(backends);
    int i;

    selected = current;

    for(i = 0; available_backends[i] && p < end - 4; i++) {
      if(i)
        *(p++) = ' ';
      if(selected != available_backends[i])
        *(p++) = '(';
      p += available_backends[i]->version(p, end - p - 2);
      if(selected != available_backends[i])
        *(p++) = ')';
    }
    *p = '\0';
    total = p - backends;
  }

Also I suggest increase it to like 1k size, 200 seems low for multiple versions.

This comment has been minimized.

Copy link
@jay

jay May 10, 2019

Member

also what is supposed to happen here if(current==selected)?

This comment has been minimized.

Copy link
@bagder

bagder May 11, 2019

Member

If we need to increase the backends buffer, or perhaps independently of that, we should also increase the version and the ssl_version buffers that are used to get this output (in lib/version.c) - both of them are probably in the smaller end if someone builds a curl with all possible TLS backends enabled (not sure exactly how many and which that might be).

This comment has been minimized.

Copy link
@danielgustafsson

danielgustafsson May 11, 2019

Author Member

also what is supposed to happen here if(current==selected)?

It's keeping the buffer cached in case the current TLS backend hasn't changed between calls, as the selectdt backend is printed differently from the rest.

This comment has been minimized.

Copy link
@danielgustafsson

danielgustafsson May 11, 2019

Author Member

You'll have to account for each time p is incremented.

Good catch, fixing.

Also I suggest increase it to like 1k size, 200 seems low for multiple versions.

I'm not sure 200 characters is too low, and 1k seems like excessive stack usage, but there are definitely buffers in the version printing which needs to get bumped. That's will be a separate PR though.

This comment has been minimized.

Copy link
@bagder

bagder May 12, 2019

Member

I would imagine 200 should be enough. If you manage to build with 6 backends, that makes it 33 bytes per backend, and I don't think any backend uses that much. 8 backends give 25 bytes each, probably also plenty enough.

I doubt we can build with more than 8, if even that...

This comment has been minimized.

Copy link
@jay

jay May 12, 2019

Member

Good catch, fixing.

I've added p += available_backends[i]->version(p, end - p - 2); since it wasn't in the fix. Technically if bytes written returned is always going to be less than the count passed (eg version(foo, 5) and therefore the max it will return is 4 bec it accounts for the null) you only need to subtract 1 but I think subtracting 2 makes the intention clearer.

memcpy(buffer, backends, total + 1);
else {
memcpy(buffer, backends, size - 1);
buffer[size - 1] = '\0';
}

return total;
return CURLMIN(size, total);

This comment has been minimized.

Copy link
@jay

jay May 10, 2019

Member

size - 1

This comment has been minimized.

Copy link
@danielgustafsson

danielgustafsson May 11, 2019

Author Member

Ah, yes.

In Curl_multissl_version() it was possible to overflow the passed in
buffer if the generated version string exceeded the size of the buffer.
Fix by inverting the logic, and also make sure to not exceed the local
buffer during the string generation.

Closes #xxxx
Reported-by: nevv on HackerOne/curl
@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-sslbuf_stackoverflow branch from 8bbfa52 to 629f4c6 May 11, 2019
@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

Pushed a rebased fixup version with the comments addressed.

danielgustafsson added a commit to danielgustafsson/curl that referenced this pull request May 12, 2019
When running a multi TLS backend build the version string needs more
buffer space. Make the internal ssl_buffer stack buffer match the one
in Curl_multissl_version() to allow for the longer string. For single
TLS backend builds there is no use in extended to buffer. This is a
fallout from curl#3863 which fixes up the multi_ssl string generation to
avoid a buffer overflow when the buffer is too small.
@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Squashed and pushed this to master, thanks for review!

danielgustafsson added a commit that referenced this pull request May 19, 2019
When running a multi TLS backend build the version string needs more
buffer space. Make the internal ssl_buffer stack buffer match the one
in Curl_multissl_version() to allow for the longer string. For single
TLS backend builds there is no use in extended to buffer. This is a
fallout from #3863 which fixes up the multi_ssl string generation to
avoid a buffer overflow when the buffer is too small.

Closes #3875
Reviewed-by: Daniel Stenberg <daniel@haxx.se>
if(i)
*(p++) = ' ';
if(selected != available_backends[i])
*(p++) = '(';
p += available_backends[i]->version(p, backends + sizeof(backends) - p);
p += available_backends[i]->version(p, end - p - 2);

This comment has been minimized.

Copy link
@dscho

dscho May 23, 2019

Contributor

Shouldn't this be an end - p > 2 ? end - p - 2 : 0? The second parameter is declared as size_t, i.e. unsigned...

This comment has been minimized.

Copy link
@jay

jay May 24, 2019

Member

end - p is always > 2 at that point, refer to the loop condition p < (end - 4), then after that p can be incremented twice before end - p - 2

This comment has been minimized.

Copy link
@danielgustafsson

danielgustafsson May 24, 2019

Author Member

Right, unless I'm missing something @jay is right, the current coding should be safe (but it might warrant a comment explaining it).

This comment has been minimized.

Copy link
@jay

jay May 24, 2019

Member

I find adding a comment to be unnecessary I think it's pretty straightforward.

This comment has been minimized.

Copy link
@jzakrzewski

jzakrzewski May 24, 2019

Contributor

Well, one could just write the loop condition as ...&& (end - p) > 4 and it'd be easier to read

This comment has been minimized.

Copy link
@dscho

dscho May 25, 2019

Contributor

I find adding a comment to be unnecessary I think it's pretty straightforward.

So you're saying that I am dumb. Because I did not see that.

I don't take this personally, coming from you, as I had pleasant and productive interactions with you in the past, but I wonder whether you realized how unfortunate a reaction your comment would elicit (and I think it is pretty obvious that it would do exactly that). Might want to be more careful.

In short: while it might be obvious to you, now, I think that my comment is testament enough to give evidence to the contrary. And who knows, maybe you yourself will belong into the same "wait, what?" camp in 6 months from now.

This comment has been minimized.

Copy link
@bagder

bagder May 25, 2019

Member

I'm in @dscho's corner here. When I first saw the question here about this line of code it was not immediately obvious to me that it was right. A comment could've remedied that both for @dscho and for me. For those who don't need the comment, it won't hurt (much) anyway.

This comment has been minimized.

Copy link
@jay

jay May 26, 2019

Member

I find adding a comment to be unnecessary I think it's pretty straightforward.

So you're saying that I am dumb. Because I did not see that.

I don't take this personally, coming from you, as I had pleasant and productive interactions with you in the past, but I wonder whether you realized how unfortunate a reaction your comment would elicit (and I think it is pretty obvious that it would do exactly that). Might want to be more careful.

In short: while it might be obvious to you, now, I think that my comment is testament enough to give evidence to the contrary. And who knows, maybe you yourself will belong into the same "wait, what?" camp in 6 months from now.

Whoa I may not be very tactful but don't put words in my mouth. I figured you misread the code, like I do occasionally. I would've shared my opinion even if that wasn't the case. If you had a different opinion that's all you needed to say in your reply.

Anyway, clearly my opinion of it being straightforward is not in favor. Someone propose a comment so we can put this to bed. /* end - p is > 2 at this point */ ?

@lock lock bot locked as resolved and limited conversation to collaborators Aug 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.