Opened 7 years ago

Closed 5 years ago

#6994 closed (fixed)

Files created by FastCGI server are world-writable

Reported by: Antonis Christofides <anthony@…> Owned by: nobody
Component: Core (Management commands) Version: master
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When I run my site using ./manage.py runfcgi (with many more parameters), all files created by django are world-writable. This includes the PID file specified with the pidfile parameter, and any files uploaded through the Django admin in FileField's.

The reason for this appears to be the command "os.umask(0)" in line 16 of django/utils/daemonize.py (also in line 41 for non-posix systems). What is the purpose of that statement?

Attachments (2)

6994.patch (2.3 KB) - added by Antonis Christofides <anthony@…> 7 years ago.
Patch for properly setting umask
6994.2.patch (2.3 KB) - added by Antonis Christofides <anthony@…> 7 years ago.
Updated patch, default umask now 022 instead of 027

Download all attachments as: .zip

Change History (12)

Changed 7 years ago by Antonis Christofides <anthony@…>

Patch for properly setting umask

comment:1 Changed 7 years ago by Antonis Christofides <anthony@…>

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Files created by Django daemon are world-writable to Files created by FastCGI server are world-writable

I am not an expert on Unix daemons, so I made some research. I found out that setting the umask when daemonizing appears to be standard practice. However, there is some confusion concerning what you actually set it to. The Linux daemon-writing howto (http://www.netzmafia.de/skripten/unix/linux-daemon-howto.html) says to set it to zero, with the following rationale:

"By setting the umask to 0, we will have full access to the files generated by the daemon. Even if you aren't planning on using any files, it is a good idea to set the umask here anyway, just in case you will be accessing files on the filesystem."

This doesn't make much sense to me. Other texts, such as the howto daemonize, http://www-theorie.physik.unizh.ch/~dpotter/howto/daemonize, also recommend to set the umask to 0, without rationale. Finally, other texts recommend a nonzero value for the umask. For example, the Unix Daemon Server Programming, http://www.enderunix.org/docs/eng/daemon.php, says:

"Most servers runs as super-user, for security reasons they should protect files that they create. Setting user mask will prevent unsecure file priviliges that may occur on file creation."

and it gives, as an example, a umask of 027.

My conclusion is that it is good practice, when daemonizing, to properly reset the umask to a sane value; otherwise you inherit the parent's umask, and this might be insecure. But the actual value to which you set the umask to should be sane, and definitely nonzero. I don't know why some people recommend setting it to zero, but I dare wildly guess that an error was once made in some howto and then it has been copied generously. I can't find any other explanation.

Since there is no single umask value that can fit all cases (for example, I think 027 is reasonable, but in my installation I happen to need 002), it must be possible to override. I therefore wrote a patch where you can specify the umask as a runfcgi option, and the default is a sane 027.

Changed 7 years ago by Antonis Christofides <anthony@…>

Updated patch, default umask now 022 instead of 027

comment:2 Changed 7 years ago by Antonis Christofides <anthony@…>

I think that using a umask of 027 may break backwards compatibility and create unexpected results. Specifically, uploaded files would now be unreadable by "other", which usually includes the web server, which means that in some installations newly uploaded files might be unreadable until the umask is set. So I changed the default umask to 022 and uploaded an updated patch.

comment:3 Changed 7 years ago by Simon Greenhill

  • Needs documentation set
  • Triage Stage changed from Unreviewed to Ready for checkin

comment:4 follow-up: Changed 7 years ago by aptiko

Hi Simon,

when you check "needs documentation", is this something for me? I _have_ provided documentation (albeit only one line), the topmost modification in the patch. Is more than that needed?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 7 years ago by telenieko

Replying to aptiko:

when you check "needs documentation", is this something for me? I _have_ provided documentation (albeit only one line), the topmost modification in the patch. Is more than that needed?

Errr... documentation does not mean comments, the contributing doc says it all, patches should have (when applicable):

  • The patch itself (code changes).
  • Tests that reproduce the bug fixed (and hence, pass) or that test the new functionality
  • Documentation, changes to the docs/ folder so users see those changes.

comment:6 in reply to: ↑ 5 Changed 7 years ago by aptiko

Replying to telenieko:

Errr... documentation does not mean comments, the contributing doc says it all, patches should have (when applicable):

  • Documentation, changes to the docs/ folder so users see those changes.

The docs/ directory does not document any of runfcgi's arguments: there is nothing about workdir, outlog or errlog in there. The only thing that is mentioned is (http://www.djangoproject.com/documentation/fastcgi/):

"If you specify help as the only option after runfcgi, it’ll display a list of all the available options."

The modifications I've provided include documentation of "umask" in "runfcgi help". If a one-liner there is enough for the "workdir" parameter (and in my opinion it is), then a one-liner about "umask" should also suffice.

comment:7 Changed 7 years ago by mtredinnick

Good catch. I'm a bit embarrassed to have missed this when the initial idea was floated. 022 is a pretty normal umask value in these situations, so it's a good default. Thanks for the patch.

comment:8 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [7800]) Fixed #6994 -- For fastcgi, set a more sensible default umask.
Also allow the umask value to be customised. Thanks, Antonis Christofides.

comment:9 Changed 5 years ago by petteyg359@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

The umask option is converted to an int, instead of taken as typed.

./manage.py runfcgi umask=0111 creates a socket with umask 0157.
umask=0022 creates a socket with umask 0026.
umask=0027 creares a socket with umask 0033.

A user on IRC suggested use of ast.literal_eval() instead.

comment:10 Changed 5 years ago by aptiko

  • Resolution set to fixed
  • Status changed from reopened to closed

I am not absolutely certain of the trac workflow used in Django, but in most software projects we don't reopen fixed bugs when the fix turns out to be buggy; instead, we file new bugs. The original bug was that the files are world-writeable, whereas the new bug is that the new configuration option does not work properly.

Therefore, please file this as a new bug (and CC me since I'm the author of the buggy code).

Note: See TracTickets for help on using tickets.
Back to Top