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

net/url: JoinPath doesn't strip relative path components in all circumstances #54385

Closed
neild opened this issue Aug 11, 2022 · 10 comments · Fixed by #54390
Closed

net/url: JoinPath doesn't strip relative path components in all circumstances #54385

neild opened this issue Aug 11, 2022 · 10 comments · Fixed by #54390
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone

Comments

@neild
Copy link
Contributor

neild commented Aug 11, 2022

fmt.Println(url.JoinPath("https://go.dev", "../x"))  // https://go.dev/../x
fmt.Println(url.JoinPath("https://go.dev/", "../x")) // https://go.dev/x

https://go.dev/play/p/gLLv0cc_jn1

JoinPath doesn't remove ../ path components appended to a domain that is not terminated by a slash. This is surprising and could conceivably lead to a directory traversal attack. The result of JoinPath shouldn't depend on whether the first component is / terminated or not.

Thanks to @q0jt for reporting this bug.

@neild neild added the Security label Aug 11, 2022
@neild neild self-assigned this Aug 11, 2022
@neild
Copy link
Contributor Author

neild commented Aug 11, 2022

This is CVE-2022-32190.

@q0jt
Copy link

q0jt commented Aug 11, 2022

Relative paths are now removed from elements with this patch #54390.

// OK
fmt.Println(url.JoinPath("https://go.dev", "../x")) // https://go.dev/x
fmt.Println(url.JoinPath("https://go.dev", "./x")) // https://go.dev/x

If a path contains whitespace or an invalid string at the beginning, it can be bypassed as follows.

- url.JoinPath("https://go.dev", "./../../x") // https://go.dev/../../x
- url.JoinPath("https://go.dev", "..;/../../../../../x") // https://go.dev/../../../../x
- url.JoinPath("https://go.dev", " ../../../../../../x") // https://go.dev/../../../../x

// OK
-  url.JoinPath("https://go.dev", "../.././x") // https://go.dev/x

cc @neild @cuishuang

@gopherbot
Copy link

gopherbot commented Aug 12, 2022

Change https://go.dev/cl/423514 mentions this issue: net/url: consistently remove ../ elements in JoinPath

@gopherbot
Copy link

gopherbot commented Aug 13, 2022

Change https://go.dev/cl/422715 mentions this issue: net/url: strip relative path components for JoinPath

@cuishuang
Copy link
Contributor

cuishuang commented Aug 16, 2022

Relative paths are now removed from elements with this patch #54390.

// OK
fmt.Println(url.JoinPath("https://go.dev", "../x")) // https://go.dev/x
fmt.Println(url.JoinPath("https://go.dev", "./x")) // https://go.dev/x

If a path contains whitespace or an invalid string at the beginning, it can be bypassed as follows.

- url.JoinPath("https://go.dev", "./../../x") // https://go.dev/../../x
- url.JoinPath("https://go.dev", "..;/../../../../../x") // https://go.dev/../../../../x
- url.JoinPath("https://go.dev", " ../../../../../../x") // https://go.dev/../../../../x

// OK
-  url.JoinPath("https://go.dev", "../.././x") // https://go.dev/x

cc @neild @cuishuang

Relative paths are now removed from elements with this patch #54390.

// OK
fmt.Println(url.JoinPath("https://go.dev", "../x")) // https://go.dev/x
fmt.Println(url.JoinPath("https://go.dev", "./x")) // https://go.dev/x

If a path contains whitespace or an invalid string at the beginning, it can be bypassed as follows.

- url.JoinPath("https://go.dev", "./../../x") // https://go.dev/../../x
- url.JoinPath("https://go.dev", "..;/../../../../../x") // https://go.dev/../../../../x
- url.JoinPath("https://go.dev", " ../../../../../../x") // https://go.dev/../../../../x

// OK
-  url.JoinPath("https://go.dev", "../.././x") // https://go.dev/x

cc @neild @cuishuang

Maybe refer to this pr https://go-review.googlesource.com/c/go/+/423514/

@q0jt
Copy link

q0jt commented Aug 16, 2022

Maybe refer to this pr https://go-review.googlesource.com/c/go/+/423514/

PR https://go-review.googlesource.com/c/go/+/423514/ has resolved this issue.
This fix is working properly, and therefore I mark this ticket as resolved.

Many thanks.

@seankhliao seankhliao added this to the Go1.20 milestone Aug 20, 2022
@neild
Copy link
Contributor Author

neild commented Aug 23, 2022

@gopherbot please open backport issues.

@gopherbot
Copy link

gopherbot commented Aug 23, 2022

Backport issue(s) opened: #54634 (for 1.18), #54635 (for 1.19).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link

gopherbot commented Aug 24, 2022

Change https://go.dev/cl/425357 mentions this issue: [release-branch.go1.19] net/url: consistently remove ../ elements in JoinPath

gopherbot pushed a commit that referenced this issue Aug 29, 2022
…JoinPath

JoinPath would fail to remove relative elements from the start of
the path when the first path element is "".

In addition, JoinPath would return the original path unmodified
when provided with no elements to join, violating the documented
behavior of always cleaning the resulting path.

Correct both these cases.

    JoinPath("http://go.dev", "../go")
    // before: http://go.dev/../go
    // after:  http://go.dev/go

    JoinPath("http://go.dev/../go")
    // before: http://go.dev/../go
    // after:  http://go.dev/go

For #54385.
Fixes #54635.
Fixes CVE-2022-32190.

Change-Id: I6d22cd160d097c50703dd96e4f453c6c118fd5d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/423514
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
(cherry picked from commit 0765da5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425357
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 6, 2022
bradfitz pushed a commit to tailscale/go that referenced this issue Sep 8, 2022
…JoinPath

JoinPath would fail to remove relative elements from the start of
the path when the first path element is "".

In addition, JoinPath would return the original path unmodified
when provided with no elements to join, violating the documented
behavior of always cleaning the resulting path.

Correct both these cases.

    JoinPath("http://go.dev", "../go")
    // before: http://go.dev/../go
    // after:  http://go.dev/go

    JoinPath("http://go.dev/../go")
    // before: http://go.dev/../go
    // after:  http://go.dev/go

For golang#54385.
Fixes golang#54635.
Fixes CVE-2022-32190.

Change-Id: I6d22cd160d097c50703dd96e4f453c6c118fd5d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/423514
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
(cherry picked from commit 0765da5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425357
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@wqwljz
Copy link

wqwljz commented Sep 15, 2022

I use versions earlier than 1.18, such as 1.17. Do I need to upgrade to 1.18?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants