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.

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

Don't call chown() unless necessary. #621

Merged
merged 1 commit into from Feb 18, 2014

Conversation

Projects
None yet
2 participants
@jart
Copy link
Contributor

jart commented Feb 16, 2014

Right now it's impossible to AppArmor NGINX when this module is enabled because you've got a mandatory chown() call. Worse yet, the nginx process exits if the chown() call fails. This means that you have to add capability chown to your NGINX AppArmor config. But granting chown to the application would undermine the security of the AppArmor sandbox. Hence this patch.

Here's a screenshot of NGINX not starting due to this bug:

screen shot 2014-02-15 at 9 39 20 pm

If you're not familiar with AppArmor, this config file /etc/apparmor.d/usr.local.nginx.sbin.nginx should hopefully help you understand it better.

#include <tunables/global>

/usr/local/nginx/sbin/nginx {
  #include <abstractions/base>
  #include <abstractions/nameservice>
  #include <abstractions/openssl>
  #include <abstractions/user-tmp>

  capability dac_override,
  capability setgid,
  capability setuid,
  capability mknod,

  /etc/nginx/** r,
  owner /etc/ssl/** r,
  /home/nginx/** r,
  /home/ows/ows/occupywallst/occupywallst/media/** r,
  /home/recordings/** r,
  /home/soundboards/**.mp3 r,
  /home/soundboards/**.ogg r,
  /opt/celebcall/static/** r,
  /run/nginx.pid rw,
  /usr/local/nginx/conf/** r,
  /usr/local/nginx/html/** r,
  /usr/local/nginx/logs/** rw,
  owner /usr/local/nginx/proxy_temp/** rw,
  /usr/local/nginx/sbin/nginx mr,
  /var/cache/nginx/** rw,
  /var/cache/pagespeed/** rwlk,
  /var/log/pagespeed/** rw,
  /var/log/nginx/* w,
  /var/run/nginx.pid rw,
}
@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Feb 18, 2014

Makes sense: don't call chown if we already own it.

LGTM.

Could you sign our CLA? https://developers.google.com/open-source/cla/individual

@jart

This comment has been minimized.

Copy link
Contributor

jart commented Feb 18, 2014

I'm a Googler. Is it necessary?

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Feb 18, 2014

Verified that jart works for Google; merging.

jeffkaufman added a commit that referenced this pull request Feb 18, 2014

Merge pull request #621 from jart/dont-chown
Security Fix: Don't call chown() unless necessary.

@jeffkaufman jeffkaufman merged commit bf6c6c0 into apache:master Feb 18, 2014

@jart jart deleted the jart:dont-chown branch Feb 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment