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

[Python] mrhttp: a project with many problems #9055

Open
remittor opened this issue May 24, 2024 · 4 comments
Open

[Python] mrhttp: a project with many problems #9055

remittor opened this issue May 24, 2024 · 4 comments

Comments

@remittor
Copy link
Contributor

remittor commented May 24, 2024

Fields in HTTP responses

Date field in responses is missing or does not meet requirements.

https://github.com/MarkReedZ/mrhttp/blob/fb0870254016dae2d224915f634448c395914e0d/src/mrhttp/internals/protocol.c#L660-L702

HTTP header parser

HTTP parser is sure that untruncated data came from the TCP stream

https://github.com/MarkReedZ/mrhttp/blob/fb0870254016dae2d224915f634448c395914e0d/src/mrhttp/internals/mrhttpparser.c#L170-L188

Function get_len_to_space searches the buffer for the nearest space and returns the length. Yes, but this space may be missing, since it may be in the next TCP package. But in this situation, function parse_request returns error -1, which leads to termination of request processing.

https://github.com/MarkReedZ/mrhttp/blob/fb0870254016dae2d224915f634448c395914e0d/src/mrhttp/internals/mrhttpparser.c#L100-L155

Function parse_headers_avx2 also hopes that the buffer contains absolutely all headers and their values.

HTTP header parser does not comply with the standard

https://github.com/MarkReedZ/mrhttp/blob/fb0870254016dae2d224915f634448c395914e0d/src/mrhttp/internals/parser.c#L144C16-L144C29

The standard allows more than 1 space between a : and a value.
Therefore, if the request contains a line like this:
Connection: close\r\n
the condition will not work.

The same goes for this place:
https://github.com/MarkReedZ/mrhttp/blob/fb0870254016dae2d224915f634448c395914e0d/src/mrhttp/internals/parser.c#L133-L136
The standard allows you to send the following requests:
Content-Length: 0\r\n

This parser will not pass quality unit tests!

@errantmind
Copy link
Contributor

No comment on standards compliance (haha) but the primary issue I can see is mrhttp is using a cached implementation which not allowed by the tfb rules for plaintext.

@uNetworkingAB
Copy link
Contributor

Forget about mrhttp. Look at gnet. Its entire HTTP parser literally is:

requests[] = split(data, '\r\n\r\n')

and was the official 1st place in Round 22. And it has been tagged as "implementation approach: realistic".

@uNetworkingAB
Copy link
Contributor

I don't think mrhttp does anything spectacularly wrong. It clearly does parse the headers and it does apply some kind of higher level handling above TCP. It uses AVX2 intrinsics to get a bitmap of the ; or \r which is an okay way. It's most likely not standards compliant or secure in any way, but it does look like an HTTP server to me. I'll give it a pass.

@remittor
Copy link
Contributor Author

@uNetworkingAB

It's most likely not standards compliant or secure in any way

So this is not just a matter of compliance with the HTTP standard! I have already described above that the basic principle of working with the TCP data stream has not been observed!
If the HTTP-request is split into 2 parts (which TCP allows), then server mrhttp will not be able to understand that it was sent 1 HTTP-request.

but it does look like an HTTP server to me. I'll give it a pass.

First, mrhttp must become a simple TCP-server, and only then - an HTTP-server.

@TechEmpower TechEmpower deleted a comment from sgammon Jul 11, 2024
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

No branches or pull requests

3 participants