fix(SFTP): infinite loop scanning directories with upward symlinks #52289
Conversation
Signed-off-by: Kent Delante <[email protected]>
leftybournes
requested review from
artonge,
nfebe and
skjnldsv
and removed request for
a team
sorbaugh
requested review from
artonge,
icewind1991 and
Altahrim
and removed request for
artonge
| $fileAdded = false; | ||
|
|
||
| if (!empty($linkedPath)) { | ||
| $fileAdded = substr_count($absPath, basename($absPath) . '/' . $file) > 0; |
There was a problem hiding this comment.
I think using substr_count is quite unsafe here.
It does not check that the linked path really is part of the parent path.
It can also lead to other funny problems when there are files and folders with similar names and your check would confuse a symlink at /dir123/sym to a file at /dir to cause the loop with /dir123 while it actually doesn't.
Besides that I'm not sure what you are checking here, since you are not involving the $linkedPath and it seems to me like this will always evaluate to true (but I might be wrong as I don't know how this exactly works).
There was a problem hiding this comment.
I'm comparing the symlink along with each child (e.g. sym/child_dir) done in basename($absPath) . '/' . $file against the current absolute path and it only checks if the current path is a symlink. Would the issue persist in that case?
From my testing, $linkedPath is a blank string when it's checked against a regular file or directory so checking if it's not empty evaluates to false in those cases as expected.
I see that my code lacks clarity. I'll make adjustments accordingly.
Sure, I'll add tests for this. |
Signed-off-by: Kent Delante <[email protected]>
The absolute path currently always starts with a '/' so check the current directory along with each symlinked child if it is already in the path (e.g. /parent/symlink/). Adding '/' to both ends helps with avoiding collisions with partial matches in the path. Signed-off-by: Kent Delante <[email protected]>
leftybournes
force-pushed
the
leftybournes/fix/sftp_scan_infinite_loop
branch
from
21b37b2 to
f9cfd2c
Compare
Signed-off-by: Kent Delante <[email protected]>
Summary
When a symlink is detected, check if the path already includes the file. If it does, ignore the file. This helps prevents the scanner from going into an infinite loop when the configured root contains a directory symlinked to a parent/grandparent/higher directory which eventually contains it.
Checklist