fail if user tries writing bytes after request is sent #270

artemredkin · 2020-06-23T14:48:48Z

Fail if user tries writing body parts after request is sent.

Motivation:
This is could be a security issue.

Modifications:

Check state when writing body parts and fail if its not idle
Added a test

Result:
Closes #252

Lukasa

LGTM, nice patch

weissi

This looks good! Left a few comments because I think some details aren't quite right yet.

weissi · 2020-06-23T15:44:11Z

Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift

@@ -424,11 +424,10 @@ class HTTPClientInternalTests: XCTestCase {
        let randoEL = group.next()

        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))


aren't we leaking the group here? It needs to be stopped.

oops, we do

weissi · 2020-06-23T15:47:15Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+            context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise)
+        default:
+            let error = HTTPClientError.writeAfterRequestSent
+            promise.fail(error)


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.

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?

@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.

ah, that is so obvious now :) I'll think about how to test this

@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

weissi · 2020-06-23T15:49:09Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+    private func writeBodyPart(context: ChannelHandlerContext, part: IOData, promise: EventLoopPromise<Void>) {
+        switch self.state {
+        case .idle:
+            self.actualBodyLength += part.readableBytes


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

thats a great point, yeah, we only check in the end, let me see how I can handle it here

ok, I added an early fail here

@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)

we already have it, by accident, we have two tests for content-length/body-length checks:

testContentLengthTooLongFails

testContentLengthTooShortFails
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

Potentially we can split the error message, just to be sure

@artemredkin I think we need a new one that tests that we do not send the bytes.

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.

@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

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.

Lukasa · 2020-07-17T14:58:50Z

Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift

+        }
+
+        XCTAssertThrowsError(try channel.writeOutbound(request))
+        XCTAssertTrue(delegate.triggered)


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.

great idea, done!

Lukasa · 2020-07-24T13:02:01Z

Sadly I don't have admin on this repo, so we'll need @tomerd to dismiss @weissi's out-of-date review.

@Lukasa

later review by @Lukasa

tomerd · 2020-07-29T19:58:02Z

@Lukasa go ahead

tomerd · 2020-07-29T19:58:27Z

cc @ktoso

fail if user tries writing bytes after request is sent

009bfd7

artemredkin added this to the 1.2.0 milestone Jun 23, 2020

artemredkin requested review from weissi and Lukasa June 23, 2020 14:48

Lukasa approved these changes Jun 23, 2020

View reviewed changes

artemredkin added 2 commits June 23, 2020 16:36

rewrite test

86faf47

swiftformat

b2199f0

weissi previously requested changes Jun 23, 2020

View reviewed changes

artemredkin and others added 6 commits June 23, 2020 16:59

small fix

fa12314

Merge branch 'master' into fail_on_uploading_after_max

9ef725a

Merge branch 'master' into fail_on_uploading_after_max

0a33554

add another test

373d19e

additional test

d112695

Merge branch 'master' into fail_on_uploading_after_max

e947698

artemredkin requested a review from Lukasa July 17, 2020 14:05

Lukasa requested changes Jul 17, 2020

View reviewed changes

artemredkin and others added 3 commits July 17, 2020 16:04

Merge branch 'master' into fail_on_uploading_after_max

8187a97

review fix

5123115

Merge branch 'master' into fail_on_uploading_after_max

414bba6

artemredkin requested a review from Lukasa July 22, 2020 15:30

Lukasa approved these changes Jul 24, 2020

View reviewed changes

Lukasa requested a review from weissi July 24, 2020 13:01

artemredkin merged commit f69b68f into swift-server:master Jul 30, 2020

artemredkin deleted the fail_on_uploading_after_max branch July 30, 2020 10:23

krzyzanowskim mentioned this pull request Apr 7, 2021

Support HTTP/1.1 Upgrade to raw TCP stream #353

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fail if user tries writing bytes after request is sent #270

fail if user tries writing bytes after request is sent #270

artemredkin commented Jun 23, 2020

Lukasa left a comment

weissi left a comment

weissi Jun 23, 2020

artemredkin Jun 23, 2020

artemredkin Jun 23, 2020

weissi Jun 23, 2020

artemredkin Jun 23, 2020

weissi Jun 23, 2020

artemredkin Jun 23, 2020

artemredkin Jul 16, 2020

weissi Jun 23, 2020

artemredkin Jun 23, 2020

artemredkin Jun 23, 2020

weissi Jun 23, 2020

artemredkin Jun 23, 2020 •

edited

Loading

artemredkin Jun 23, 2020

weissi Jun 23, 2020

weissi Jun 23, 2020

artemredkin Jul 16, 2020

Lukasa left a comment

Lukasa Jul 17, 2020

artemredkin Jul 17, 2020

Lukasa commented Jul 24, 2020

tomerd commented Jul 29, 2020

tomerd commented Jul 29, 2020

fail if user tries writing bytes after request is sent #270

fail if user tries writing bytes after request is sent #270

Conversation

artemredkin commented Jun 23, 2020

Lukasa left a comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemredkin Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Jul 24, 2020

tomerd commented Jul 29, 2020

tomerd commented Jul 29, 2020

artemredkin Jun 23, 2020 •

edited

Loading