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

Add test for HTTP header Host in redirects #318

Merged
merged 4 commits into from Feb 5, 2018

Conversation

timopollmeier
Copy link
Contributor

The "Host" HTTP header used in redirects now has to refer to a known
host name or address, which by default includes 127.0.0.1, ::1,
localhost and either all addresses given by the --listen option or
all local interface addresses.
Additional host names or addresses can be given with the new command
line option --allow-header-host.

The "Host" HTTP header used in redirects now has to refer to a known
host name or address, which by default includes 127.0.0.1, ::1,
localhost and either all addresses given by the --listen option or
all local interface addresses.
Additional host names or addresses can be given with the new command
line option --allow-header-host.
src/gsad.c Outdated
case 2:
default:
// Otherwise invalid
send_response (connection, BAD_REQUEST_PAGE, MHD_HTTP_NOT_ACCEPTABLE,
Copy link
Member

@cfi-gb cfi-gb Feb 1, 2018

Choose a reason for hiding this comment

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

For improved UX i wouldn't throw a bad request around if the user just accessed the system via a wrong Hostname/IP. If the accepted host headers are just misconfigured the user will never know directly why he gets a bad request when accessing the system.

A way better approach would be to throw a 403 / Forbidden in the case if a valid but not allowed Hostname/IP is used and include a short note in the returned HTML document explaining how to configure the system in a way to be able to access it.

Copy link
Member

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

The big gsad.c is still a mess to review...
Your code looks valid overall. But I would like to get some stuff extracted into separate functions.

src/gsad.c Outdated
else if (g_utf8_validate (host_header, -1, NULL) == FALSE)
return 1;

bracket_index = -1;
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this code in the for loop. Maybe it would be better to extract it into a function or at least at a comment.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!!!

src/gsad.c Outdated

bracket_index = -1;
colon_index = -1;
for (char_index = strlen (host_header) - 1;
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this code. Maybe you can extract it into another function or at least add some comment.

@@ -3828,6 +3833,120 @@ gsad_add_content_type_header (struct MHD_Response *response,
}
}

/**
* @brief Add all local IP addresses to a GHashTable.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't include localhost, 127.x.x.x and the ipv6 local addresses right? So the description is a bit confusing.
Maybe move adding the localhost addresses to the hashmap also into this function.

@@ -6402,6 +6538,14 @@ main (int argc, char **argv)
exit (EXIT_FAILURE);
}

/* Initialize addresses and accepted host headers for HTTP header */
Copy link
Member

Choose a reason for hiding this comment

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

Adding the localhost addresses should be moved into a function.

The default loopback addresses localhost, 127.0.0.1 and ::1 are now
added in add_local_addresses, which now also has an option not to add
the other interface addresses.
Also, add_local_addresses is now a separate step from gsad_address_init,
which only adds the explicit host addresses to the ones allowed in
the Host header.
In validate_host_header comments have been added to explain the cases in
extracting the host address or name from the header value and some of
the conditions have been made clearer.
If an invalid or unknown Host header is detected, respond with a more
specific error message.
Copy link
Member

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

Nice! Thanks a lot. It's much easier to understand now.

src/gsad.c Outdated
else if (g_utf8_validate (host_header, -1, NULL) == FALSE)
return 1;

bracket_index = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!!!

@bjoernricks
Copy link
Member

If @cfi-gb is fine with the additional changes I would merge it instantly.

@cfi-gb
Copy link
Member

cfi-gb commented Feb 2, 2018

@bjoernricks I'm still unsure about the MHD_HTTP_BAD_REQUEST vs. MHD_HTTP_FORBIDDEN in the case where the GSA is misconfigured / doesn't have the expected allowed header configured. Because in this case the client isn't doing a "Bad Request" but just not allowed to access the GSA.

The text / html is IMHO fine for now. It might be linked to the online documentation in the future but i think that is out of the scope of this PR.

@bjoernricks
Copy link
Member

@cfi-gb choosing the http status code is difficult indeed. I would got for bad request because it is technically correct.

@bjoernricks bjoernricks merged commit fff55ca into greenbone:gsa-7.0 Feb 5, 2018
@timopollmeier timopollmeier deleted the check-host-header-7.0 branch February 8, 2018 17:02
This was referenced Apr 5, 2018
@bjoernricks
Copy link
Member

Next time we should ensure that such a PR is mentioned in the release notes explicitly. A lot of people are not aware of it like http://lists.wald.intevation.org/pipermail/openvas-discuss/2018-April/011929.html

@cfi-gb
Copy link
Member

cfi-gb commented Jul 1, 2021

It seems CVE-2018-25016 has been assigned to this recently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants