fail if user tries writing bytes after request is sent#270
fail if user tries writing bytes after request is sent#270artemredkin merged 12 commits intoswift-server:masterfrom
Conversation
weissi
left a comment
There was a problem hiding this comment.
This looks good! Left a few comments because I think some details aren't quite right yet.
| let httpClient = HTTPClient(eventLoopGroupProvider: .shared(group)) | ||
| let promise: EventLoopPromise<Channel> = httpClient.eventLoopGroup.next().makePromise() | ||
| let httpBin = HTTPBin(channelPromise: promise) | ||
| let server = NIOHTTP1TestServer(group: MultiThreadedEventLoopGroup(numberOfThreads: 1)) |
There was a problem hiding this comment.
aren't we leaking the group here? It needs to be stopped.
| context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise) | ||
| default: | ||
| let error = HTTPClientError.writeAfterRequestSent | ||
| promise.fail(error) |
There was a problem hiding this comment.
This can't be quite right (and I think we should have a test for this). We're calling out to the user here before we change our state. So we may get user code "sandwiched in" here.
There was a problem hiding this comment.
I've re-ordered things, not sure yet how to test it though, I'll have to get access to a handlers internal state... Could you elaborate on "sandwiched" code a bit?
There was a problem hiding this comment.
@artemredkin sure. If you do promise.fail(...), then the promise's whenFailure and other code will run inline here. So we detected an error (ie. we should be in state .endOrError) but we call out before we entered the .endOrError state. So if the user for example uses another method that does a state check, we potentially have an issue because their code runs before the self.state = .endOrError so they won't see it. That could lead us to make wrong decisions.
There was a problem hiding this comment.
ah, that is so obvious now :) I'll think about how to test this
There was a problem hiding this comment.
@Lukasa could you pick up from Johannes here? I've added a test to verify that if delegate is called here, state is changed. I haven't figured out how to attach anything to that promise though... It seems that I need to have access to channel.write(HTTPClient.request, promise), and I don't think anything in the code can attach to it
| private func writeBodyPart(context: ChannelHandlerContext, part: IOData, promise: EventLoopPromise<Void>) { | ||
| switch self.state { | ||
| case .idle: | ||
| self.actualBodyLength += part.readableBytes |
There was a problem hiding this comment.
I think (would need a test here too) that we're policing the body length too late here, no? We seem to just add the readableBytes but I can't see that we check that it's actually less than or equal the number of body bytes allowed.
What happens if we do say content-length: 1 and then send XABCDEFG, aren't we then still sending ABCDEFG? I think we are and that'd be a security vulnerability because we could smuggle a request (say we did XGET /something HTTP/1.1\r\n\r\n
There was a problem hiding this comment.
thats a great point, yeah, we only check in the end, let me see how I can handle it here
There was a problem hiding this comment.
ok, I added an early fail here
There was a problem hiding this comment.
@artemredkin this looks good but given that it's security relevant, could we add a test specifically for that case (one that'd fail without the new condition)
There was a problem hiding this comment.
we already have it, by accident, we have two tests for content-length/body-length checks:
testContentLengthTooLongFailstestContentLengthTooShortFails
Last one will fail at the end of the request, before we send.end, but the first one will now be failed by this new check
There was a problem hiding this comment.
Potentially we can split the error message, just to be sure
There was a problem hiding this comment.
@artemredkin I think we need a new one that tests that we do not send the bytes.
There was a problem hiding this comment.
Because as soon as the bytes are on the wire, we created a security vulnerability. Even if we detect later that something's wrong, it'll be too late.
There was a problem hiding this comment.
@Lukasa same here, I've added a test to check that we do not send more bytes then body length, although I'm not 100% sure its foolproof
Lukasa
left a comment
There was a problem hiding this comment.
One note in the test.
Regarding the promise that Johannes was talking about, you cannot attach to it directly so it does not immediately matter. In practice it will trigger the callouts to the delegate first: if those callouts are safe, the rest will be too.
| } | ||
|
|
||
| XCTAssertThrowsError(try channel.writeOutbound(request)) | ||
| XCTAssertTrue(delegate.triggered) |
There was a problem hiding this comment.
We can also check on the EmbeddedChannel that only the bytes we expected got written (i.e. the HTTP request head and nothing else, indicating that the check fired before the write did.
There was a problem hiding this comment.
great idea, done!
|
@Lukasa go ahead |
|
cc @ktoso |
Fail if user tries writing body parts after request is sent.
Motivation:
This is could be a security issue.
Modifications:
Result:
Closes #252