log file reopen should pass opened fd from master process

[Tiziano Müller]

When starting nginx all the log files (error_log, access_log) are created and opened by the master process and the filehandles passed to the worker while forking.

On SIGUSR1 the master reopens the files, chown's them and then the worker reopens the files himself. This has several drawbacks:

• It is inconsistent behaviour and rather surprising (sudden change of ownership upon signal). If you really want to do it this way you should chown the files from the very beginning.
• It permits the unprivileged nginx user read and write access to the current log files which is bad from the security perspective since the unprivileged user also needs to be able to change into/read the log directory

A better solution may be to reopen the log files in the master process as currently done and then use the already available ngx_{read,write}_channel functions to pass the new filehandles down to the worker.

[Maxim Dounin]

Current behaviour is as follows:

• Do not attempt to do anything with logs ownership/permissions on normal startup.
• Chown logs to a user nginx is configured to run workers on logfile quick reopen signal (USR1).

This allows to use any permissions desired as long as logfile quick reopen isn't used (e.g., full configuration reload is used instead), and ensures logfile quick reopen works as currently implemented. While ownership change after USR1 might be a bit surprising for unfamiliar users, it is believed to be better than unconditional ownership change in any case. The need of execute bit on a log directory doesn't looks like a problem, too (note that read access isn't needed). If you are paranoid enough, you can use full configuration reload instead.

On the other hand, passing opened file descriptors from a master process is certainly better approach on platforms which support it, mostly because it needs less configuration. It is planned enhancement for a long time, let this ticket sit in trac as a reminder.

[MTecknology@…]

What would it take to get some attention drawn to this bug?

In Debian, we thought we fixed one broken hack to address a CVE [1], but years later it was brought to our attention that we'd inadvertently created a bigger concern [2]. Now, we've reverted back to the old behavior, in effect, re-opening the old CVE.

It appears this is not just a Debian problem. I've looked around at other distributions and haven't been able to find any that don't have some form of security concern.

I'll admit, most of the security concerns require at least one other vulnerability be exploited, but web application vulnerabilities aren't exactly uncommon.

[1] ​https://security-tracker.debian.org/tracker/CVE-2013-0337
[2] ​https://security-tracker.debian.org/tracker/CVE-2016-1247

[Jérôme Poulin]

To make matter worse, the log is always opened using O_CREAT, disallowing the use of AppArmor to restrict the log file opening to append only.

The workaround we're using is:

1. Make a AppArmor profile which removes chown capability from Nginx.
2. chown -R root:adm /var/log/nginx
3. chmod 0755 /var/log/nginx
4. chmod 0640 /var/log/nginx/*
5. Setup logrotate to create files as www-data:adm/0640.
6. Setup logrotate post-rotate to invoke-rc.d nginx rotate, then, chown root /var/log/nginx/*.log

/etc/apparmor.d/usr.sbin.nginx

#include <tunables/global>

/usr/sbin/nginx {
  #include <abstractions/base>
  #include <abstractions/nameservice>

  deny capability chown,

  capability dac_override,
  capability net_bind_service,
  capability setgid,
  capability setuid,


  /etc/nginx/** r,
  /etc/ssl/certs/** r,
  /etc/ssl/openssl.cnf r,
  /run/nginx.pid rw,
  /run/nginx.pid.oldbin rw,
  /usr/lib/nginx/modules/*.so mr,
  /usr/sbin/nginx mr,
  /usr/share/nginx/** r,
  /var/cache/nginx/ rw,
  /var/cache/nginx/** rw,
  /var/lib/nginx/ r,
  /var/lib/nginx/** rw,
  /var/log/nginx/*.log w,
  /var/log/nginx/*.log.1 w,
  /var/www/** r,

  #include <nginx.d>
  #include <local/usr.sbin.nginx>
}

/etc/logrotate.d/nginx

/var/log/nginx/*.log {
...
    create 0640 www-data adm
    postrotate
        invoke-rc.d nginx rotate >/dev/null 2>&1
        sleep 2
        chown root:adm /var/log/nginx/*.log
    endscript
}

EDIT: Updated procedure to prevent having to reload nginx.

[Beuc@…]

Hi,
I'm part of the Debian LTS Team.
What is the nginx maintainers' stance on this issue?

[Maxim Dounin]

This is going to be implemented at some point. This is relatively low priority since there isn't much difference with new log files properly pre-created during log rotation, and the only noticeable benefit is no need to pre-create log files during log rotation.

[Maxim Dounin]

Just for the record, Ruslan's attempt to implement this revealed multiple edge cases, notably:

• Obviously enough, this will work only on platforms which support passing file descriptors, so platforms without the support need to use the existing approach of reopening logs in the worker processes.

• Sending descriptors from master to worker processes might block, and this needs to be correctly handled without blocking the master process.

• File descriptors maintained by the old worker processes might be different from the ones currently maintained by the master process and new worker processes (for example, if a configuration was changed, and some logs were added or removed). At the same time, old worker processes might work for a long time, so not rotating logs in the old worker processes does not look like a viable solution. For proper handling this will require some infrastructure to keep old logs open in the master and tracking of the required logs on the per-worker basis.

This work was postponed for now.

[Maxim Dounin]

See also #1686, #1740, #2471.