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

url: reject ASCII control characters and space in host names #2092

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
2 participants
@bagder
Copy link
Member

commented Nov 17, 2017

Host names like "127.0.0.1 moo" would otherwise be accepted by some
getaddrinfo() implementations.

Fixes #2073

@bagder

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2017

This is an alternative take to #2073, and as a bonus it also checks proxy host names etc. No test in here, but @mkauf's test from #2073 should be able to verify this approach as well.

@mkauf
Copy link
Contributor

left a comment

We could push both PRs (#2073, #2092) - they fix the problem at different levels. But maybe that's overkill...?

Now that fix_hostname() has a return value, it could also report failed IDN conversions. What do you think?

lib/url.c Outdated
goto out;
if(conn->bits.conn_to_host) {
result = fix_hostname(conn, &conn->conn_to_host);
goto out;

This comment has been minimized.

Copy link
@mkauf

mkauf Nov 17, 2017

Contributor
goto out;

... must be changed to:

if(result)
  goto out;
lib/url.c Outdated
}
if(conn->bits.httpproxy) {
result = fix_hostname(conn, &conn->http_proxy.host);
goto out;

This comment has been minimized.

Copy link
@mkauf

mkauf Nov 17, 2017

Contributor

same problem here

lib/url.c Outdated
}
if(conn->bits.socksproxy) {
result = fix_hostname(conn, &conn->socks_proxy.host);
goto out;

This comment has been minimized.

Copy link
@mkauf

mkauf Nov 17, 2017

Contributor

same problem here

This comment has been minimized.

Copy link
@bagder

bagder Nov 17, 2017

Author Member

I clearly was a bit too eager to get something to show...

@bagder

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2017

it could also report failed IDN conversions. What do you think?

Oh yes! I'll add that too when I fix my initial mistakes.

@bagder bagder force-pushed the bagder/hostname-reject-control-characters branch from c767b0f to d34a68f Nov 17, 2017

fixed

@bagder

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2017

Hah, the new IDN error changes the error for a few test cases so I need to clean those up...

url: reject ASCII control characters and space in host names
Host names like "127.0.0.1 moo" would otherwise be accepted by some
getaddrinfo() implementations.

Updated test 1034 and 1035 accordingly.

Fixes #2073

@bagder bagder force-pushed the bagder/hostname-reject-control-characters branch from d34a68f to 0c2470e Nov 20, 2017

@bagder

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2017

Test 1264 was now cherry-picked into this branch from #2073

@mkauf

mkauf approved these changes Nov 21, 2017

@bagder bagder closed this in fa93922 Nov 22, 2017

@bagder bagder deleted the bagder/hostname-reject-control-characters branch Nov 22, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.