Skip to content

Commit

Permalink
Fix vulnerability caused by filename escaping (#877)
Browse files Browse the repository at this point in the history
  • Loading branch information
motoyasu-saburi authored Oct 13, 2023
1 parent 105d311 commit c6cf4a6
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 3 deletions.
24 changes: 21 additions & 3 deletions fuel/src/main/kotlin/com/github/kittinunf/fuel/core/DataPart.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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 {
Expand All @@ -282,3 +282,21 @@ data class BlobDataPart(
}
}
}

/**
* 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")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
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")
val shortFile = File(currentDir, "lorem_ipsum_short.tmp")
val file = File(currentDir, "lorem_ipsum_short.tmp")

@Test
fun escapeContentDispositionFileName() {
val filename = "malicious.sh\";\r\ndummy=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)
val specialCharBlobPart = BlobDataPart(file.inputStream(), name = "third", contentLength = file.length(), filename = filename)

assertThat(
"ContentDisposition filename must escape Double-Quote and CRLF",
specialCharInlinePart.contentDisposition == "form-data; name=\"first\"; filename=\"$expectEscapedFilename\""
)
assertThat(
"ContentDisposition filename must escape Double-Quote and CRLF",
specialCharFilePart.contentDisposition == "form-data; name=\"second\"; filename=\"$expectEscapedFilename\""
)
assertThat(
"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)
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 == "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 == "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 == "form-data; name=\"third\"; filename=\"abc.txt\""
)
}
}

0 comments on commit c6cf4a6

Please sign in to comment.