Fix symlink issues when scanning for files #21723

adrium · 2020-07-06T22:32:49Z

Description

Hi, there are issues with the file scanner and symlinks.
This PR fixes the ones that are crashing the files:scan command for local storage:

Inexisting symlink targets
Symlinks pointing outside the data directory

I am using Nextcloud more like a GUI to browse files online and do the sync or copy by other means.
Hence, some of the symlinks are not valid anymore when copied to the storage folder.

With the fixes applied, I was finally able to run a full scan of all files:

Folders: 46353
Files: 433857
Elapsed time: 00:20:28

The commits may not pass all checks when submitting.
I will try to update the code and force-push the branch with improvements.
Let me know about other adjustments so that the code is ready for merging.

Case 1

The scanner uses OC\Files\Storage\Storage::getDirectoryContent() which calls OC\Files\Storage\Local::getMetaData() for local storage to populate the array of children.
Because invalid symlinks are not readable, the array can contain null which is not expected by OC\Files\Cache\Scanner::handleChildren()

The PR fixes this by ignoring unreadable entries.

Case 2

OC\Files\Storage\Local::getMetaData() uses OC\Files\Storage\Local::getSourcePath() to resolve the filename.
The method can throw a ForbiddenException if the path is outside the data directory which is not handled gracefully.
Since null is returned, if the file is not readable, it is consistent to return null in that case as well.

kesselb

Thank you for taking the time to debug and submit a patch 👍

kesselb · 2020-07-07T12:00:12Z

lib/private/Files/Storage/Local.php

-		$fullPath = $this->getSourcePath($path);
+		$fullPath = null;
+		try {
+			$fullPath = $this->getSourcePath($path);


Please see my comment for Local.php below. It's getting a bit longer...

kesselb · 2020-07-07T12:03:18Z

lib/private/Files/Cache/Scanner.php

@@ -415,6 +415,10 @@ private function handleChildren($path, $recursive, $reuse, $folderId, $lock, &$s
 		$childQueue = [];
 		$newChildNames = [];
 		foreach ($newChildren as $fileMeta) {
+			if ($fileMeta === null) {
+				continue;


I'm unsure. If we are not able to process symlinks properly getDirectoryContent should not return them.

server/lib/private/Files/Storage/Common.php

Line 877 in da2d425

if (!Filesystem::isIgnoredDir($file) && !Filesystem::isFileBlacklisted($file)) {

We might add is_link check there and not yield the meta data. Probably is_link could be added to isFileBlacklisted if every symlink is actually forbidden (cc @icewind1991)

Please see my comment below. It's getting a bit longer...

Ref https://github.com/nextcloud/server/pull/21789/files

adrium · 2020-07-07T19:13:58Z

Hey @kesselb Thanks a lot for your review! I gave this some thought...

It makes sense to return only readable data from getDirectoryContent.
However, leaving this PR aside, there is still the possibility that getMetaData returns null:

In Local.php: if (!@stat($fullPath))
In Common.php: if (!$this->getPermissions($path) & \OCP\Constants::PERMISSION_READ)

And by the way, in Common.php it is written: can't read, nothing we can do, return null so it seems that getting null from this function is valid. (Documentation fix necessary?)

Hence, it seams reasonable to fix it in getDirectoryContent and I would propose to directly filter out the null values instead of other checks, because:

is_link alone is not sufficient, because symlinks are followed in the current implementation (there are other bugs related to that)
isFileBlacklisted does not seem appropriate because it seems to be a config-related function
isIgnoredDir could be considered
- But then the problem with getMetaData returning null will occur again

J0WI · 2020-07-08T17:26:51Z

I just found another symlink issue. If you scanned a folder that contains symlinks and try to delete the folder afterwards in Nextcloud, it will fail with the following error: rmdir(/var/www/html/data/admin/files/path/to/symlink): Directory not empty at /var/www/html/lib/private/Files/Storage/Local.php#115

adrium · 2020-07-09T07:19:55Z

Thanks for your report. This PR tries to fix issues with the scanner.
What happens if you delete the file by other means (rm on the server) and run files:scan again?

kesselb · 2020-10-22T18:15:43Z

@adrium would you mind to rebase this branch and solve the conflicts? I will request another review then. Sorry it took so long. Thanks again for debugging 👍

adrium · 2020-11-13T19:03:33Z

I am not using nextcloud for this anymore, but I can try set up another test case, if there is interest.

Please give me about two weeks.

adrium · 2021-01-03T23:33:18Z

Hey, I worked on the PR again and tested it in the VM. It also fixes cyclic symlinks now!

Inexisting symlink targets (fixed thanks to change in filter files containing a hash in the path for ftp storages #21789)
Symlinks pointing outside the data directory (fixed)
- Same fix as initial PR by catching the ForbiddenException
Cyclic symlinks (fixed)
- Fixed by detecting a parent path. It is deliberate to not introduce new functions such as is_link()
- Also throwing ForbiddenException

What to do about the DCO check? Do I just have to repeat my name in the commit message?

The below script can be used to create files to test the scanner.

NCDATA=/mnt/ncdata
NCUSER=admin

NCTEST=$NCDATA/$NCUSER/files/Tests

DIR=$NCTEST/valid
mkdir -p $DIR
echo '# Valid links' > $DIR/README.md
ln -s ../../Photos $DIR/photos
ln -s ../../Photos/Library.jpg $DIR/file.jpg

# Case 1
DIR=$NCTEST/invalid
mkdir -p $DIR
echo '# Test for invalid link target' > $DIR/README.md
ln -s /invalid $DIR/link1
ln -s /tmp/invalid $DIR/link2

# Case 2
DIR=$NCTEST/outside
mkdir -p $DIR
mkdir /tmp/hello
echo '# Test for link target outside the user folder' > $DIR/README.md
echo '# Hello World' > /tmp/hello/README.md
ln -s /tmp/hello $DIR/link

# Case 3
DIR=$NCTEST/cyclic
mkdir -p $DIR
echo '# Test link pointing to parent' > $DIR/README.md
mkdir -p $DIR/directory/files
ln -s ../../directory $DIR/directory/files/parent

kesselb · 2021-01-08T09:30:53Z

To reference:
#24989
#22943

I'm not sure if we need this patch anymore. That needs some testing.

adrium · 2021-01-09T10:58:53Z

Hi, from the first look at OC\Files\Storage\Local::getSourcePath() it can still throw a ForbiddenException which is not handled in getMetaData() so I believe this is still relevant.

Do you need support with testing?

AndreKoepke · 2021-02-27T17:17:06Z

@icewind1991 and @kesselb what is the current status? Can we help?

BUFU1610 · 2021-08-23T08:11:48Z

Just one quick question: Is this still a problem now?
How has this not been reviewed in half a year otherwise?

skjnldsv · 2024-02-27T13:14:16Z

Just one quick question: Is this still a problem now? How has this not been reviewed in half a year otherwise?

It seems like this isn't such an issue for most people and failed to gain attraction :(
As there is no feedback since a while I will close this ticket.
If you will decide to work on this feature again and if it hasn't been fixed or implemented already, feel free to re-open and solve the various conflicts.

Thanks for the interest in Nextcloud and the effort put into this! 🙇

MorrisJobke requested a review from icewind1991 July 7, 2020 07:44

kesselb reviewed Jul 7, 2020

View reviewed changes

kesselb mentioned this pull request Sep 14, 2020

Trying to access array offset on value of type null at /var/www/html/lib/private/Files/Cache/Scanner.php#418 #22846

Closed

solracsf linked an issue Sep 15, 2020 that may be closed by this pull request

Trying to access array offset on value of type null at /var/www/html/lib/private/Files/Cache/Scanner.php#418 #22846

Closed

x86-39 mentioned this pull request Oct 6, 2020

Exception during scan, Filename too long. Directory name has 4000+ '/' characters according to Nextcloud, these characters do not actually appear in the name. #22580

Closed

skjnldsv added 2. developing Work in progress bug feature: filesystem labels Oct 31, 2020

Ignore symlinks outside user folder

7fc952c

adrium force-pushed the fix-symlinks-scanner branch 2 times, most recently from 3e5f43e to 7bcd87b Compare January 3, 2021 20:48

Forbid cyclic symlinks

191c57e

adrium force-pushed the fix-symlinks-scanner branch from 7bcd87b to 191c57e Compare January 3, 2021 20:55

Handle special path names

2439b7c

adrium force-pushed the fix-symlinks-scanner branch from e77fa75 to 2439b7c Compare January 3, 2021 23:14

adrium requested a review from kesselb January 3, 2021 23:33

J0WI added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 5, 2021

skjnldsv closed this Feb 27, 2024

skjnldsv removed the 3. to review Waiting for reviews label Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix symlink issues when scanning for files #21723

Fix symlink issues when scanning for files #21723

adrium commented Jul 6, 2020

kesselb left a comment

kesselb Jul 7, 2020

adrium Jul 7, 2020

kesselb Jul 7, 2020

adrium Jul 7, 2020 •

edited

Loading

kesselb Jul 10, 2020

adrium commented Jul 7, 2020

J0WI commented Jul 8, 2020

adrium commented Jul 9, 2020

kesselb commented Oct 22, 2020

adrium commented Nov 13, 2020

adrium commented Jan 3, 2021

kesselb commented Jan 8, 2021

adrium commented Jan 9, 2021

AndreKoepke commented Feb 27, 2021

BUFU1610 commented Aug 23, 2021

skjnldsv commented Feb 27, 2024

Fix symlink issues when scanning for files #21723

Fix symlink issues when scanning for files #21723

Conversation

adrium commented Jul 6, 2020

Description

Case 1

Case 2

kesselb left a comment

Choose a reason for hiding this comment

kesselb Jul 7, 2020

Choose a reason for hiding this comment

adrium Jul 7, 2020

Choose a reason for hiding this comment

kesselb Jul 7, 2020

Choose a reason for hiding this comment

adrium Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

kesselb Jul 10, 2020

Choose a reason for hiding this comment

adrium commented Jul 7, 2020

J0WI commented Jul 8, 2020

adrium commented Jul 9, 2020

kesselb commented Oct 22, 2020

adrium commented Nov 13, 2020

adrium commented Jan 3, 2021

kesselb commented Jan 8, 2021

adrium commented Jan 9, 2021

AndreKoepke commented Feb 27, 2021

BUFU1610 commented Aug 23, 2021

skjnldsv commented Feb 27, 2024

adrium Jul 7, 2020 •

edited

Loading