Add escape logic for header #3500

t0rchwo0d · 2023-02-14T17:52:37Z

Add escape logic for header

Hi, Team.
I added a escape logic to the header reflecting user input values for the following.
I'd appreciate it if you could review it.

Description

Basically X-Forwarded-Prefix is not required for any purpose other than the / delimiter. However, unintended execution by crafted request.

X-Forwarded-Prefix has a potential problems. Although actively exploiting this flaw is unlikely, Need to prevents abuse in scenarios such as cache poisoning.

How to reproduce

package main

import (
	"net/http"

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

func main() {

	r := gin.Default()

	r.GET("/bug", func(c *gin.Context) {
		c.JSON(http.StatusBadRequest, gin.H{"msg": "bug"})
	})

	r.Run()
}

Case 1. Modulate

Expectations

$ curl -L --path-as-is -i "http://192.168.1.19:8080/bug/" -H "x-forwarded-prefix: ../../bug#?"

HTTP/1.1 301 Moved Permanently
Content-Type: text/html; charset=utf-8
Location: ../../../bug%2523%253F/bug
Date: Tue, 14 Feb 2023 17:17:42 GMT
Content-Length: 61

HTTP/1.1 404 Not Found
Content-Type: text/plain
Date: Tue, 14 Feb 2023 17:17:42 GMT
Content-Length: 18

404 page not found

Actual result

$ curl -L --path-as-is -i "http://192.168.1.19:8080/bug/" -H "x-forwarded-prefix: ../../bug#?"

HTTP/1.1 301 Moved Permanently
Content-Type: text/html; charset=utf-8
Location: ../../bug#?//../../bug%23%3F//bug
Date: Tue, 14 Feb 2023 17:10:43 GMT
Content-Length: 68

HTTP/1.1 400 Bad Request
Content-Type: application/json; charset=utf-8
Date: Tue, 14 Feb 2023 17:10:43 GMT
Content-Length: 13

{"msg":"bug"}

Case 2. Redirect

Expectations

$ curl -L --path-as-is -i "http://192.168.1.19:8080/bug/" -H "x-forwarded-prefix: https://gin-gonic.com/#"

HTTP/1.1 301 Moved Permanently
Content-Type: text/html; charset=utf-8
Location: https%3A/gin-gonic.com/%23/https%253A/gin-gonic.com/%2523/bug
Date: Tue, 14 Feb 2023 17:16:27 GMT
Content-Length: 96

HTTP/1.1 404 Not Found
Content-Type: text/plain
Date: Tue, 14 Feb 2023 17:16:27 GMT
Content-Length: 18

404 page not found

Actual result

$ curl -L --path-as-is -i "http://192.168.1.19:8080/bug/" -H "x-forwarded-prefix: https://gin-gonic.com/#"

HTTP/1.1 301 Moved Permanently
Content-Type: text/html; charset=utf-8
Location: https:/gin-gonic.com/#/https:/gin-gonic.com/%23/bug
Date: Tue, 14 Feb 2023 17:13:50 GMT
Content-Length: 86

HTTP/2 200 
server: GitHub.com
content-type: text/html; charset=utf-8
last-modified: Fri, 29 Apr 2022 07:17:57 GMT
access-control-allow-origin: *
etag: "626b9125-6605"
expires: Tue, 14 Feb 2023 17:23:50 GMT
cache-control: max-age=600
x-proxy-cache: MISS
x-github-request-id: 77D4:63EE:1951BF:1A6FCE:63EBC14E
accept-ranges: bytes
date: Tue, 14 Feb 2023 17:13:50 GMT
via: 1.1 varnish
age: 0
x-served-by: cache-tyo11949-TYO
x-cache: MISS
x-cache-hits: 0
x-timer: S1676394830.054868,VS0,VE181
vary: Accept-Encoding
x-fastly-request-id: a7775619919d2344b41c5b64e35a19b5f1db36c0
content-length: 26117

# ... skip ...

Case 3. Infinite Loop

Expectations

$ curl -L --path-as-is -i "http://192.168.1.19:8080/bug/" -H "x-forwarded-prefix: #bug"

HTTP/1.1 301 Moved Permanently
Content-Type: text/html; charset=utf-8
Location: %23bug/%2523bug/bug
Date: Tue, 14 Feb 2023 17:19:00 GMT
Content-Length: 54

HTTP/1.1 404 Not Found
Content-Type: text/plain
Date: Tue, 14 Feb 2023 17:19:00 GMT
Content-Length: 18

404 page not found

Actual result

$ curl -L --path-as-is -i "http://192.168.1.19:8080/bug/" -H "x-forwarded-prefix: #bug"

HTTP/1.1 301 Moved Permanently
Content-Type: text/html; charset=utf-8
Location: #bug/%23bug/bug
Date: Tue, 14 Feb 2023 17:20:09 GMT
Content-Length: 50

# ... Loop ...

HTTP/1.1 301 Moved Permanently
Content-Type: text/html; charset=utf-8
Location: #bug/%23bug/bug
Date: Tue, 14 Feb 2023 17:20:19 GMT
Content-Length: 50

curl: (47) Maximum (50) redirects followed

Environment

go version: go version go1.19 windows/amd64
gin version (or commit ref): latest
operating system: windows

Reference

codecov · 2023-02-16T08:04:31Z

Codecov Report

Merging #3500 (cb5832c) into master (c1d06e3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3500   +/-   ##
=======================================
  Coverage   98.63%   98.63%           
=======================================
  Files          42       42           
  Lines        3148     3151    +3     
=======================================
+ Hits         3105     3108    +3     
  Misses         29       29           
  Partials       14       14

Flag	Coverage Δ
	`98.63% <100.00%> (+<0.01%)`	⬆️
go-1.16	`∅ <ø> (∅)`
go-1.17	`98.54% <100.00%> (+<0.01%)`	⬆️
go-1.18	`98.54% <100.00%> (+<0.01%)`	⬆️
go-1.19	`98.63% <100.00%> (+<0.01%)`	⬆️
go-1.20	`98.63% <100.00%> (+<0.01%)`	⬆️
macos-latest	`98.54% <100.00%> (-0.10%)`	⬇️
ubuntu-latest	`98.63% <100.00%> (+<0.01%)`	⬆️

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

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

appleboy · 2023-02-16T08:06:59Z

@t0rchwo0d testing fail. Please take a look.

t0rchwo0d · 2023-02-16T09:55:37Z

Hi, @appleboy
Thank you for your review.

Hmm, It's stuck down there.
macos-latest @ Go 1.19 -tags "sonic avx"

=== RUN   TestPathCleanMallocs
    path_test.go:85: 
        	Error Trace:	/Users/runner/work/gin/gin/path_test.go:85
        	Error:      	Not equal: 
        	            	expected: float64(3837)
        	            	actual  : int(0)
        	Test:       	TestPathCleanMallocs

I think it's related to the following.
Looks like probabilistic error occur.
#2596

Can you plz start testing again?

t0rchwo0d · 2023-02-16T10:18:14Z

Update the same code for build test

t0rchwo0d · 2023-02-16T12:05:13Z

@appleboy Oh, testing passed!
I'd appreciate it if you could review it again.

thinkerou · 2023-02-16T12:35:28Z

gin.go

@@ -668,6 +669,9 @@ func redirectTrailingSlash(c *Context) {
 	req := c.Request
 	p := req.URL.Path
 	if prefix := path.Clean(c.Request.Header.Get("X-Forwarded-Prefix")); prefix != "." {
+		prefix = url.QueryEscape(prefix)
+		prefix = strings.ReplaceAll(prefix, "%2F", "/")
+


please add some unit test cases, thanks!

Hi, @thinkerou
Thank you for your review!

I update "routes_test.go", Plz check again : )

t0rchwo0d · 2023-02-16T13:43:48Z

update "routes_test.go

This reverts commit 81ac7d5.

t0rchwo0d · 2023-02-17T03:59:19Z

Hi, @appleboy, @thinkerou

Sorry, There was a lack of additional filter consideration.
So, I have opened the following.
#3503

t0rchwo0d force-pushed the add-escape-logic branch from ec759ae to ff49e55 Compare February 16, 2023 10:17

thinkerou reviewed Feb 16, 2023

View reviewed changes

t0rchwo0d force-pushed the add-escape-logic branch 2 times, most recently from da8520c to d1f3e5f Compare February 16, 2023 13:38

Add escape logic for header

cb5832c

t0rchwo0d force-pushed the add-escape-logic branch from d1f3e5f to cb5832c Compare February 16, 2023 13:41

appleboy added this to the v1.9 milestone Feb 16, 2023

appleboy added the bug label Feb 16, 2023

appleboy requested review from appleboy and thinkerou February 16, 2023 22:17

appleboy approved these changes Feb 16, 2023

View reviewed changes

thinkerou approved these changes Feb 17, 2023

View reviewed changes

thinkerou merged commit 81ac7d5 into gin-gonic:master Feb 17, 2023

t0rchwo0d deleted the add-escape-logic branch February 17, 2023 02:07

t0rchwo0d restored the add-escape-logic branch February 17, 2023 03:02

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023

Revert "Add escape logic for header (gin-gonic#3500)"

c2c6952

This reverts commit 81ac7d5.

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023

Fix gin-gonic#3500 Add escape logic for header

c22614f

t0rchwo0d mentioned this pull request Feb 17, 2023

Fix #3500 Add escape logic for header #3503

Merged

3 tasks

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023

Fix gin-gonic#3500 Add escape logic for header

bd78de0

t0rchwo0d deleted the add-escape-logic branch February 17, 2023 03:57

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023

Fix gin-gonic#3500 Add escape logic for header

fd9f98e

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023

Fix gin-gonic#3500 Add escape logic for header

2deb996

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023

Fix gin-gonic#3500 Add escape logic for header

2c2e15f

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023

Fix gin-gonic#3500 Add escape logic for header

6e8f186

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 18, 2023

Fix gin-gonic#3500 Add escape logic for header

6b8cd84

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 18, 2023

Fix gin-gonic#3500 Add escape logic for header

260f21b

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 18, 2023

Fix gin-gonic#3500 Add escape logic for header

0cc8dca

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 18, 2023

Fix gin-gonic#3500 Add escape logic for header

69e306f

appleboy pushed a commit that referenced this pull request Feb 19, 2023

Fix #3500 Add escape logic for header (#3503)

4cee78f

GoVulnBot mentioned this pull request May 4, 2023

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

Closed

archfish mentioned this pull request May 18, 2023

gin 1.7.4 has redirect issue #3552

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add escape logic for header #3500

Add escape logic for header #3500

t0rchwo0d commented Feb 14, 2023 •

edited

codecov bot commented Feb 16, 2023 •

edited

appleboy commented Feb 16, 2023

t0rchwo0d commented Feb 16, 2023 •

edited

t0rchwo0d commented Feb 16, 2023

t0rchwo0d commented Feb 16, 2023

thinkerou Feb 16, 2023

t0rchwo0d Feb 16, 2023

t0rchwo0d commented Feb 16, 2023

t0rchwo0d commented Feb 17, 2023

Add escape logic for header #3500

Add escape logic for header #3500

Conversation

t0rchwo0d commented Feb 14, 2023 • edited

Add escape logic for header

Description

How to reproduce

Case 1. Modulate

Expectations

Actual result

Case 2. Redirect

Expectations

Actual result

Case 3. Infinite Loop

Expectations

Actual result

Environment

Reference

codecov bot commented Feb 16, 2023 • edited

Codecov Report

appleboy commented Feb 16, 2023

t0rchwo0d commented Feb 16, 2023 • edited

t0rchwo0d commented Feb 16, 2023

t0rchwo0d commented Feb 16, 2023

thinkerou Feb 16, 2023

Choose a reason for hiding this comment

t0rchwo0d Feb 16, 2023

Choose a reason for hiding this comment

t0rchwo0d commented Feb 16, 2023

t0rchwo0d commented Feb 17, 2023

t0rchwo0d commented Feb 14, 2023 •

edited

codecov bot commented Feb 16, 2023 •

edited

t0rchwo0d commented Feb 16, 2023 •

edited