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
fix: fix path-traversal bug #229
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #229 +/- ##
===========================================
- Coverage 59.19% 59.17% -0.03%
===========================================
Files 79 79
Lines 8105 8110 +5
===========================================
+ Hits 4798 4799 +1
- Misses 2953 2957 +4
Partials 354 354
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
When checking other http framework source code, I find that fasthttp community fix the same bug six months ago. |
They just repeat the logic, there is no need to write duplicate code. |
pkg/protocol/uri.go
Outdated
| @@ -480,6 +480,15 @@ func splitHostURI(host, uri []byte) ([]byte, []byte, []byte) { | |||
| } | |||
|
|
|||
| func normalizePath(dst, src []byte) []byte { | |||
| // replace all backslashes with forward slashes | |||
| for { | |||
| n := bytes.Index(src, bytestr.StrBackSlash) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Seems it will double check for bytes that have been replaced, is this an performance issue on long paths?
- I'm not sure it's a good idea to replace
srcin place, maybe it's hard to understand that ifnormalizePathhas a return value and also modifies the input parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Seems it will double check for bytes that have been replaced, is this an performance issue on long paths?
- I'm not sure it's a good idea to replace
srcin place, maybe it's hard to understand that ifnormalizePathhas a return value and also modifies the input parameters.
maybe this one would be better
func normalizePath(dst, src []byte) []byte {
dst = dst[:0]
dst = addLeadingSlash(dst, src)
dst = decodeArgAppendNoPlus(dst, src)
// replace all backslashes with forward slashes
if filepath.Separator == '\\' {
for i := range dst {
if dst[i] == '\\' {
dst[i] = '/'
}
}
}
...
}BTW: echo use function filepath.ToSlash
https://github.com/labstack/echo/blob/master/context_fs.go#L33
func ToSlash(path string) string {
if Separator == '/' {
return path
}
return strings.ReplaceAll(path, string(Separator), "/")
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bytes.IndexByte will do a SIMD accelerate in some platforms. Not sure which one is better.
And since the bug only happens in Windows? Maybe add a if filepath.Separator == '\\' { to filter the platform?
|
The |
this version solve the problem func normalizePath(dst, src []byte) []byte {
dst = dst[:0]
dst = addLeadingSlash(dst, src)
dst = decodeArgAppendNoPlus(dst, src)
// replace all backslashes with forward slashes
if filepath.Separator == '\\' {
for i := range dst {
if dst[i] == '\\' {
dst[i] = '/'
}
}
}
...
}before the if judgment curl -v 127.0.0.1:8888/..%5c..%5cgo.sum
* Trying 127.0.0.1:8888...
* Connected to 127.0.0.1 (127.0.0.1) port 8888 (#0)
> GET /..%5c..%5cgo.sum HTTP/1.1
> Host: 127.0.0.1:8888
> User-Agent: curl/7.83.1
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 404 Page not found
< Date: Mon, 05 Sep 2022 09:10:07 GMT
< Content-Type: text/plain; charset=utf-8
< Content-Length: 26
<
Cannot open requested path* Connection #0 to host 127.0.0.1 left intact |
Good job! As for the performance part, maybe do a bench test to see the answer. |
I did some simple benchmarks, when there are backslashes in path, for-range option is faster, when there is no backslash in path, IndexByte option is faster. Choose IndexByte maybe more reasonable since most normal URLs don't have backslashes in them. var benchDstBackslashShort = []byte("/..\\foo")
var benchDstBackslashLong = []byte("/..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\foo")
var benchDstNoBackslashShort = []byte("/../foo")
var benchDstNoBackslashLong = []byte("/../../../../../../../../../../foo")
func BenchmarkForrange(b *testing.B) {
for i := 0; i < b.N; i++ {
for i := range benchDstNoBackslashShort {
if benchDstNoBackslashShort[i] == '\\' {
benchDstNoBackslashShort[i] = '/'
}
}
benchDstNoBackslashShort = []byte("/../foo")
}
}
func BenchmarkIndexByte(b *testing.B) {
for i := 0; i < b.N; i++ {
for {
n := bytes.IndexByte(benchDstNoBackslashShort, '\\')
if n < 0 {
break
}
benchDstNoBackslashShort[n] = '/'
}
benchDstNoBackslashShort = []byte("/../foo")
}
}environment goos: windows
goarch: amd64
pkg: github.com/cloudwego/hertz/pkg/protocol
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHzbenchDstBackslashShort BenchmarkForrange-12 57650732 19.92 ns/op 8 B/op 1 allocs/op
BenchmarkIndexByte-12 43823608 27.97 ns/op 8 B/op 1 allocs/op
PASS
ok github.com/cloudwego/hertz/pkg/protocol 2.521sbenchDstBackslashLong BenchmarkForrange-12 21818498 52.70 ns/op 48 B/op 1 allocs/op
BenchmarkIndexByte-12 9185786 130.1 ns/op 48 B/op 1 allocs/op
PASS
ok github.com/cloudwego/hertz/pkg/protocol 2.941sbenchDstNoBackslashShort BenchmarkForrange-12 53754288 19.66 ns/op 8 B/op 1 allocs/op
BenchmarkIndexByte-12 61503766 19.34 ns/op 8 B/op 1 allocs/op
PASS
ok github.com/cloudwego/hertz/pkg/protocol 2.379sbenchDstNoBackslashLong BenchmarkForrange-12 21096711 49.30 ns/op 48 B/op 1 allocs/op
BenchmarkIndexByte-12 32403181 37.07 ns/op 48 B/op 1 allocs/op
PASS
ok github.com/cloudwego/hertz/pkg/protocol 2.724s |
|
PTAL. There may have a side effect which lead to a failure of existing UT |
|
My fault. Since we add if filepath.Separator == '\\' && string(parsedPath) != expectedPath {
t.Fatalf("Unexpected Path: %q. Expected %q", parsedPath, expectedPath)
}Also, github action can not check windows path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
What type of PR is this?
fix: A bug fix
Check the PR title.
This PR title match the format: (optional scope):
The description of this PR title is user-oriented and clear enough for others to understand.
(Optional) Translate the PR title into Chinese.
修复目录穿越漏洞
(Optional) More detail description for this PR(en: English/zh: Chinese).
en: fix by simply replace all backslashes with forward slashes
zh(optional): 将反斜杠全部改成正斜杠
Which issue(s) this PR fixes:
Fixes #228
Currently hertz has no restriction on backslash in
normalizePathfunction.After: