Attachements are readable by every shell user

[rcubetrac]

Reported by quabla on 15 Mar 2013 15:11 UTC as Trac ticket #1488996

Using the default way to handle uploaded attachments (plugins/filesystem_attachements) the attachment is stored in a tmp directory (/tmp/) with read access for every user on the system [seems to imply that on every system that has a default setup of roundcube (checked versions 0.72-master), say every ssh user can read all attachments.

The error is still present with a suitable umask. This seems to be a known behavior (bug?) of PHPs move_uploaded_file [2](1].

This). I suggest to add something like

if (move_uploaded_file($args[$tmpfname) && file_exists($tmpfname)) {

chmod($tmpfname, 0640);
$args'id' = $this->file_id();

to plugins/filesystem_attachments/filesystem_attachments.php. This fixed the problem for me. Still I would appreciate a review of all functions that create files to ensure that they are not readable by others, irrespective of the umask.

[Tested on a Debian Wheezy system with Apache2, PHP5 (fcgid)
2 http://php.net/manual/en/function.move-uploaded-file.php, see comments

Migrated-From: http://trac.roundcube.net/ticket/1488996

[rcubetrac]

Comment by @alecpl on 15 Mar 2013 16:22 UTC

I suppose, you should set appropriate privileges on Roundcube temp directory. Will investigate your proposal.

[rcubetrac]

Milestone changed by @alecpl on 15 Mar 2013 16:22 UTC

later => 0.9-stable

[rcubetrac]

Comment by quabla on 15 Mar 2013 17:50 UTC

Replying to alec:

I suppose, you should set appropriate privileges on Roundcube temp directory.

That's what I did in the past. But there are two reasons why this is not a solution

1. That doesnt fixes that the default behavior is less secure than it could be.
2. There are security reasons to use /tmp/ as tempdir (different mount options). Creating a /tmp/roundcube directory is a bad idea either since all contents of /tmp/ are deleted on reboot.

[rcubetrac]

Comment by @alecpl on 15 Mar 2013 18:00 UTC

In my system (Ubuntu, mod_php) uploaded files in Roundcube temp_dir have 0600 rights. So, maybe it's system-specific (fcgi?). Additionally I've found that files created by rcube_image class (thumbnails, resized images) have 0644 and this we should fix by using chmod() for sure.

[rcubetrac]

Comment by aberglund on 15 Mar 2013 19:57 UTC

FWIW, the bulk of the files in my system (RHEL5) are also 600.

[rcubetrac]

Comment by quabla on 15 Mar 2013 20:16 UTC

I have checked the code for PHPs move_uploaded_file and it respects the umask. For me the problem is caused by a apache2 running with a bad umask seems to be Debians default. However I would still recommend to fix this in roundcube. Maybe by introducing a umask setting which defaults to 600 and is also used to fix the rcube_image stuff described above.

[rcubetrac]

Comment by @alecpl on 27 Mar 2013 12:07 UTC

I'm still not sure what (and if) to do with this. From my observation, all files except these handled by move_uploaded_file() are created according to configured umask. Uploads are created with 0600. Yes, default in Ubuntu is 0644, but it can be changed. So, if it's always true, we don't need to do anything in the code. All is a matter of configuration.[[BR]]
If we decide to set umask in the code, is adding

umask(0077);

on the beginning of scripts execution a good solution? I'm afraid not, according to PHP manual about umask().

[rcubetrac]

Comment by @thomascube on 10 Apr 2013 21:41 UTC

Fixed in commit [and backported to release-0.9 in commit 2741d8e(b413bb2])

[rcubetrac]

Status changed by @thomascube on 10 Apr 2013 21:41 UTC

new => closed