From 87e8b783adeda3aaf4b71f579a8f202750a9e10a Mon Sep 17 00:00:00 2001 From: motoyasu-saburi Date: Mon, 2 Oct 2023 21:42:15 +0900 Subject: [PATCH 1/6] Fix vulnerability caused by filename escaping --- .../github/kittinunf/fuel/core/DataPart.kt | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/fuel/src/main/kotlin/com/github/kittinunf/fuel/core/DataPart.kt b/fuel/src/main/kotlin/com/github/kittinunf/fuel/core/DataPart.kt index d91f91e09..5b742f4d0 100644 --- a/fuel/src/main/kotlin/com/github/kittinunf/fuel/core/DataPart.kt +++ b/fuel/src/main/kotlin/com/github/kittinunf/fuel/core/DataPart.kt @@ -97,7 +97,7 @@ data class InlineDataPart( val name: String, val filename: String? = null, override val contentType: String = "$GENERIC_CONTENT; charset=utf-8", - override val contentDisposition: String = "form-data; name=\"$name\"${if (filename != null) "; filename=\"$filename\"" else "" }" + override val contentDisposition: String = "form-data; name=\"$name\"${if (filename != null) "; filename=\"${escapeFilename(filename)}\"" else "" }" ) : DataPart() { override fun inputStream() = content.byteInputStream() override val contentLength get() = content.toByteArray(Charsets.UTF_8).size.toLong() @@ -173,7 +173,7 @@ data class FileDataPart( // private; this might result, for example, when selection or drag-and- // drop is used or when the form data content is streamed directly from // a device. - override val contentDisposition: String = "form-data; name=\"$name\"${if (filename != null) "; filename=\"$filename\"" else "" }" + override val contentDisposition: String = "form-data; name=\"$name\"${if (filename != null) "; filename=\"${escapeFilename(filename)}\"" else "" }" ) : DataPart() { override fun inputStream() = file.inputStream() override val contentLength get() = file.length() @@ -269,7 +269,7 @@ data class BlobDataPart( val filename: String? = null, override val contentLength: Long? = null, override val contentType: String = BlobDataPart.guessContentType(inputStream), - override val contentDisposition: String = "form-data; name=\"$name\"${if (filename != null) "; filename=\"$filename\"" else "" }" + override val contentDisposition: String = "form-data; name=\"$name\"${if (filename != null) "; filename=\"${escapeFilename(filename)}\"" else "" }" ) : DataPart() { override fun inputStream() = inputStream companion object { @@ -282,3 +282,33 @@ data class BlobDataPart( } } } + +/** + * Create a FileDataPart from a [directory] and [filename] + * + * @param directory [File] the directory + * @param filename [String] the filename relative to the [directory] + * @param name [String] the name for the field, uses [filename] without the extension by default + * @param remoteFileName [String] the filename parameter for the DataPart, set to null to exclude, empty to use [filename] + * @param contentType [String] the Content-Type for the DataPart, set to null to [guessContentType] + * + * @return [FileDataPart] the DataPart + */ + +/** + * Escape "filename" in Content-Disposition + * + * https://html.spec.whatwg.org/#multipart-form-data + * > For field names and filenames for file fields, the result of the encoding in the previous bullet point must be + * > escaped by replacing any 0x0A (LF) bytes with the byte sequence `%0A`, 0x0D (CR) with `%0D` and 0x22 (") with `%22`. + * > The user agent must not perform any other escapes. + * + * @param filename [String] the filename on Content-Disposition header + * + * @return [String] the escaped filename + */ +private fun escapeFilename(filename: String): String { + return filename.replace("\"", "%22") + .replace("\r", "%0D") + .replace("\n", "%0A") +} \ No newline at end of file From 849e467484bc2148b4d281d476d40e25b577b958 Mon Sep 17 00:00:00 2001 From: motoyasu-saburi Date: Mon, 2 Oct 2023 21:53:58 +0900 Subject: [PATCH 2/6] remove unwanted comments --- .../com/github/kittinunf/fuel/core/DataPart.kt | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/fuel/src/main/kotlin/com/github/kittinunf/fuel/core/DataPart.kt b/fuel/src/main/kotlin/com/github/kittinunf/fuel/core/DataPart.kt index 5b742f4d0..367cab858 100644 --- a/fuel/src/main/kotlin/com/github/kittinunf/fuel/core/DataPart.kt +++ b/fuel/src/main/kotlin/com/github/kittinunf/fuel/core/DataPart.kt @@ -283,18 +283,6 @@ data class BlobDataPart( } } -/** - * Create a FileDataPart from a [directory] and [filename] - * - * @param directory [File] the directory - * @param filename [String] the filename relative to the [directory] - * @param name [String] the name for the field, uses [filename] without the extension by default - * @param remoteFileName [String] the filename parameter for the DataPart, set to null to exclude, empty to use [filename] - * @param contentType [String] the Content-Type for the DataPart, set to null to [guessContentType] - * - * @return [FileDataPart] the DataPart - */ - /** * Escape "filename" in Content-Disposition * From 69d4f060ce5d0c145b0a59dac58e04303b7de920 Mon Sep 17 00:00:00 2001 From: motoyasu-saburi Date: Tue, 3 Oct 2023 00:54:43 +0900 Subject: [PATCH 3/6] add escape test --- .../kittinunf/fuel/core/DataPartTest.kt | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt diff --git a/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt b/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt new file mode 100644 index 000000000..b9ccb43cd --- /dev/null +++ b/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt @@ -0,0 +1,52 @@ +package com.github.kittinunf.fuel.core + +import org.hamcrest.MatcherAssert.assertThat +import org.junit.Test +import java.io.File + +class DataPartTest { + private val currentDir = File(System.getProperty("user.dir"), "src/test/assets") + + @Test + fun escapeContentDispositionFileName() { + val shortFile = File(currentDir, "lorem_ipsum_short.tmp") + val file = File(currentDir, "lorem_ipsum_short.tmp") + val filename = "malicious.sh\";\r\ndummy=a.txt" + val exceptEscapedFilename = "malicious.sh%22;%0D%0Adummy=a.txt" + + val specialCharInlinePart = InlineDataPart("first", name = "first", contentType = "application/json", filename = filename) + val specialCharFilePart = FileDataPart(shortFile, name = "second", filename = filename) + val specialCharBlobPart = BlobDataPart(file.inputStream(), name = "third", contentLength = file.length(), filename = filename) + + assertThat( + "ContentDisposition filename must escape Double-Quote and CRLF", + specialCharInlinePart.contentDisposition.contains(exceptEscapedFilename) + ) + assertThat( + "ContentDisposition filename must escape Double-Quote and CRLF", + specialCharFilePart.contentDisposition.contains(exceptEscapedFilename) + ) + assertThat( + "ContentDisposition filename must escape Double-Quote and CRLF", + specialCharBlobPart.contentDisposition.contains(exceptEscapedFilename) + ) + + val normalFileName = "abc.txt" + val normalCharInlinePart = InlineDataPart("first", name = "first", contentType = "application/json", filename = normalFileName) + val normalFilePart = FileDataPart(shortFile, name = "second", filename = normalFileName) + val normalBlobPart = BlobDataPart(file.inputStream(), name = "third", contentLength = file.length(), filename = normalFileName) + + assertThat( + "filename should be output as is if it does not contain special characters(Double-Quote, CRLF)", + normalCharInlinePart.contentDisposition.contains("abc.txt") + ) + assertThat( + "filename should be output as is if it does not contain special characters(Double-Quote, CRLF)", + normalFilePart.contentDisposition.contains("abc.txt") + ) + assertThat( + "filename should be output as is if it does not contain special characters(Double-Quote, CRLF)", + normalBlobPart.contentDisposition.contains("abc.txt") + ) + } +} \ No newline at end of file From 8798c67b5e5704d2281b42b163da7b4dba0cc7d4 Mon Sep 17 00:00:00 2001 From: motoyasu-saburi Date: Tue, 3 Oct 2023 20:04:30 +0900 Subject: [PATCH 4/6] fix test case --- .../com/github/kittinunf/fuel/core/DataPartTest.kt | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt b/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt index b9ccb43cd..9f8220caf 100644 --- a/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt +++ b/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt @@ -20,15 +20,15 @@ class DataPartTest { assertThat( "ContentDisposition filename must escape Double-Quote and CRLF", - specialCharInlinePart.contentDisposition.contains(exceptEscapedFilename) + specialCharInlinePart.contentDisposition == "form-data; name=\"first\"; filename=\"$exceptEscapedFilename\"" ) assertThat( "ContentDisposition filename must escape Double-Quote and CRLF", - specialCharFilePart.contentDisposition.contains(exceptEscapedFilename) + specialCharFilePart.contentDisposition == "form-data; name=\"second\"; filename=\"$exceptEscapedFilename\"" ) assertThat( "ContentDisposition filename must escape Double-Quote and CRLF", - specialCharBlobPart.contentDisposition.contains(exceptEscapedFilename) + specialCharBlobPart.contentDisposition == "form-data; name=\"third\"; filename=\"$exceptEscapedFilename\"" ) val normalFileName = "abc.txt" @@ -38,15 +38,16 @@ class DataPartTest { assertThat( "filename should be output as is if it does not contain special characters(Double-Quote, CRLF)", - normalCharInlinePart.contentDisposition.contains("abc.txt") + normalCharInlinePart.contentDisposition == "form-data; name=\"first\"; filename=\"abc.txt\"" + ) assertThat( "filename should be output as is if it does not contain special characters(Double-Quote, CRLF)", - normalFilePart.contentDisposition.contains("abc.txt") + normalFilePart.contentDisposition == "form-data; name=\"second\"; filename=\"abc.txt\"" ) assertThat( "filename should be output as is if it does not contain special characters(Double-Quote, CRLF)", - normalBlobPart.contentDisposition.contains("abc.txt") + normalBlobPart.contentDisposition == "form-data; name=\"third\"; filename=\"abc.txt\"" ) } } \ No newline at end of file From ebf6ed3e2b1d1430c0242543aad665248a3bbbc3 Mon Sep 17 00:00:00 2001 From: motoyasu-saburi Date: Tue, 3 Oct 2023 20:06:09 +0900 Subject: [PATCH 5/6] fix typo --- .../kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt b/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt index 9f8220caf..db1cfd2e7 100644 --- a/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt +++ b/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt @@ -12,7 +12,7 @@ class DataPartTest { val shortFile = File(currentDir, "lorem_ipsum_short.tmp") val file = File(currentDir, "lorem_ipsum_short.tmp") val filename = "malicious.sh\";\r\ndummy=a.txt" - val exceptEscapedFilename = "malicious.sh%22;%0D%0Adummy=a.txt" + val expectEscapedFilename = "malicious.sh%22;%0D%0Adummy=a.txt" val specialCharInlinePart = InlineDataPart("first", name = "first", contentType = "application/json", filename = filename) val specialCharFilePart = FileDataPart(shortFile, name = "second", filename = filename) @@ -20,15 +20,15 @@ class DataPartTest { assertThat( "ContentDisposition filename must escape Double-Quote and CRLF", - specialCharInlinePart.contentDisposition == "form-data; name=\"first\"; filename=\"$exceptEscapedFilename\"" + specialCharInlinePart.contentDisposition == "form-data; name=\"first\"; filename=\"$expectEscapedFilename\"" ) assertThat( "ContentDisposition filename must escape Double-Quote and CRLF", - specialCharFilePart.contentDisposition == "form-data; name=\"second\"; filename=\"$exceptEscapedFilename\"" + specialCharFilePart.contentDisposition == "form-data; name=\"second\"; filename=\"$expectEscapedFilename\"" ) assertThat( "ContentDisposition filename must escape Double-Quote and CRLF", - specialCharBlobPart.contentDisposition == "form-data; name=\"third\"; filename=\"$exceptEscapedFilename\"" + specialCharBlobPart.contentDisposition == "form-data; name=\"third\"; filename=\"$expectEscapedFilename\"" ) val normalFileName = "abc.txt" From b0ea243d65bc617adb3b48f05483d70298044721 Mon Sep 17 00:00:00 2001 From: motoyasu-saburi Date: Thu, 12 Oct 2023 00:54:02 +0900 Subject: [PATCH 6/6] fix split test cases --- .../kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt b/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt index db1cfd2e7..6bba6ec10 100644 --- a/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt +++ b/fuel/src/test/kotlin/com/github/kittinunf/fuel/core/DataPartTest.kt @@ -6,11 +6,11 @@ import java.io.File class DataPartTest { private val currentDir = File(System.getProperty("user.dir"), "src/test/assets") + val shortFile = File(currentDir, "lorem_ipsum_short.tmp") + val file = File(currentDir, "lorem_ipsum_short.tmp") @Test fun escapeContentDispositionFileName() { - val shortFile = File(currentDir, "lorem_ipsum_short.tmp") - val file = File(currentDir, "lorem_ipsum_short.tmp") val filename = "malicious.sh\";\r\ndummy=a.txt" val expectEscapedFilename = "malicious.sh%22;%0D%0Adummy=a.txt" @@ -30,7 +30,10 @@ class DataPartTest { "ContentDisposition filename must escape Double-Quote and CRLF", specialCharBlobPart.contentDisposition == "form-data; name=\"third\"; filename=\"$expectEscapedFilename\"" ) + } + @Test + fun escapeNotContainSpecialChar() { val normalFileName = "abc.txt" val normalCharInlinePart = InlineDataPart("first", name = "first", contentType = "application/json", filename = normalFileName) val normalFilePart = FileDataPart(shortFile, name = "second", filename = normalFileName) @@ -39,7 +42,6 @@ class DataPartTest { assertThat( "filename should be output as is if it does not contain special characters(Double-Quote, CRLF)", normalCharInlinePart.contentDisposition == "form-data; name=\"first\"; filename=\"abc.txt\"" - ) assertThat( "filename should be output as is if it does not contain special characters(Double-Quote, CRLF)",