feat: Fix usages of possibly concurrent framer read and write calls#3
feat: Fix usages of possibly concurrent framer read and write calls#3ndrscodes wants to merge 2 commits intosecengjeff:mainfrom
Conversation
Signed-off-by: ndrscodes <98schmidtandreas@gmail.com>
|
Thank you for submitting a PR. I really appreciate your contribution and interest in this repo. An earlier version of this tool used a mutex, which made the go routines less useful since a user could only send frames as quickly as the locking calls allowed. I ultimately removed the mutexes, left the routines, and added a flag that allows the user to decide on timing between the HEADERS and RST_STREAM flag. I've only tested this locally on a nginx daemon with a default page (615 bytes) and the routines didn't seem useful in that case. I left them it in place just in case it does become important with real world use cases. The documentation you cited from http2 refers to ReadFrame and WriteContinuation functions. This should not matter in our particular case since it's a security tool that tests server behavior and should never have a need to read a frame or write a continuation frame. The fact that many routines are writing frames should be inconsequential since the framer is first allocated a stream ID before any frames are written from that stream. The stream ID is tracked by a global counter and incremented by two for each new stream (so that the streams stay odd numbered per RFC). |
|
As far as i can tell, the warning is meant for all read and write operations on the framer. I suspect there might be cases in which multiple frames are written to the same output stream on the single underlying TCP connection, which could lead to incorrect or unexpected responses from the server. I'll check the implementation on these read and write operations on the framer and get back to you :) |
|
This is an admittedly weird use case. Frame ordering within a stream matters for the case where we want to implement a proper, end-to-end http2 client. As a security tool, this is just sending many pairs of HEADERS and RST_STREAM frames to allow a researcher to evaluate the server behavior. If we were interested in handling the response I think you're correct that mutexes would be important. |
|
The point i'm making is not that we're not transmitting frames in order, but rather that we might be destroying frames being written by another thread. Looking at the http2 implementation, all frames written to a framer are written to a single buffer. this means that two concurrently running routines might write two frames to a single buffer within one request. This would mean we are sending malformed frames to the remote side, possibly leading to the server sending unexpected results (for example a GOAWAY), but not because of our attempts to exploit the CVE, but rather because of our malformed frames |
The go documentation states the following for the framers write operations:
It will perform exactly one Write to the underlying Writer. It is the caller's responsibility to not call other Write methods concurrently.As well as the following for read operations:
ReadFrame reads a single frame. The returned Frame is only valid until the next call to ReadFrame.Thus, read and write operations on the same framer should not be done by multiple go routines at the same time. I added a read and a write mutex, which ensures only one frame is written or read at the same time. As this might decrease performance, it might some day be necessary to use a connection pool and an individual framer for each routine in order to maximize the amount of frames sent to the server.