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

Fix numerous message parsing issues. #3059

Closed
wants to merge 5 commits into from
Closed

Fix numerous message parsing issues. #3059

wants to merge 5 commits into from

Conversation

kenballus
Copy link
Contributor

This patch changes the set of allowed characters in header names and values to match RFC 9110.

It also updates chunk length parsing to stop the erroneous parsing of chunk lengths containing '_' and '0x'.

The relevant rules from RFCs 9110, 9112, and 5234:

ALPHA         = %x41-5A / %x61-7A
HEXDIG        = DIGIT / "A" / "B" / "C" / "D" / "E" / "F"
DIGIT         = %x30-39
VCHAR         = %x21-7E
...
field-name    = token
token         = 1*tchar
tchar         = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
...
field-value   = *field-content
field-content = field-vchar [ 1*( SP / HTAB / field-vchar ) field-vchar ]
field-vchar   = VCHAR / obs-text
obs-text      = %x80-FF
...
chunk-size    = 1*HEXDIG

…length parsing to disallow 0x prefix and digit-separating underscores.
@kenballus kenballus mentioned this pull request Sep 1, 2023
@kenballus
Copy link
Contributor Author

This PR now also fixes #3104.

@kenballus kenballus changed the title Update allowed set of characters in headers and chunk lengths Update allowed set of characters in headers and chunk lengths; disallow empty header names Dec 4, 2023
- Replace CRLF with SP during obs-fold processing (See RFC 9112 Section 5.2, last paragraph)
- Stop stripping header names.
- Remove HTAB in OWS in header values that use obs-fold (See RFC 9112 Section 5.2, last paragraph)
- Use fullmatch instead of search, which has problems with empty strings. (See GHSA-68xg-gqqm-vgj8)
- Split proxy protocol line on space only. (See proxy protocol Section 2.1, bullet 3)
- Use fullmatch for method and version (Thank you to Paul Dorn for noticing this.)
- Replace calls to str.strip() with str.strip(' \t')
- Split request line on SP only.

Co-authored-by: Paul Dorn <[email protected]>
@kenballus kenballus changed the title Update allowed set of characters in headers and chunk lengths; disallow empty header names Fix numerous message parsing issues. Dec 6, 2023
@kenballus
Copy link
Contributor Author

Just added a new patch that addresses a few more issues:

  • Unify HEADER_RE and METH_RE (Thank you to Paul Dorn for noticing this.)
  • Replace CRLF with SP during obs-fold processing (See RFC 9112 Section 5.2, last paragraph)
  • Stop stripping header names.
  • Remove HTAB in OWS in header values that use obs-fold (See RFC 9112 Section 5.2, last paragraph)
  • Use fullmatch instead of search, which has problems with empty strings. (See GHSA-68xg-gqqm-vgj8)
  • Split proxy protocol line on space only. (See proxy protocol Section 2.1, bullet 3)
  • Use fullmatch for method and version (Thank you to Paul Dorn for noticing this.)
  • Replace calls to str.strip() with str.strip(" \t")
  • Split request line on SP only.

Copy link
Owner

@benoitc benoitc left a comment

Choose a reason for hiding this comment

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

I think it's OK to merge. Can you clarify why you removed the check of the line length?

@pajod
Copy link
Contributor

pajod commented Dec 7, 2023

@benoitc Just in case I chose the wrong email or octocat thingy earlier: I do invite you to have a look at these closely related patches on top which I did not feel appropriate to publish before you had a chance to commit to or decline the review/documentation/maintenance work that should go with them.

@benoitc
Copy link
Owner

benoitc commented Dec 7, 2023

@benoitc Just in case I chose the wrong email or octocat thingy earlier: I do invite you to have a look at these closely related patches on top which I did not feel appropriate to publish before you had a chance to commit to or decline the review/documentation/maintenance work that should go with them.

Looking at them thanks :)

@benoitc
Copy link
Owner

benoitc commented Dec 12, 2023

@pajod please add the commits to this PR .Let's merge it :)

@pajod
Copy link
Contributor

pajod commented Dec 12, 2023

@benoitc Since you specifically asked for it, I opened #3113 with rebased version of the commits previously shared privately.

I still highly recommend this is bundled with opting into some Github features (private PRs, issue templates, ..), better testing, explicitly notifying affected users and provision of carefully drafted advisories, ideally linked here.

You already have access to a suggested advisory draft (3-4 such might be appropriate to address issues with varying impact) and a list of parties I would like to work with in testing the release.

@tilgovi
Copy link
Collaborator

tilgovi commented Dec 27, 2023

Is this covered by #3113 and closeable now?

@kenballus
Copy link
Contributor Author

Yes.

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

Successfully merging this pull request may close these issues.

4 participants