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

reset http_parser if required #192

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

maccoylton
Copy link

If the parser sees a "Connection: Close" message it will not process any subsequent requests. This modification checks for that status and resets the parser. This was first seen when resetting Homebridge on a Raspberry PI, but could equally be exploited by anyone who has access to send http message in the clear to your accessories

@rottenhowler
Copy link

Wait, what? If some client sends "Connection: Close", it should affect it's own HTTP parser. Why would you want to reset it or care if it behaves properly? It's that client's own problem. Are you saying that it affects other clients?

@maccoylton
Copy link
Author

maccoylton commented Mar 30, 2021

An accessory using esp-homekit uses http_parser to process request from the IOS device like the pair request. If for whatever reason that request continues "Connection: Closed" then the parser will not process any subsequent requests, and the accessory will be in a not responding state.

In my case for whatever reason a Raspberry PI running homebridge-zigbee-nt was sending this each time homebridge was restarted, and causing accessories to go into a not responding state. Here's an example of the offending message from the PI, in this case the Pi is .33 and the accessory is .54

`>>> HomeKit: [Client 2] Got new client connection from 192.168.1.33

HomeKit: [Client 1] Have existing connection from 192.168.1.43
on_homekit_event: Client connected, Free Heap=19832
on_homekit_event: End, Freep Heap=19832
homekit_client_process: [Client 2] Got 142 incoming data
Not encrypted data (142 bytes): "GET /accessories HTTP/1.1\x0D\x0AAccept: application/json, text/plain, /\x0D\x0AUser-Agent: axios/0.21.1\x0D\x0AHost: 192.168.1.54:5556\x0D\x0AConnection: close\x0D\x0A\x0D\x0A"
homekit_server_on_message_complete: Unknown endpoint
client_sendv: [Client 2] Sending payload: HTTP/1.1 404 Not Found\x0D\x0A\x0D\x0A
homekit_client_process: [Client 2] Finished processing
`

@maximkulkin
Copy link
Owner

This log does not demonstrate the problem: some client sends request and gets 404. There's nothing wrong with that.

@maccoylton
Copy link
Author

Max, per IM in our slack channel, the issue is NOT the 404 error. The issue is that if ANY message contains "Connection:close" as in the above example then the parser will set an error code and each SUBSEQUENT call to the parser will come straight out ... here is the relevant section in the parser code confirming this behaviour:-

case s_dead: /* this state is used after a 'Connection: close' message * the parser will error out if it reads another message */

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.

4 participants