per_user_logging: username used as a path component without basename()

Open Beginner friendly
#10,331 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
78/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
php
Domain
backend, security

Research direction

Start in program/lib/Roundcube/rcube.php at get_user_log_dir(), and trace how it is reached when per_user_logging is enabled. Verify that usernames containing traversal components can no longer place logs outside the configured log_dir, while valid usernames still resolve to writable per-user directories; check existing PHP tests for the appropriate coverage location.

Written by the indexing model from the issue text.

Description

Summary

get_user_log_dir() concatenates the authenticated username into a filesystem path without basename() or any traversal filtering. A username containing ../ resolves outside the configured log_dir.

Affected code

program/lib/Roundcube/rcube.php:

protected function get_user_log_dir()
{
    $log_dir = $this->config->get('log_dir', RCUBE_INSTALL_PATH . 'logs');
    $user_name = $this->get_user_name();
    $user_log_dir = $log_dir . '/' . $user_name;

    return !empty($user_name) && is_writable($user_log_dir) ? $user_log_dir : false;
}

Only reachable when $config['per_user_logging'] is enabled, which is not the default.

Evidence

Measured on current master, PHP 8.5.10, inside a private probe tree:

sanitised with basename(): NO
gated by is_writable():    YES

username                 resulting path                    accepted?
alice                    <root>/logs/alice                 no
../elsewhere             <root>/logs/../elsewhere          YES -> <root>/elsewhere
../does-not-exist        <root>/logs/../does-not-exist     no
../../../../tmp          <root>/logs/../../../../tmp       no
bob/../../elsewhere      <root>/logs/bob/../../elsewhere   no
Impact — deliberately stated narrowly

I want to be precise about how limited this is, because the is_writable() check does most of the work:

  • The traversal target must already exist and be writable by the web server user. ../does-not-exist is rejected; no directory is created.
  • The only case that succeeded in my probe was ../elsewhere, where I had created that directory beforehand.
  • It requires a username containing ../ to be provisioned in the IMAP or directory backend, which is outside Roundcube's control.

So this is not an arbitrary-file-write primitive. What it is: a missing input constraint that lets log output land outside the intended directory when both preconditions happen to line up. I'd classify it as hardening rather than a vulnerability, which is why I'm filing it publicly rather than reporting it privately.

Suggested fix

Constrain the path component:

$user_name = basename($this->get_user_name());

or, stricter, reject usernames that are not a safe filename before using them as a path component. basename() alone already neutralises every case in the table above.

Happy to send a PR.

Dominant language
PHP
Stars
7.2k
Forks
1.8k
Avg merge
5d 12h
Merged PRs (30d)
2

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from roundcube/roundcubemail

All issues in roundcube/roundcubemail

Similar issues

More PHP issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.