VAULT-32330: fix sanitize path function to account for backslashes #28878
Conversation
github-actions
bot
added
the
hashicorp-contributed-pr
|
CI Results: |
|
Build Results: |
aslamovamir
added
backport/ent/1.16.x+ent
There was a problem hiding this comment.
A little comment so far. Remember also to add a title to the PR to briefly describe what it does, and a changelog will also need to be added to this PR.
I think the milestone is incorrect, as 1.16.0 has already released. My recommendation for a PR like this would be to use the pr/no-milestone tag for this PR as it shouldn't block any release. I would recommend only using milestones when this PR must be in an upcoming release.
vault/logical_system.go
Outdated
| path = path[1:] | ||
| } | ||
| // Remove all leading forward slashes and backslashes | ||
| path = strings.TrimLeft(path, "/\\") |
There was a problem hiding this comment.
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 /\.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
My reading is that it needs to check for /, //, and /\ -- it shouldn't modify a path that starts with \
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Right, so we don't want to modify strings that start with a backslash -- which TrimLeft will do
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Pushed a commit to address the concerns, please do let me know if it looks better now, many thanks!
Ahh nice notes, thanks Violet! |
|
I would label this as an For backporting, just search |
aslamovamir
added
pr/no-milestone
backport/ent/1.16.x+ent
aslamovamir
changed the title
…well as slash followed by backslash
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for strings.HasPrefix(path, "/") { | ||
| path = path[1:] | ||
| // Check for the specified prefixes and trim them if present | ||
| for strings.HasPrefix(path, "/") || strings.HasPrefix(path, "/\\") { |
There was a problem hiding this comment.
This seems to satisfy the requirements, it may be worth double checking with Mickael or somebody to make sure this is accurate, since the code scanner seems to still be complaining. I'll hook you two up :)
aslamovamir
added
backport/ent/1.17.x+ent
Description
What does this PR do?
Fixes a backslash check in logical_system.go by expanding the check to also include filtering for double forward and back slashes.
TODO only if you're a HashiCorp employee
to N, N-1, and N-2, using the
backport/ent/x.x.x+entlabels. If this PR is in the CE repo, you should only backport to N, using thebackport/x.x.xlabel, not the enterprise labels.of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
in the PR description, commit message, or branch name.
description. Also, make sure the changelog is in this PR, not in your ENT PR.