fix lack of escaping of filename in Content-Disposition #3556
Conversation
context.go
Outdated
| @@ -1056,7 +1056,7 @@ func (c *Context) FileFromFS(filepath string, fs http.FileSystem) { | |||
| // On the client side, the file will typically be downloaded with the given filename | |||
| func (c *Context) FileAttachment(filepath, filename string) { | |||
| if isASCII(filename) { | |||
| c.Writer.Header().Set("Content-Disposition", `attachment; filename="`+filename+`"`) | |||
| c.Writer.Header().Set("Content-Disposition", `attachment; filename="`+strings.Replace(filename, "\"", "\\\"", -1)+`"`) | |||
There was a problem hiding this comment.
This can fix this specific situation (that enables RFD attack) but what about using filePath#Clean?
There was a problem hiding this comment.
I tried using that method,
but the " was not escaped (or URL Encoded), so The vulnerability was not fixed.
There was a problem hiding this comment.
But since it's for file reading those quotes will be needed? File params would be readable with or without it, isn't?
There was a problem hiding this comment.
Perhaps you are confusing the file name with the file path.
There may be some misunderstandings, so I will add to this.
The first thing to escape is the filename. Not filepath.
The second thing, The Filename escape requirements are then as follows
"-->%2F,\"\r-->%0D,\\r\n-->%0A,\\n
The third thing,
This line is the process of "escaping the 'file name at download time' on the HTTP Response Header.
In other words, filepath is irrelevant.
(The part that actually reads the file is the following line (filepath) )
https://github.com/gin-gonic/gin/pull/3556/files#diff-552f47512a00afe5fc6850cc9ddc830a6daeca162750e50aab4ed549685e0253R1063
But since it's for file reading those quotes will be needed? File params would be readable with or without it, isn't?
This is answered by the third matter mentioned above.
This tells the Request side to "download the file with this kind of file name".
It is not the file path to be read by the Server side.
Does this answer your question?
|
Is there a plan for this fix to be merged in? |
|
Is there a reason we can't use For my own implementation I had copied the implementation from Go http/multipart, so: var quoteEscaper = strings.NewReplacer("\\", "\\\\", `"`, "\\\"")
func escapeQuotes(s string) string {
return quoteEscaper.Replace(s)
}
h.Set("Content-Disposition", fmt.Sprintf(`form-data; name="%s"; filename="%s"`, escapeQuotes(fieldname), escapeQuotes(filename))) |
Both will solve the security issue. In this case, the requirement to "perform URL Encoding" is not (strictly) the same as that of What's WG, since this is a response. On the other hand, there is no clear escape requirement specified in the RFC 6266 either. After that, we checked about 80 systems of several services, WebFrameworks, Browsers, etc., and found that most of them use BackSlash encoding for response headers, so we chose BackSlash style escaping and implemented it this time. (For example, Gmail file downloads would have been in backslash format.) In my personal opinion, |
|
@motoyasu-saburi I tested some different things and couldn't get around this fix, so for the least invasive change, I would indeed go for the string replacer version, and not the |
fix backslashes before backquotes were not properly escaped problem.
|
@jerbob92 Thank you, |
|
@motoyasu-saburi |
|
@fabricioereche |
Gin Web Framework does not properly sanitize filename parameter of Context.FileAttachment function References gin-gonic/gin#3555 gin-gonic/gin#3556 https://pkg.go.dev/vuln/GO-2023-1737
Gin Web Framework does not properly sanitize filename parameter of Context.FileAttachment function References gin-gonic/gin#3555 gin-gonic/gin#3556 https://pkg.go.dev/vuln/GO-2023-1737 Co-authored-by: MHSanaei <ho3ein.sanaei@gmail.com>
Gin Web Framework does not properly sanitize filename parameter of Context.FileAttachment function References gin-gonic/gin#3555 gin-gonic/gin#3556 https://pkg.go.dev/vuln/GO-2023-1737 Co-authored-by: MHSanaei <ho3ein.sanaei@gmail.com>
|
Could you review PR ? |
|
Can we get this in @fabricioereche? Lot's of folks waiting for a patch for GHSA-2c4m-59x9-fr2g |
all set for me |
|
Hi, when will this be merged and available in the next release? We use the gin framework at my workplace, and this CVE issue is urgent for us to fix, else we will have to migrate to another framework, which we don't want to do. |
Codecov Report
@@ Coverage Diff @@
## master #3556 +/- ##
==========================================
+ Coverage 98.63% 99.01% +0.38%
==========================================
Files 42 42
Lines 3151 3153 +2
==========================================
+ Hits 3108 3122 +14
+ Misses 29 21 -8
+ Partials 14 10 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@appleboy please review, thanks! |
|
@appleboy @thinkerou Any updates on when this will be merged and when the next release will be available? Thank you! |
|
This PR is referenced in the Govulcheck database (https://pkg.go.dev/vuln/GO-2023-1737) and many people are waiting for the fix. |
|
@appleboy is there anything we can do to help move this forward? It has been almost 2 months with this vulnerability and no fix, people get anxious, feeling unprotected. A suggestion from me would be that if you cannot do the review and merge, could you possibly delegate it to @thinkerou? Many thanks! |
|
Unfortunately, we decided to move to another framework. For your security, take action. |
|
wait #3620 thanks all! |
* fix lack of escaping of filename in Content-Disposition * add test for Content-Disposition filename escaping process * fix filename escape bypass problem fix backslashes before backquotes were not properly escaped problem.
#3555