Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Sanitize paths during archive creation and extraction #7108
Sanitize paths during archive creation and extraction #7108
Changes from all commits
438cf2e
db96c0c
b7ce3b1
518c4fb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
also, i am asking myself whether this is useful, it might completely change the path so it points somewhere else.
maybe rather reject than modify-and-accept?
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.
Which kind of paths should be accepted/rejected? I assume we still want to allow users to specify absolute paths and simple relative paths like a/b/c or ./a/b/c. So, are you suggesting to refuse any ../some/path?
Note that the regex starts with ^ and we only remove prefixes from the 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.
Guess we can approach this from 2 perspectives:
a) what does your PR change compared to the existing code
for archiving items the new
remove_dotdot_prefixes
is quite close to whatmake_path_safe
did, except that it removes leading slashes (which we must do somewhere) and handles the special cases of "nothing left" ("" and ".."), which is also fine (except that a mere "." as archived path is not really useful for extraction)b) what do we really want it to be
usually borg recurses starting from the recursion roots (which we can normalize first) and then only pretty normal paths will be generated by the recursion. for this, we do not need special path processing per item.
when fed with a paths list via stdin (and not using borg's recursor), borg does not have control over what's coming in from there, but borg is also not required to accept too crappy stuff and still make great sense from it (the admin or tool feeding that list into borg can be expected to provide reasonable paths), borg instead could skip invalid paths with an error msg.
So:
if normpath(p).startswith("../"): reject
.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.
I'm not sure I agree with it not being useful. When I create an archive for '.', I'd also expect that it contains '.' (user, permissions, attributes). So, that I can extract it again, move it to it's original location and permission are correct again.
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.
I, personally, wouldn't give this too much consideration. Seems unlikely to me that people will end up providing such crappy paths to Borg. Paths with './' or '//', sure, but something like "root/foo/../../../bar" seems rather unlikely to me.
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.
Handling of '.' we should perhaps be looked at separately. There are some cases where it isn't handled ideally:
Empty line at end is interpreted as '.' somehow.
Probably related to this:
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.
yeah, likely.
about archiving "." directory making sense: yes, i somehow agree, but how would you ever extract that? borg usually expects nothing being in the way, so it usually rmdirs target and then extracts target. not sure whether that would work with target == ".".
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.
Guess the main issue remaining here is that the code removes arbitrary amounts of leading
../
.So we basically archive something and lose information about its original path.
We won't remember whether it was
../foo
or../../foo
or whatever other upwards directory level.The original code before your PR also had this issue, but guess if we clean it up, we should do it right.
I guess the only context where this could make sense is if we want "no warnings / no errors" borg1 archive transfer and accept that information loss (preferring it over introducing some security issue into borg2 archives).
So, in case this is only meant for
borg transfer
, maybe it could be put there so it won't stay here forever?For borg2 "borg create", I guess we rather want to reject if something starts with
../
.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.
Hmm, thought more about this, and guess I have to correct myself: