Skip to content

Commit

Permalink
KTOR-8015 Do not accept CR as line delimiter in chunked requests
Browse files Browse the repository at this point in the history
  • Loading branch information
osipxd committed Feb 12, 2025
1 parent bd652e4 commit 2be045a
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 13 deletions.
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
/*
* Copyright 2014-2021 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/
* Copyright 2014-2025 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

package io.ktor.http.cio

import io.ktor.http.cio.internals.*
import io.ktor.utils.io.*
import io.ktor.utils.io.bits.*
import io.ktor.utils.io.core.*
import io.ktor.utils.io.pool.*
import kotlinx.coroutines.*
import kotlinx.io.*
import kotlin.coroutines.*
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.DelicateCoroutinesApi
import kotlinx.coroutines.GlobalScope
import kotlinx.io.EOFException
import kotlin.coroutines.CoroutineContext

private const val MAX_CHUNK_SIZE_LENGTH = 128
private const val CHUNK_BUFFER_POOL_SIZE = 2048
Expand All @@ -34,7 +35,6 @@ public typealias DecoderJob = WriterJob
*
* [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.http.cio.decodeChunked)
*/
@Suppress("TYPEALIAS_EXPANSION_DEPRECATION")
@Deprecated(
"Specify content length if known or pass -1L",
ReplaceWith("decodeChunked(input, -1L)"),
Expand All @@ -43,12 +43,17 @@ public typealias DecoderJob = WriterJob
public fun CoroutineScope.decodeChunked(input: ByteReadChannel): DecoderJob =
decodeChunked(input, -1L)

// Although the line terminator for the start-line and fields is the sequence CRLF,
// a recipient MAY recognize a single LF as a line terminator and ignore any preceding CR.
// https://datatracker.ietf.org/doc/html/rfc9112#section-2.2-3
private val AllowedLineEndings = LineEndingMode.CRLF + LineEndingMode.LF

/**
* Start a chunked stream decoder coroutine
*
* [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.http.cio.decodeChunked)
*/
@Suppress("UNUSED_PARAMETER", "TYPEALIAS_EXPANSION_DEPRECATION")
@Suppress("UNUSED_PARAMETER")
public fun CoroutineScope.decodeChunked(input: ByteReadChannel, contentLength: Long): DecoderJob =
writer(coroutineContext) {
decodeChunked(input, channel)
Expand All @@ -70,7 +75,7 @@ public suspend fun decodeChunked(input: ByteReadChannel, out: ByteWriteChannel)
try {
while (true) {
chunkSizeBuffer.clear()
if (!input.readUTF8LineTo(chunkSizeBuffer, MAX_CHUNK_SIZE_LENGTH)) {
if (!input.readUTF8LineTo(chunkSizeBuffer, MAX_CHUNK_SIZE_LENGTH, AllowedLineEndings)) {
throw EOFException("Chunked stream has ended unexpectedly: no chunk size")
} else if (chunkSizeBuffer.isEmpty()) {
throw EOFException("Invalid chunk size: empty")
Expand All @@ -86,7 +91,7 @@ public suspend fun decodeChunked(input: ByteReadChannel, out: ByteWriteChannel)
}

chunkSizeBuffer.clear()
if (!input.readUTF8LineTo(chunkSizeBuffer, 2)) {
if (!input.readUTF8LineTo(chunkSizeBuffer, 2, AllowedLineEndings)) {
throw EOFException("Invalid chunk: content block of size $chunkSize ended unexpectedly")
}
if (chunkSizeBuffer.isNotEmpty()) {
Expand Down
47 changes: 45 additions & 2 deletions ktor-io/common/src/io/ktor/utils/io/ByteReadChannelOperations.kt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
* Copyright 2014-2025 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

@file:Suppress("DEPRECATION")
Expand All @@ -14,6 +14,7 @@ import kotlinx.io.Buffer
import kotlinx.io.bytestring.*
import kotlinx.io.unsafe.*
import kotlin.coroutines.*
import kotlin.jvm.JvmInline
import kotlin.math.*

@OptIn(InternalAPI::class)
Expand Down Expand Up @@ -395,6 +396,30 @@ public suspend fun ByteReadChannel.discard(max: Long = Long.MAX_VALUE): Long {
private const val CR: Byte = '\r'.code.toByte()
private const val LF: Byte = '\n'.code.toByte()

@JvmInline
public value class LineEndingMode private constructor(private val mode: Int) {

public operator fun contains(other: LineEndingMode): Boolean =
mode or other.mode == mode

public operator fun plus(other: LineEndingMode): LineEndingMode =
LineEndingMode(mode or other.mode)

override fun toString(): String = when (this) {
CR -> "CR"
LF -> "LF"
CRLF -> "CRLF"
else -> listOf(CR, LF, CRLF).filter { it in this }.toString()
}

public companion object {
public val CR: LineEndingMode = LineEndingMode(0b001)
public val LF: LineEndingMode = LineEndingMode(0b010)
public val CRLF: LineEndingMode = LineEndingMode(0b100)
public val Any: LineEndingMode = LineEndingMode(0b111)
}
}

/**
* Reads a line of UTF-8 characters to the specified [out] buffer.
* It recognizes CR, LF and CRLF as a line delimiter.
Expand All @@ -408,8 +433,16 @@ private const val LF: Byte = '\n'.code.toByte()
* @return `true` if a new line separator was found or max bytes appended. `false` if no new line separator and no bytes read.
* @throws TooLongLineException if max is reached before encountering a newline or end of input
*/
@OptIn(InternalAPI::class, InternalIoApi::class)
public suspend fun ByteReadChannel.readUTF8LineTo(out: Appendable, max: Int = Int.MAX_VALUE): Boolean {
return readUTF8LineTo(out, max, lineEnding = LineEndingMode.Any)
}

@OptIn(InternalAPI::class, InternalIoApi::class)
public suspend fun ByteReadChannel.readUTF8LineTo(
out: Appendable,
max: Int = Int.MAX_VALUE,
lineEnding: LineEndingMode = LineEndingMode.Any,
): Boolean {
if (readBuffer.exhausted()) awaitContent()
if (isClosedForRead) return false

Expand All @@ -421,13 +454,17 @@ public suspend fun ByteReadChannel.readUTF8LineTo(out: Appendable, max: Int = In
// Check if LF follows CR after awaiting
if (readBuffer.exhausted()) awaitContent()
if (readBuffer.buffer[0] == LF) {
lineEnding.assertIncludes(LineEndingMode.CRLF)
readBuffer.discard(1)
} else {
lineEnding.assertIncludes(LineEndingMode.CR)
}
out.append(lineBuffer.readString())
return true
}

LF -> {
lineEnding.assertIncludes(LineEndingMode.LF)
out.append(lineBuffer.readString())
return true
}
Expand All @@ -450,6 +487,12 @@ public suspend fun ByteReadChannel.readUTF8LineTo(out: Appendable, max: Int = In
}
}

private fun LineEndingMode.assertIncludes(other: LineEndingMode) {
if (other !in this) {
throw EOFException("Unexpected line ending $other, while expected $this")
}
}

@OptIn(InternalAPI::class, UnsafeIoApi::class, InternalIoApi::class)
public suspend inline fun ByteReadChannel.read(crossinline block: suspend (ByteArray, Int, Int) -> Int): Int {
if (isClosedForRead) return -1
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
* Copyright 2014-2025 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

package io.ktor.server.testing.suites
Expand All @@ -11,6 +11,7 @@ import io.ktor.client.statement.*
import io.ktor.http.*
import io.ktor.http.cio.*
import io.ktor.http.content.*
import io.ktor.network.sockets.*
import io.ktor.server.application.*
import io.ktor.server.engine.*
import io.ktor.server.http.content.*
Expand All @@ -35,6 +36,7 @@ import java.util.*
import java.util.concurrent.*
import java.util.concurrent.atomic.*
import kotlin.test.*
import kotlin.time.Duration.Companion.seconds
import kotlin.use

abstract class SustainabilityTestSuite<TEngine : ApplicationEngine, TConfiguration : ApplicationEngine.Configuration>(
Expand Down Expand Up @@ -605,6 +607,49 @@ abstract class SustainabilityTestSuite<TEngine : ApplicationEngine, TConfigurati
}
}

@Test
@NoHttp2
fun testInvalidLineTerminator() = runTest {
var rootRouteCalled = false
var adminRouteCalled = false

createAndStartServer {
post {
rootRouteCalled = true
call.respondText { "POST /" }
}

get("/admin") {
adminRouteCalled = true
call.respondText { "GET /admin" }
}
}

socket {
getOutputStream().writer().use { writer ->
writer.append(
"POST / HTTP/1.1\r\n",
"Host: localhost:8080\r\n",
"Transfer-Encoding: chunked\r\n",
"\r\n",
"5\r\n",
"AAAAA\r2\r\n",
"47\r\n",
"0\r\n\r\n",
"GET /admin HTTP/1.1\r\n",
"Host: localhost:8080\r\n",
"Transfer-Encoding: chunked\r\n",
"\r\n",
"0\r\n\r\n",
)
}
delay(1.seconds)
}

assertTrue(rootRouteCalled, "Root route should be called")
assertFalse(adminRouteCalled, "Route /admin shouldn't be called")
}

@Ignore("Flaky. To be investigated in KTOR-7811")
@Test
@NoHttp2
Expand Down

0 comments on commit 2be045a

Please sign in to comment.