Skip to content

fix(user): confine the home folder naming rule to the data directory #849

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.

Sign up for GitHub

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

Merged
DeepDiver1975 merged 3 commits into from
Jul 28, 2026
+441 −7
Merged

fix(user): confine the home folder naming rule to the data directory #849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7df5d4
fix(user): confine the home folder naming rule to the data directory
DeepDiver1975 Jul 28, 2026
e2091e7
chore: correct the changelog PR reference to #849
DeepDiver1975 Jul 28, 2026
1f7034b
fix(user): refuse a base directory that is not an absolute path
DeepDiver1975 Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)

## [Unreleased] - XXXX-XX-XX

### Fixed

- [#849](https://github.com/owncloud/user_ldap/pull/849) - Confine the LDAP home folder naming rule to the data directory


## [0.20.2] - 2026-07-27

Expand Down
135 changes: 130 additions & 5 deletions lib/User/UserEntry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ public function getEMailAddress() {
* returns the home directory of the user if specified by LDAP settings
* @return string|null
* @throws \Exception if a naming rule attribute is enforced, but it doesn't exist for that LDAP user
* @throws \OutOfBoundsException if the configured attribute points outside of the permitted base directories
*/
public function getHome() {
$path = '';
Expand All @@ -274,15 +275,53 @@ public function getHome() {
}

if ($path !== '') {
$dataDir = $this->config->getSystemValue(
'datadirectory',
\OC::$SERVERROOT.'/data'
);
//if attribute's value is an absolute path take this, otherwise append it to data dir
//check for / at the beginning
if ($path[0] !== '/') {
$path = $this->config->getSystemValue(
'datadirectory',
\OC::$SERVERROOT.'/data'
) . '/' . $path;
$path = $dataDir . '/' . $path;
}
return $path;

// The value comes from the LDAP directory, which is not necessarily
// under the control of the ownCloud administrator. Left unchecked, a
// home pointing at the ownCloud code root turns the user's file
// listing into read/write access to the application's own PHP files,
// i.e. remote code execution. Confine it to the data directory, and
// to any additional base directories the admin opted into.
$path = self::normalizePath($path);
$baseDirs = $this->config->getSystemValue('user_ldap.home_base_dirs', []);
if (!\is_array($baseDirs)) {
$baseDirs = [];
}
\array_unshift($baseDirs, $dataDir);

foreach ($baseDirs as $baseDir) {
// A base directory is only meaningful as an absolute path. Anything
// else is a misconfiguration and must not widen the comparison: '.'
// and './' would normalize to the empty string, which makes the
// containment check below accept every absolute path, and a relative
// value would resolve against the working directory of whichever
// process happens to run this check.
if (!\is_string($baseDir) || !isset($baseDir[0]) || $baseDir[0] !== '/') {
continue;
}
if (self::isContainedIn($path, self::normalizePath($baseDir))) {
return $path;
}
}

$this->logger->error(
"Home dir <$path> for uid <{$this->getUserId()}> is outside of the data directory" .
" <$dataDir> and of any directory listed in the 'user_ldap.home_base_dirs' config" .
" option - refusing it",
['app' => 'user_ldap']
);
throw new \OutOfBoundsException(
'Home dir read from LDAP is not inside a permitted base directory for uid: ' . $this->getUserId()
);
}

// TODO use OutOfBoundsException and https://github.com/owncloud/core/pull/28805
Expand All @@ -298,6 +337,92 @@ public function getHome() {
return null;
}

/**
* Resolve a path to a canonical form for comparison: symlinks are resolved on
* the longest leading part that exists on disk, the remainder is normalized
* lexically. realpath() alone cannot be used here because a home directory is
* created lazily on first login, so it usually does not exist yet at the time
* it is validated - but resolving the existing prefix keeps a symlink from
* pointing an apparently contained home somewhere else, and lets an admin have
* a symlinked data directory.
*
* Callers are responsible for passing an absolute path: a relative one would be
* resolved against the working directory of whichever process happens to run
* this, and '' and '.' normalize to the empty string.
*
* @param string $path an absolute path
* @return string the resolved path, without a trailing slash
*/
private static function normalizePath($path) {
$lexical = self::normalizePathLexically($path);

$suffix = [];
$candidate = $lexical;
while ($candidate !== '' && $candidate !== '/') {
$real = \realpath($candidate);
if ($real !== false) {
return \rtrim($real, '/') . (empty($suffix) ? '' : '/' . \implode('/', $suffix));
}
\array_unshift($suffix, \basename($candidate));
$parent = \dirname($candidate);
if ($parent === $candidate) {
break;
}
$candidate = $parent;
}

return $lexical;
}

/**
* Resolve '.', '..' and duplicate slashes without touching the filesystem.
*
* @param string $path
* @return string the normalized path, without a trailing slash
*/
private static function normalizePathLexically($path) {
$isAbsolute = isset($path[0]) && $path[0] === '/';

@jvillafanez jvillafanez Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the $path won't be empty in this case, it seems better to either ensure it won't happen here, or document that is the caller's responsibility to ensure that the $path isn't empty.

@DeepDiver1975 DeepDiver1975 Jul 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good instinct — following it up turned out to find a real bug, not just a missing doc line.

You're right that $path (the home) can't be empty here: it is guarded absolute before normalizePath() is reached. But the other argument was not. getHome() skipped an empty base dir like this:

if (!\is_string($baseDir) || $baseDir === '') {
    continue;
}
if (self::isContainedIn($path, self::normalizePath($baseDir))) {

The guard ran before normalization — and ., ./ and ./. normalize to the empty string, so they slipped past it. isContainedIn() then computed:

\strpos($path, \rtrim('', '/') . '/') === 0   //  strpos($path, '/') === 0

which is true for every absolute path. A single stray '.' in user_ldap.home_base_dirs silently disabled the containment check completely.

It's also worth noting that an empty value is asymmetric — which is exactly why your comment was worth chasing:

isContainedIn('',     '/var/www/owncloud/data') = false   <- fail-safe
isContainedIn('/etc', '')                       = true    <- fail-OPEN

Harmless in the first argument, disabling in the second.

Fixed in 1f7034b by requiring the base dir to be an absolute path before normalizing it. One condition covers '', '.', './', './.', '..' and non-strings, and it additionally rules out a relative entry such as 'data' — realpath() would have resolved that against the working directory of whichever process ran the check, so the answer would differ between the web server and occ.

Then both halves of what you asked for:

  • Ensured it won't happen: isContainedIn() now returns false for an empty $baseDir in its own right, so the predicate can't fail open even if a future caller forgets to guard.
  • Documented the responsibility: both normalizePath() docblocks now state that passing an absolute path is the caller's job, and why.

Verified with a negative control — reverting only the guard reproduces exactly the three '.'-shaped failures and nothing else; restoring it turns them green. owncloud/core#41752 had the identical defect in SyncService::verifyHomeLocation() and is fixed the same way.

New tests: testGetHomeIsRefusedForUnusableBaseDir (8 cases).

$parts = [];
foreach (\explode('/', $path) as $part) {
if ($part === '' || $part === '.') {
continue;
}
if ($part === '..') {
// a leading '..' cannot escape the root, mirroring the kernel
if (\end($parts) === '..') {
$parts[] = $part;
} elseif (\array_pop($parts) === null && !$isAbsolute) {
$parts[] = $part;
}
continue;
}
$parts[] = $part;
}
return ($isAbsolute ? '/' : '') . \implode('/', $parts);
}

/**
* Whether $path is $baseDir itself or lies underneath it. Both arguments are
* expected to be normalized already.
*
* @param string $path
* @param string $baseDir
* @return bool
*/
private static function isContainedIn($path, $baseDir) {
// an empty base dir contains nothing - without this the comparison below
// degenerates into strpos($path, '/') === 0, accepting every absolute path
if ($baseDir === '') {
return false;
}
if ($path === $baseDir) {
return true;
}
// compare against the separator as well, so that a sibling directory
// sharing the base dir's prefix (/data-evil vs /data) is not accepted
return \strpos($path, \rtrim($baseDir, '/') . '/') === 0;
}

/**
* @brief reads the image from LDAP that shall be used as Avatar
* @return string|null image binary data (provided by LDAP)
Expand Down
Loading