fix lack of escaping of filename in Content-Disposition #3556

motoyasu-saburi · 2023-04-01T03:26:29Z

fabricioereche · 2023-04-08T13:08:20Z

context.go

@@ -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)+`"`)


This can fix this specific situation (that enables RFD attack) but what about using filePath#Clean?

I tried using that method,
but the " was not escaped (or URL Encoded), so The vulnerability was not fixed.

But since it's for file reading those quotes will be needed? File params would be readable with or without it, isn't?

@fabricioereche

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?

zpavlinovic · 2023-05-03T21:32:07Z

Is there a plan for this fix to be merged in?

jerbob92 · 2023-05-12T07:48:05Z

Is there a reason we can't use url.QueryEscape here? Just like in the UTF-8 version?

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)))

motoyasu-saburi · 2023-05-12T09:06:06Z

@jerbob92

Is there a reason we can't use url.QueryEscape here? Just like in the UTF-8 version?

Both will solve the security issue.
First, the requirement for encoding by URL Encode is Whtat's WG > multipart/form-data requirements.

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.
https://datatracker.ietf.org/doc/html/rfc6266#section-5

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,
either is honestly fine, since the root cause is that the Content-Disposition specification is not strict.

jerbob92 · 2023-05-13T08:10:53Z

@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 url.QueryEscape. I would go with the escapeQuotes method from http/multipart just so that the logic is matched.

fix backslashes before backquotes were not properly escaped problem.

motoyasu-saburi · 2023-05-13T10:45:44Z

@jerbob92 Thank you,
As you said, there was a problem.
I tried to fix it, Could you review it?

jerbob92 · 2023-05-13T14:06:43Z

@motoyasu-saburi
I tried the escape trick too, but it didn't work, Chrome just transformed it into an underscore, so filename becomes tampering_field.sh_, so that's why I didn't see it as a real problem. But I think the current fix is the best solution.

motoyasu-saburi · 2023-05-15T04:35:23Z

@fabricioereche
I have made some corrections after receiving your approval,
so could you please review it again?

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 <[email protected]>

hmert

This fix is very important to us.

motoyasu-saburi · 2023-05-17T15:04:59Z

Could you review PR ?
@thinkerou @appleboy

adrianosela · 2023-05-19T16:46:54Z

Can we get this in @fabricioereche? Lot's of folks waiting for a patch for GHSA-2c4m-59x9-fr2g

fabricioereche · 2023-05-19T17:40:07Z

Can we get this in @fabricioereche? Lot's of folks waiting for a patch for GHSA-2c4m-59x9-fr2g

all set for me

sm003ash · 2023-05-22T23:05:53Z

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 · 2023-05-24T08:31:08Z

Codecov Report

Merging #3556 (ee5b2c9) into master (a889c58) will increase coverage by 0.38%.
The diff coverage is 100.00%.

@@            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

Flag	Coverage Δ
	`99.01% <100.00%> (+0.38%)`	⬆️
go-1.18	`98.92% <100.00%> (+0.38%)`	⬆️
go-1.19	`99.01% <100.00%> (+0.38%)`	⬆️
go-1.20	`99.01% <100.00%> (+0.38%)`	⬆️
macos-latest	`99.01% <100.00%> (+0.38%)`	⬆️
ubuntu-latest	`99.01% <100.00%> (+0.38%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
context.go	`97.98% <100.00%> (+<0.01%)`	⬆️

... and 1 file with indirect coverage changes

thinkerou · 2023-05-24T08:44:15Z

@appleboy please review, thanks!

amandalal · 2023-05-25T16:50:35Z

@appleboy @thinkerou Any updates on when this will be merged and when the next release will be available? Thank you!

jclebreton · 2023-05-26T09:03:53Z

This PR is referenced in the Govulcheck database (https://pkg.go.dev/vuln/GO-2023-1737) and many people are waiting for the fix.

AndrewSav · 2023-05-28T00:23:38Z

@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!

hmert · 2023-05-28T02:12:25Z

Unfortunately, we decided to move to another framework. For your security, take action.

thinkerou · 2023-05-29T03:15:32Z

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.

motoyasu-saburi added 2 commits April 1, 2023 12:25

fix lack of escaping of filename in Content-Disposition

ded02a8

add test for Content-Disposition filename escaping process

96c3c55

fabricioereche reviewed Apr 8, 2023

View reviewed changes

fabricioereche previously approved these changes Apr 11, 2023

View reviewed changes

motoyasu-saburi mentioned this pull request Apr 25, 2023

x/vulndb: potential Go vuln in github.com/gin-gonic/gin: CVE-2023-29401 golang/vulndb#1737

Closed

fix filename escape bypass problem

ee5b2c9

fix backslashes before backquotes were not properly escaped problem.

motoyasu-saburi dismissed fabricioereche’s stale review via ee5b2c9 May 13, 2023 10:43

jerbob92 approved these changes May 13, 2023

View reviewed changes

6543 mentioned this pull request May 14, 2023

security issue of gin (dependencie) woodpecker-ci/woodpecker#1756

Closed

nabbar mentioned this pull request May 16, 2023

Gin Security Issue nabbar/golib#160

Closed

hmert approved these changes May 17, 2023

View reviewed changes

zadil approved these changes May 17, 2023

View reviewed changes

fabricioereche approved these changes May 19, 2023

View reviewed changes

thinkerou approved these changes May 24, 2023

View reviewed changes

thinkerou requested a review from appleboy May 24, 2023 08:26

thinkerou added the enhancement label May 24, 2023

thinkerou added this to the v1.10 milestone May 24, 2023

This was referenced May 25, 2023

High severity security vulnerability #3608

Closed

Gin Web Framework does not properly sanitize filename parameter #3615

Open

thinkerou added the security label May 29, 2023

thinkerou merged commit 2d4bbec into gin-gonic:master May 29, 2023

This was referenced May 29, 2023

FileAttachment() is vulnerable to Reflect File Download #3555

Closed

ready release gin 1.9.1 #3620

Closed

Ready release gin 1.9.1 (by: thinkerou) #3630

Merged

thinkerou modified the milestones: v1.10, v1.9.1 Jun 1, 2023

motoyasu-saburi deleted the fix-rfd-vuln branch June 1, 2023 03:04

adam-baxter_cbais mentioned this pull request Jun 1, 2023

[GHSA-2c4m-59x9-fr2g] Gin Web Framework does not properly sanitize filename parameter of Context.FileAttachment function github/advisory-database#2348

Merged

abrar71 mentioned this pull request Jun 1, 2023

x/vulndb: suggestion regarding GO-2023-1737 golang/vulndb#1817

Closed

yossich6 mentioned this pull request Jul 26, 2023

gin-keycloakv1.4.0 uses github.com/gin-gonic/[email protected] which has an open cve tbaehler/gin-keycloak#18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix lack of escaping of filename in Content-Disposition #3556

fix lack of escaping of filename in Content-Disposition #3556

motoyasu-saburi commented Apr 1, 2023

fabricioereche Apr 8, 2023

motoyasu-saburi Apr 8, 2023

fabricioereche Apr 8, 2023

motoyasu-saburi Apr 9, 2023

zpavlinovic commented May 3, 2023

jerbob92 commented May 12, 2023

motoyasu-saburi commented May 12, 2023 •

edited

Loading

jerbob92 commented May 13, 2023

motoyasu-saburi commented May 13, 2023

jerbob92 commented May 13, 2023

motoyasu-saburi commented May 15, 2023

hmert left a comment

motoyasu-saburi commented May 17, 2023

adrianosela commented May 19, 2023

fabricioereche commented May 19, 2023

sm003ash commented May 22, 2023

codecov bot commented May 24, 2023 •

edited

Loading

thinkerou commented May 24, 2023

amandalal commented May 25, 2023

jclebreton commented May 26, 2023

AndrewSav commented May 28, 2023

hmert commented May 28, 2023

thinkerou commented May 29, 2023

fix lack of escaping of filename in Content-Disposition #3556

fix lack of escaping of filename in Content-Disposition #3556

Conversation

motoyasu-saburi commented Apr 1, 2023

fabricioereche Apr 8, 2023

Choose a reason for hiding this comment

motoyasu-saburi Apr 8, 2023

Choose a reason for hiding this comment

fabricioereche Apr 8, 2023

Choose a reason for hiding this comment

motoyasu-saburi Apr 9, 2023

Choose a reason for hiding this comment

zpavlinovic commented May 3, 2023

jerbob92 commented May 12, 2023

motoyasu-saburi commented May 12, 2023 • edited Loading

jerbob92 commented May 13, 2023

motoyasu-saburi commented May 13, 2023

jerbob92 commented May 13, 2023

motoyasu-saburi commented May 15, 2023

hmert left a comment

Choose a reason for hiding this comment

motoyasu-saburi commented May 17, 2023

adrianosela commented May 19, 2023

fabricioereche commented May 19, 2023

sm003ash commented May 22, 2023

codecov bot commented May 24, 2023 • edited Loading

Codecov Report

thinkerou commented May 24, 2023

amandalal commented May 25, 2023

jclebreton commented May 26, 2023

AndrewSav commented May 28, 2023

hmert commented May 28, 2023

thinkerou commented May 29, 2023

motoyasu-saburi commented May 12, 2023 •

edited

Loading

codecov bot commented May 24, 2023 •

edited

Loading