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

Issue #8716 - Handle bad host/authority headers better #8717

Merged
merged 13 commits into from
Nov 9, 2022

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 14, 2022

This is a minimal version of code to reject bad Host/authority header usage.

The more robust validation can be found in the closed (and never merged) PR #7279

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime added the Bug For general bugs on Jetty side label Oct 14, 2022
@joakime joakime requested review from gregw and sbordet October 14, 2022 19:08
@joakime joakime self-assigned this Oct 14, 2022
…t headers

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime added the Specification For all industry Specifications (IETF / Servlet / etc) label Oct 17, 2022
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from sbordet October 25, 2022 21:07
@sbordet
Copy link
Contributor

sbordet commented Oct 26, 2022

@joakime @gregw I'm on the fence about this PR.

I think we should fix the double Host header, but we should not attempt to do any check or parsing of the Host value besides validity of host:port format (already done by HostPort).

If someone sends Host: a, b, c then the authority is a, b, c and so we report.

Looking for , or doing QuotedCSV parsing, etc. I think it's too much with no additional benefit and a possible performance impact on all the valid cases.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Looks good... but we can make the isRegName more efficient with a lookup table.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

static
{
REGNAME_ALLOWED = new boolean[127];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 128.

REGNAME_ALLOWED[c] = true;
}
}

// Use UTF-8 as per http://www.w3.org/TR/html40/appendix/notes.html#non-ascii-chars
public static final Charset __CHARSET = StandardCharsets.UTF_8;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make private and possibly remove the constant altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, lemme do this in a different PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, can't do this for Jetty 10.0.x (where this PR is).

BUT I will do it for Jetty 12.0.x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took care of this (along with other URIUtil cleanup) in jetty-12.0.x PR #8861

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from sbordet November 3, 2022 15:28
@joakime joakime merged commit 793bee9 into jetty-10.0.x Nov 9, 2022
@joakime joakime deleted the fix/jetty-10-invalid-host-headers branch November 9, 2022 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Specification For all industry Specifications (IETF / Servlet / etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants