FileAttachment() is vulnerable to Reflect File Download

[motoyasu-saburi]

I have contacted the maintainer 4-5 times via email, Snyk, etc.,
but have not received a reply, so I am raising an Issue.

Description

gin > context.go > func FileAttachment
contains a vulnerability that allows the extension/file name to be tampered with when downloading files.

This is known as a Reflect File Download attack.
https://www.blackhat.com/docs/eu-14/materials/eu-14-Hafif-Reflected-File-Download-A-New-Web-Attack-Vector.pdf

This vulnerability allows a Threat Actor to replace what was .txt when uploading with .sh, etc. when (victim) downloading.

This is due to improper handling of the filename in the Content-Disposition response header.
https://github.com/gin-gonic/gin/blob/a889c58de78711cb9b53de6cfcc9272c8518c729/context.go#LL1059-L1059C86

My calculations CVSS (v3):

• CVSS:3.0/AV:N/AC:H/PR:N/UI:R/S:U/C:H/I:H/A:H
• Score: 7.5

However, I think the impact of the actual situation is much lower than that of CVSS.

How to reproduce

Env

OS: macOSVentura(13.1)
Go ver: 1.18.2 darwin/arm64
gin ver: v1.9.0 (The latest ver has not been corrected either, so the problem probably remains.)
Browser: Chrome 111.0.5563.110 (arm64)

(Linux or MacOS is required.
This is because Windows does not allow file names containing " (double-quote) .)

---

1. Create File

$ touch 'malicious.sh";dummy=.txt'

2. Setup gin project

$ mkdir myproject
$ go mod init tidy
$ go get -u github.com/gin-gonic/gin

3. Generate Vuln code

$ vi main.go

Edit Directory

package main

import (
  "github.com/gin-gonic/gin"
)

func main() {
  r := gin.Default()
  r.GET("/download", func(c *gin.Context) {
    dir := "/Users/{CHANGE_PROJECT_DIRECTRY}/"

    // Although the file name is hard-coded, we assume that the file name is actually determined by the DB or user input.
    filename := "malicious.sh\";dummy=.txt"
    c.FileAttachment(dir + filename, filename)
  })
  r.Run()
}

4. Build & Run & Access

$ go build ./main.go
$ ./main

Access: http://localhost:8080/download

---

Return Response Header:

HTTP/1.1 304 Not Modified
Content-Disposition: attachment; filename="malicious.sh";dummy=.txt"
Last-Modified: Tue, 19 Jul 2022 08:53:26 GMT
Date: Wed, 20 Jul 2022 11:17:43 GMT

Content-Disposition:

Content-Disposition: attachment; filename="malicious.sh";dummy=.txt"

Download File:
malicious.sh

Cause & Fix

The reason is that the filename in Content-Disposition is not escape / encoded.

If not escape / encoded ( " ), the filename field can be terminated arbitrarily, as shown in the PoC.

Content-Disposition: attachment; filename="malicious.sh";dummy=.txt"

Therefore, the following escapes are required.

• " --> \" or %22
• \r --> \\r or %0D
• \n --> \\n or %0A

( \r, \n was converted to space, so no problem.)

The requirements for encoding filename in Content-Disposition are described in RFC6266 and WhatWG html spec(multipart/form-data).

RFC 6266 (section 5)
https://tools.ietf.org/html/rfc6266#section-5

Appendix D. Advice on Generating Content-Disposition Header Fields
...
o Avoid including the percent character followed by two hexadecimal characters (e.g., %A9) in the filename parameter, since some existing implementations consider it to be an escape character, while others will pass it through unchanged.
...
Avoid including the "" character in the quoted-string form of the filename parameter, as escaping is not implemented by some user agents, and "" can be considered an illegal path character.

What WG - html spec - multipart/form-data
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.

Golang also implements Content-Disposition escaping based on these requirements.
(This implementation is a Content-Disposition implementation for multipart/form-data,
so the situation is slightly different from the HTTP Response > Content-Disposition in this issue.)
https://github.com/golang/go/blob/e0e0c8fe9881bbbfe689ad94ca5dddbb252e4233/src/mime/multipart/writer.go#L144

---

Reference

RFC 6266 (section 5)
https://tools.ietf.org/html/rfc6266#section-5

What WG - html spec > multipart/form-data :
https://html.spec.whatwg.org/#multipart-form-data

OWASP ASVS (Related issue):
OWASP/ASVS#1390

Golang impliments:
https://github.com/golang/go/blob/e0e0c8fe9881bbbfe689ad94ca5dddbb252e4233/src/mime/multipart/writer.go#L144

Spring (Java) Impliments:
https://github.com/spring-projects/spring-framework/blob/4f8516e2c3ca420b1608840ab901bf9df7e4d5f1/spring-web/src/main/java/org/springframework/http/ContentDisposition.java#L594-L617

Symfony(PHP) Impliments:
https://github.com/symfony/symfony/blob/123b1651c4a7e219ba59074441badfac65525efe/src/Symfony/Component/HttpFoundation/HeaderUtils.php#L187-L189

This is my own article, but it summarizes the impact, etc. on this issue.
https://gist.github.com/motoyasu-saburi/1b19ef18e96776fe90ba1b9f910fa714

[zpavlinovic]

The Go Vulnerability Database has designated this GO-2023-1737 (https://pkg.go.dev/vuln/GO-2023-1737) and CVE-2023-29401. To add a fixed version or otherwise update this report, you can reopen and comment on golang/vulndb#1737.

[Clivern]

GHSA-2c4m-59x9-fr2g

[jnelle]

Silence is the new answer, thx @appleboy and all the other maintainers

[vitordm]

any news about that?

[adrianosela]

@vitordm the fix has been merged via #3556 (btw: written by the author of this issue) and we are waiting for a new release, which is ready to go out (see #3620) but one of the maintainers seems to be taking their sweet time...

[motoyasu-saburi]

Thank you so much @thinkerou .

If possible, I would appreciate it if you could enable the GitHub feature
"Private vulnerability reporting,".

There are people (@igibek) in this Issue who,
like me, are looking for a place to report vulnerabilities.
#3563

[thinkerou]

@motoyasu-saburi sorry, I have not the permission.

[thinkerou]

v1.9.1 have released, please see https://github.com/gin-gonic/gin/releases/tag/v1.9.1, thanks!

[soumiksamanta]

nancy is still complaining about the CVE for v1.9.1

pkg:golang/github.com/gin-gonic/gin@v1.9.1
1 known vulnerabilities affecting installed version
┃ [CVE-2023-29401] CWE-494: Download of Code Without Integrity Check
┃ Description ┃ github.com/gin-gonic/gin - Reflected File Download ┃
┃ ┃ ┃
┃ ┃ Sonatype's research suggests that this CVE's details differ from those ┃
┃ ┃ defined at NVD. See ┃
┃ ┃ https://ossindex.sonatype.org/vulnerability/CVE-2023-29401 for details

[dhawton]

nancy is still complaining about the CVE for v1.9.1

pkg:golang/github.com/gin-gonic/gin@v1.9.1 1 known vulnerabilities affecting installed version ┃ [CVE-2023-29401] CWE-494: Download of Code Without Integrity Check ┃ Description ┃ github.com/gin-gonic/gin - Reflected File Download ┃ ┃ ┃ ┃ ┃ ┃ Sonatype's research suggests that this CVE's details differ from those ┃ ┃ ┃ defined at NVD. See ┃ ┃ ┃ https://ossindex.sonatype.org/vulnerability/CVE-2023-29401 for details

It might not know that 1.9.1 has the patch. Scanners don't always look at the code and wait for someone to update what the fix version is.

[cschneider4711]

Current scan (10 mins ago) as part of a GitHub workflow with Nancy no longer complains, so it appears that the update is now also known to them...