fix lack of escaping of filename in Content-Disposition

[motoyasu-saburi]

https://github.com/gin-gonic/gin/issues/3555

[zpavlinovic]

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

[jerbob92]

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]

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

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

[motoyasu-saburi]

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

[jerbob92]

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

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

[motoyasu-saburi]

Could you review PR ? @thinkerou @appleboy

[adrianosela]

Can we get this in @fabricioereche? Lot's of folks waiting for a patch for https://github.com/advisories/GHSA-2c4m-59x9-fr2g

[fabricioereche]

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

all set for me

[sm003ash]

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[bot]]

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%)	:arrow_up:
go-1.18	98.92% <100.00%> (+0.38%)	:arrow_up:
go-1.19	99.01% <100.00%> (+0.38%)	:arrow_up:
go-1.20	99.01% <100.00%> (+0.38%)	:arrow_up:
macos-latest	99.01% <100.00%> (+0.38%)	:arrow_up:
ubuntu-latest	99.01% <100.00%> (+0.38%)	:arrow_up:

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%)	:arrow_up:

... and 1 file with indirect coverage changes

[thinkerou]

@appleboy please review, thanks!

[amandalal]

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

[jclebreton]

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]

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

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

[thinkerou]

wait #3620 thanks all!