On 03.01.2011 17:48, Gena Makhomed wrote:

> On 03.01.2011 16:05, Piotr Karbowski wrote:

>>> master process running as root open/write files in /var/log/nginx
>>> - if nginx user have write permissions to this directory,
>>> 700 nginx:nginx - such setup is vulnerable by symlink attack

>>> better approach set permissions 750 root:nginx /var/log/nginx

>>> or 750 root:www-logs /var/log/nginx and add user nginx to group www-logs

>> Now when you mention it, if nginx worker have read perms there (as you
>> suggested above), then if user symlink to any log, he will be able fetch
>> it via nginx which is security hole.

[...]

> if nginx log files will have permissons nginx:root 244
> in this case nginx worker processes can only write(append)
> to log files, and can't read anything from it, even via symlink -
> it this case 403 Forbidden will be returned to access via symlink.

> but nginx source need to be patched for 244 logfile permissions,
> and this is can't be done via logrotate create 0244 nginx root
> directive, because nginx master process after USR1 signal
> explicitly reset logfiles permissioons to S_IRUSR|S_IWUSR

patches:
tested with 0.8.53, 0.8.54 and 0.9.3.

nginx-logfiles-permissions-1.patch:
reset log files owner (nginx) permissions to S_IWUSR only.

nginx-logfiles-permissions-2.patch:
first change log file permissions, thereafter change owner.

--
Best regards,
Gena
--- src/core/ngx_cycle.c.orig 2011-01-03 18:41:26.000000000 +0200
+++ src/core/ngx_cycle.c 2011-01-03 18:46:47.000000000 +0200
@@ -1201,9 +1201,9 @@
}
}

- if ((fi.st_mode & (S_IRUSR|S_IWUSR)) != (S_IRUSR|S_IWUSR)) {
+ if ((fi.st_mode & S_IRWXU) != S_IWUSR) {

- fi.st_mode |= (S_IRUSR|S_IWUSR);
+ fi.st_mode = (fi.st_mode & ~S_IRWXU) | S_IWUSR;

if (chmod((const char *) file[i].name.data, fi.st_mode) == -1) {
ngx_log_error(NGX_LOG_EMERG, cycle->log, ngx_errno,
--- src/core/ngx_cycle.c.orig 2011-01-03 19:24:20.000000000 +0200
+++ src/core/ngx_cycle.c 2011-01-03 19:27:50.000000000 +0200
@@ -1187,11 +1187,13 @@
}
}

- if (fi.st_uid != user) {
- if (chown((const char *) file[i].name.data, user, -1) == -1) {
+ if ((fi.st_mode & S_IRWXU) != S_IWUSR) {
+
+ fi.st_mode = (fi.st_mode & ~S_IRWXU) | S_IWUSR;
+
+ if (chmod((const char *) file[i].name.data, fi.st_mode) == -1) {
ngx_log_error(NGX_LOG_EMERG, cycle->log, ngx_errno,
- "chown(\"%s\", %d) failed",
- file[i].name.data, user);
+ "chmod() \"%s\" failed", file[i].name.data);

if (ngx_close_file(fd) == NGX_FILE_ERROR) {
ngx_log_error(NGX_LOG_EMERG, cycle->log, ngx_errno,
@@ -1201,13 +1203,11 @@
}
}

- if ((fi.st_mode & S_IRWXU) != S_IWUSR) {
-
- fi.st_mode = (fi.st_mode & ~S_IRWXU) | S_IWUSR;
-
- if (chmod((const char *) file[i].name.data, fi.st_mode) == -1) {
+ if (fi.st_uid != user) {
+ if (chown((const char *) file[i].name.data, user, -1) == -1) {
ngx_log_error(NGX_LOG_EMERG, cycle->log, ngx_errno,
- "chmod() \"%s\" failed", file[i].name.data);
+ "chown(\"%s\", %d) failed",
+ file[i].name.data, user);

if (ngx_close_file(fd) == NGX_FILE_ERROR) {
ngx_log_error(NGX_LOG_EMERG, cycle->log, ngx_errno,
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://nginx.org/mailman/listinfo/nginx-devel