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.

Sign up for GitHub

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

Jump to bottom

VAULT-32330: fix sanitize path function to account for backslashes #28878

Merged
merged 8 commits into from
Nov 13, 2024
Merged

VAULT-32330: fix sanitize path function to account for backslashes #28878

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ff68b56
expand the leading prefix check to check for double forward and back …
aslamovamir Nov 11, 2024
2a78b54
improve the logic to be more concise
aslamovamir Nov 12, 2024
917f890
add unit tests
aslamovamir Nov 12, 2024
5db03a3
Merge branch 'main' into aslamovamir-vault-32330
aslamovamir Nov 12, 2024
0aa5c4f
add a changelog
aslamovamir Nov 12, 2024
05f38a0
make it a bug type
aslamovamir Nov 12, 2024
5dc2205
feedback: reconstruct the check to explicitly check for backslash as …
aslamovamir Nov 12, 2024
9805350
Merge branch 'main' into aslamovamir-vault-32330
aslamovamir Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions vault/logical_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6168,9 +6168,8 @@ func sanitizePath(path string) string {
path += "/"
}

for strings.HasPrefix(path, "/") {
path = path[1:]
}
// Remove all leading forward slashes and backslashes
path = strings.TrimLeft(path, "/\\")
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not be the correct based on my reading of the finding, i.e.
has a leading slash, but not that it does not have '/' or '\' in its second position.

For example, I think this would modify \string to string, but that it shouldn't. Based on my reading, I think this should only trim /, //, or /\.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be misunderstanding - the way I understood this was that we not only have to check for a leading "/" but also a "//" and "/". Is this what you're saying as well Violet?

Copy link
Contributor

Choose a reason for hiding this comment

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

My reading is that it needs to check for /, //, and /\ -- it shouldn't modify a path that starts with \

Copy link
Contributor Author

@aslamovamir aslamovamir Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the exact error from the security check: A redirect check that checks for a leading slash but not two leading slashes or a leading slash followed by a backslash is incomplete.

Copy link
Contributor

@divyaac divyaac Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Violet's comment is the way I understand it too. But since it's trimming the leading slash after it checks it, it looks there is some modification that needs to be done?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, so we don't want to modify strings that start with a backslash -- which TrimLeft will do

Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity here, the behavior we want is:

//my_string ==> my_string
/my_string ==> my_string
/\my_string ==> my_string
/my_string ==> /my_string
\\//my_string ==> \\//my_string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a commit to address the concerns, please do let me know if it looks better now, many thanks!


return path
}
Expand Down
28 changes: 28 additions & 0 deletions vault/logical_system_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7257,6 +7257,22 @@ func Test_sanitizePath(t *testing.T) {
path: "/mount/path/",
want: "mount/path/",
},
{
path: "/\\mount/path/",
want: "mount/path/",
},
{
path: "\\/mount/path/",
want: "mount/path/",
},
{
path: "//\\mount/path/",
want: "mount/path/",
},
{
path: "\\\\/mount/path/",
want: "mount/path/",
},
{
path: "",
want: "",
Expand All @@ -7269,6 +7285,18 @@ func Test_sanitizePath(t *testing.T) {
path: "///",
want: "",
},
{
path: "\\",
want: "",
},
{
path: "/\\\\",
want: "",
},
{
path: "\\\\/",
want: "",
},
}
for _, tc := range testCases {
tc := tc
Expand Down
Loading