Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add escape logic for header #3500

Merged
merged 1 commit into from
Feb 17, 2023
Merged

Conversation

t0rchwo0d
Copy link
Contributor

@t0rchwo0d t0rchwo0d commented Feb 14, 2023

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
Copy link

codecov bot commented Feb 16, 2023

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
Copy link
Member

@t0rchwo0d testing fail. Please take a look.

@t0rchwo0d
Copy link
Contributor Author

t0rchwo0d commented Feb 16, 2023

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
Copy link
Contributor Author

Update the same code for build test

@t0rchwo0d
Copy link
Contributor Author

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

@@ -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", "/")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add some unit test cases, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @thinkerou
Thank you for your review!

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

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

update "routes_test.go

@appleboy appleboy added this to the v1.9 milestone Feb 16, 2023
@appleboy appleboy added the bug label Feb 16, 2023
@thinkerou thinkerou merged commit 81ac7d5 into gin-gonic:master Feb 17, 2023
@t0rchwo0d t0rchwo0d deleted the add-escape-logic branch February 17, 2023 02:07
@t0rchwo0d 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
t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023
t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023
@t0rchwo0d t0rchwo0d deleted the add-escape-logic branch February 17, 2023 03:57
@t0rchwo0d
Copy link
Contributor Author

Hi, @appleboy, @thinkerou

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

t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023
t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023
t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023
t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 17, 2023
t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 18, 2023
t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 18, 2023
t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 18, 2023
t0rchwo0d added a commit to t0rchwo0d/gin that referenced this pull request Feb 18, 2023
appleboy pushed a commit that referenced this pull request Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants