Repository navigation
Conversation
|
Also #41563 |
|
Thank you for your contribution! Please follow our AI policy: https://github.com/nextcloud/.github/blob/master/AI_POLICY.md#disclosure |
SUMMARY On a local storage, symlinks to a directories are followed even if the directory is a parent directory or the directory itself. Under these conditions, the Nextcloud metadata scanner continues traversing the tree until it reaches the limit. Re-entering the same tree in loop. Basically it will fill the oc_filecache SQL table with a new entry until the path reach the limit of oc_filecache.path column varchar(4000) Impact We had a server crash because Nextcloud filled the database disk by creating 30M row in this table and producing a 185 GB database. This is a DoS to become that can be triggered just by creating a symlink. History The bug is known since at least 2017 see nextcloud#6395 (SMB), nextcloud#20197, nextcloud#23022 A previous fix nextcloud#21723 was closed unmerged. Proposed Fix Local::getDirectoryContent() will skip a symlink whose target is the directory being listed or a parent directory. We are comparing both path after resolution so that we also catches cycles ( a -> b, b->a ). Signed-off-by: NK <nicolas.devillers@airbus.com> Assisted-by: ClaudeCode:claude-fable-5
2b4ab6f to
ea4a989
Compare
|
done |
| if ($metadata['mimetype'] === FileInfo::MIMETYPE_FOLDER) { | ||
| try { | ||
| $childSource = $this->getSourcePath(rtrim($directory, '/') . '/' . $metadata['name']); | ||
| } catch (ForbiddenException) { | ||
| // Retargeted outside the datadir since listed; drop it like getMetaData() would. | ||
| continue; | ||
| } | ||
| if (is_link($childSource)) { | ||
| $childReal = realpath($childSource); | ||
| if ($childReal !== false) { | ||
| // Built lazily, only once a symlinked directory shows up. | ||
| $ancestors ??= $this->getAncestorRealPaths($directory); | ||
| if (isset($ancestors[rtrim($childReal, '/')])) { | ||
| Server::get(LoggerInterface::class)->warning( | ||
| "Skipping looping directory symlink '$childSource' -> '$childReal'", | ||
| ['app' => 'core'] | ||
| ); | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Please invert the if statements and make them early-continues. That will be much more readable.
| * Resolved paths of $directory and each of its ancestors up to the storage | ||
| * root, as a set. A directory symlink resolving to any of them closes a loop. | ||
| */ | ||
| private function getAncestorRealPaths(string $directory): array { |
There was a problem hiding this comment.
I'm not sure if this is not overcomplicated. I think it should be enough to check that the child real path doesn't match any of the parent paths, so splitting the path and then building every parent would be enough (without accessing the filesystem, just with string manipulation).
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
Hello, that fix is actually not good. We met the same issue again this week, filling our database again.. e.g: leading to loop such as: I'm actually not sure about the interest of following symlink at all. If there really is one, we probably want to come up wih a solution such as keeping a list of crossed inodes or using substr(realpath) as nextcloud is currently doing for checking symlinks points inside the datadir |
|
our current patch now do something like this : public function getDirectoryContent(string $directory): \Traversable {
$allowSymlinks = $this->config->getSystemValueBool('localstorage.allowsymlinks', false);
foreach (parent::getDirectoryContent($directory) as $metadata) {
if (!$allowSymlinks && $metadata['mimetype'] === 'httpd/unix-directory') {
$childSource = $this->datadir . rtrim($directory, '/') . '/' . $metadata['name'];
if (is_link($childSource)) {
Server::get(LoggerInterface::class)->debug(
"Skipping directory symlink '$childSource' during scan",
['app' => 'core']
);
continue;
}
}
yield $metadata;
}
} |
SUMMARY
On a local storage, symlinks to a directories are followed even
if the directory is a parent directory or the directory itself.
Under these conditions, the Nextcloud metadata scanner
continues traversing the tree until it reaches the limit.
Re-entering the same tree in loop.
Basically it will fill the oc_filecache SQL table with a new entry
until the path reach the limit of oc_filecache.path column varchar(4000)
Impact
We had a server crash because Nextcloud filled the database disk
by creating 30M row in this table and producing a 185 GB database.
This is a DoS to become that can be triggered just by creating a symlink.
History
The bug is known since at least 2017 see #6395 (SMB), #20197, #23022
A previous fix #21723 was closed unmerged.
Proposed Fix
Local::getDirectoryContent() will skip a symlink whose target is
the directory being listed or a parent directory.
We are comparing both path after resolution so that we also catches
cycles ( a -> b, b->a ).