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

Deflate decompression doesn't fail for truncated input #175

Closed
vojtarylko opened this issue Sep 14, 2022 · 3 comments · Fixed by #177
Closed

Deflate decompression doesn't fail for truncated input #175

vojtarylko opened this issue Sep 14, 2022 · 3 comments · Fixed by #177

Comments

@vojtarylko
Copy link

I tried to extend HTTPRequestDecompressorTest with this test case:

    func testDecompressionTruncatedInput() throws {
        // Truncated compressed data
        let compressed = ByteBuffer(bytes: [120, 156, 99, 0])

        let channel = EmbeddedChannel()
        try channel.pipeline.addHandler(NIOHTTPRequestDecompressor(limit: .none)).wait()
        let headers = HTTPHeaders([("Content-Encoding", "deflate"), ("Content-Length", "\(compressed.readableBytes)")])
        try channel.writeInbound(HTTPServerRequestPart.head(.init(version: .init(major: 1, minor: 1), method: .POST, uri: "https://nio.swift.org/test", headers: headers)))

        do {
            try channel.writeInbound(HTTPServerRequestPart.body(compressed))
            XCTFail("writeInbound should fail")
        } catch let error as NIOHTTPDecompression.DecompressionError {
            switch error {
            case .inflationError(Int(Z_BUF_ERROR)):
                // ok
                break
            default:
                XCTFail("Unexptected error: \(error)")
            }
        }
    }

As you can see I'm sending truncated invalid compressed data. I'd expect to get DecompressionError.inflationError(-5). Instead decompression succeeds.

(I guess root cause is in z_stream_s.inflatePart(to:minimumCapacity:) which always calls inflate with Z_NO_FLUSH parameter.)

Is the problem in my test case, or in the NIO code?

@Lukasa
Copy link
Contributor

Lukasa commented Sep 14, 2022

NIOHTTPRequestDecompressor does not assume the message is finished until it receives a HTTPServerRequestPart.end. In this instance I suspect we should see an error if we also send .end, but looking at the code I wonder if we're discarding the error result from dropping the decoder. It may be worth adding a send of .end after the send of .body, and then checking to see if there's an error reported here that we're losing:

@vojtarylko
Copy link
Author

The inflateEnd(&self.stream) returns just Z_OK when I add try channel.writeInbound(HTTPServerRequestPart.end(nil)) into test.

Lukasa added a commit to Lukasa/swift-nio-extras that referenced this issue Sep 15, 2022
Motivation

Currently we don't confirm that the decompression has completed
successfully. This means that we can incorrectly spin forever attempting
to decompress past the end of a message, and that we can fail to notice
that a message is truncated. Neither of these is good.

Modifications

Propagate the message zlib gives us as to whether or not decompression
is done, and keep track of it.
Add some tests written by @vojtarylko to validate the behaviour.

Result

Correctly police the bounds of the messages.
Resolves apple#175 and apple#176.
Lukasa added a commit to Lukasa/swift-nio-extras that referenced this issue Sep 15, 2022
Motivation

Currently we don't confirm that the decompression has completed
successfully. This means that we can incorrectly spin forever attempting
to decompress past the end of a message, and that we can fail to notice
that a message is truncated. Neither of these is good.

Modifications

Propagate the message zlib gives us as to whether or not decompression
is done, and keep track of it.
Add some tests written by @vojtarylko to validate the behaviour.

Result

Correctly police the bounds of the messages.
Resolves apple#175 and apple#176.
@Lukasa
Copy link
Contributor

Lukasa commented Sep 15, 2022

Good catch, resolved by #177.

Lukasa added a commit that referenced this issue Sep 16, 2022
Motivation

Currently we don't confirm that the decompression has completed
successfully. This means that we can incorrectly spin forever attempting
to decompress past the end of a message, and that we can fail to notice
that a message is truncated. Neither of these is good.

Modifications

Propagate the message zlib gives us as to whether or not decompression
is done, and keep track of it.
Add some tests written by @vojtarylko to validate the behaviour.

Result

Correctly police the bounds of the messages.
Resolves #175 and #176.
Lukasa added a commit to Lukasa/swift-nio-extras that referenced this issue Sep 16, 2022
Motivation

Currently we don't confirm that the decompression has completed
successfully. This means that we can incorrectly spin forever attempting
to decompress past the end of a message, and that we can fail to notice
that a message is truncated. Neither of these is good.

Modifications

Propagate the message zlib gives us as to whether or not decompression
is done, and keep track of it.
Add some tests written by @vojtarylko to validate the behaviour.

Result

Correctly police the bounds of the messages.
Resolves apple#175 and apple#176.

(cherry picked from commit 6c84d24)
Lukasa added a commit to Lukasa/swift-nio-extras that referenced this issue Sep 16, 2022
Motivation

Currently we don't confirm that the decompression has completed
successfully. This means that we can incorrectly spin forever attempting
to decompress past the end of a message, and that we can fail to notice
that a message is truncated. Neither of these is good.

Modifications

Propagate the message zlib gives us as to whether or not decompression
is done, and keep track of it.
Add some tests written by @vojtarylko to validate the behaviour.

Result

Correctly police the bounds of the messages.
Resolves apple#175 and apple#176.

(cherry picked from commit 6c84d24)
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 a pull request may close this issue.

2 participants