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

urlapi: avoid index underflow for short ipv6 hostnames #4389

Closed

Conversation

@pauldreik
Copy link
Contributor

commented Sep 20, 2019

See

curl/lib/urlapi.c

Lines 596 to 604 in a89aeb5

size_t hlen = strlen(hostname);
if(hostname[0] == '[') {
char dest[16]; /* fits a binary IPv6 address */
const char *l = "0123456789abcdefABCDEF:.";
hostname++;
hlen -= 2;
if(hostname[hlen] != ']')

If the input hostname is "[",
hlen will underflow to max of size_t when it is subtracted with 2 at line 602.

hostname[hlen] at line 604 will then cause a warning by ubsanitizer:

runtime error: addition of unsigned offset to 0x........ overflowed to 0x............

I think that in practice, the generated code will work, and the output
of hostname[hlen] will be the first character "[".

This can be demonstrated by the following program (tested in both clang and gcc,
with -O3)

#include <string.h>
#include <stdlib.h>
#include <stdio.h>
int main() {
char* hostname=strdup("[");
size_t hlen = strlen(hostname);

hlen-=2;
hostname++;
printf("character is %d\n",+hostname[hlen]);
free(hostname-1);
}

I found this through fuzzing, and even if it seems harmless, the proper
thing is to return early with an error.

If the input hostname is "[",
hlen will underflow to max of size_t when it is subtracted with 2.

hostname[hlen] will then cause a warning by ubsanitizer:

runtime error: addition of unsigned offset to 0x<snip> overflowed to 0x<snip>

I think that in practice, the generated code will work, and the output
of hostname[hlen] will be the first character "[".

This can be demonstrated by the following program (tested in both clang and gcc,
with -O3)

#include <string.h>
#include <stdlib.h>
#include <stdio.h>
int main() {
  char* hostname=strdup("[");
  size_t hlen = strlen(hostname);

  hlen-=2;
  hostname++;
  printf("character is %d\n",+hostname[hlen]);
  free(hostname-1);
}


I found this through fuzzing, and even if it seems harmless, the proper
thing is to return early with an error.
@bagder
bagder approved these changes Sep 20, 2019
@bagder
bagder approved these changes Sep 20, 2019
@@ -598,6 +598,8 @@ static CURLUcode hostname_check(struct Curl_URL *u, char *hostname)
if(hostname[0] == '[') {
char dest[16]; /* fits a binary IPv6 address */
const char *l = "0123456789abcdefABCDEF:.";
if(hlen <= 2)

This comment has been minimized.

Copy link
@bagder

bagder Sep 20, 2019

Member

I clicked approve and then immediately it struck me. I can't think of any IPv6 address shorter than 3 letters (::1) so maybe this can abort some illegal strings and be < 5 ?

This comment has been minimized.

Copy link
@pauldreik

pauldreik Sep 20, 2019

Author Contributor

Sounds reasonable, but why is the entire code block not wrapped in the #ifdef ENABLE_IPV6 ? I guess the correct reply if enable_ipv6 is false, is to reject anything starting with[ anyway?

This comment has been minimized.

Copy link
@bagder

bagder Sep 20, 2019

Member

Well, first that's a really rare configuration these days so I'm not sure we need to bother too much about that, but possibly more importantly the URL parser API should probably be able to extract host names even if curl itself can't actually connect to that host.

@bagder bagder closed this in 4706603 Sep 21, 2019
@bagder

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.