457373 - dependency graphs created with incorrect permissions under suexec

Status: RESOLVED FIXED

(Bugzilla :: Dependency Views, defect)

Description

[Michael Ching]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.2) Gecko/2008091620 Firefox/3.0.2
Build Identifier: 

Dependency graphs in showdependencygraph.cgi default to 0700 permissions.  When running under suexec, this can result in the web server user being unable to read these files.

Reproducible: Always

Steps to Reproduce:
1. Standard install, suexec enabled
2. Configure with local "dot"
3. Attempt to view a dependency graph
Actual Results:  
Graph is broken image, viewing it shows it is a 403 forbidden.

Expected Results:  
png file should be readable by web server

We are using the following to work around the issue:

--- showdependencygraph.cgi.orig        2008-09-26 22:27:29.000000000 -0500
+++ showdependencygraph.cgi     2008-09-26 22:40:28.000000000 -0500
@@ -242,6 +242,7 @@
     print $pngfh $_ while <DOT>;
     close DOT;
     close $pngfh;
+    chmod 0644, $pngfilename;

     # On Windows $pngfilename will contain \ instead of /
     $pngfilename =~ s|\\|/|g if $^O eq 'MSWin32';
@@ -266,6 +267,7 @@
     print $mapfh $_ while <DOT>;
     close DOT;
     close $mapfh;
+    chmod 0644, $mapfilename;
     $vars->{'image_map'} = CreateImagemap($mapfilename);
 }

A better solution might chmod 0640 and chown to .webservergroup defined in localconfig.

Comment 1

[Bradley Baetz (:bbaetz)]

Yeah, but its more than that - the data and webdot dirs need a+x too so that .htaccess can be found. See bug 123165 comment 27....

Comment 2

[Bradley Baetz (:bbaetz)]

Attachment: Patch

I think this gets everywhere. I don't think that the +x affects anything security wise, since it only allows a local person to try for filenames that were already meant to be exposed remotely. Less logging, but we use File::Temp, so it should be OK

Comment 3

[Max Kanat-Alexander]

Comment on attachment 375751 [details] [diff] [review]
Patch

>+        my $perm = (stat $pngfilename)[2] & 07777;
>+        chmod ($perm | 0004, $pngfilename);

  Could you comment both of these bitwise operations, since they're kind of cryptic for people unfamiliar with them?

>Index: Bugzilla/Install/Filesystem.pm
>     # DIRECTORIES
>     # Readable by the web server.
>     my $ws_dir_readable  = ($ws_group && !$use_suexec) ? 0750 : 0755;
>+    # Readable by the web server, searchable by anyone
>+    my $ws_dir_readable_searchable = $use_suexec ? 0755 : $ws_dir_readable;

  That looks more like all_readable, not searchable. Also, under suexec, this is exactly identical to $ws_dir_readable, so why does it even exist?

>     my $ws_dir_writeable = $ws_group ? 0770 : 01777;
>-    # The web server can overwrite files owned by other users, 
>+    # Searchable by the web server.
>+    my $ws_dir_writeable_searchable = $use_suexec ? 0771 : $ws_dir_writeable;

  Why not just re-use the 01777 under suexec?

>@@ -134,7 +146,7 @@
>     # recurse through. These are directories we didn't create
>     # in checkesetup.pl.
>     my %non_recurse_dirs = (
>-        '.'  => $ws_dir_readable,
>+        '.'  => $ws_dir_readable_searchable,

  Wouldn't every directory need that? And wouldn't we need to be able to traverse the directory as well? (That is, why not just change the definition of ws_dir_readable?)

>+    # When using suexec, the .htaccess file is read as apache, not as the user
>+    # This means that any directory with a .htaccess file needs to be
>+    # world-executable (ie $ws_dir_*_searchable or better)

  I think it's safe to make every directory world-traversable in that case.

>Index: docs/en/xml/installation.xml
> [snip]
>+        the web server runs under.  You will also need to set the
>+        <emphasis>use_suexec</emphasis> parameter to 1.</para>

  Since we said "value" earlier, you might want to say "value" instead of "parameter".

Comment 4

[Bradley Baetz (:bbaetz)]

The way suexec works is that apache needs to be able to find any non-cgi files, but the cgi files only need to be readable by the user. We don't want to make too much have +x, else other people with local access to the server may be able to see stuff they shouldn't. (We could have $suexec_group, but that doesn't help for directories)

Comment 5

[Bradley Baetz (:bbaetz)]

Attachment: v2

small bugs fixed. I left the permissions more restrictive on purpose - think another local user on the box (which is where suexec tends to be used)

Comment 6

[Max Kanat-Alexander]

(In reply to comment #4)
> We don't want to make
> too much have +x, else other people with local access to the server may be able
> to see stuff they shouldn't. 

  Not unless you set +r on the directories for them, also. Also, we're already limiting access to the root bugzilla directory to just the user and the webserver, right?

Comment 7

[Max Kanat-Alexander]

Comment on attachment 377684 [details] [diff] [review]
v2

>Index: showdependencygraph.cgi
>+    if (Bugzilla->localconfig->{'use_suexec'}) {
>+        # We need to get the file mode (from stat)
>+        # then mask it off to only look at the permission bits
>+        # and then set the execute bit

  It doesn't look like you're setting the execute bit. And anyhow, why would we need to set the execute bit? And couldn't we just set umask appropriately instead?

>Index: Bugzilla/Install/Filesystem.pm

  As far as all this goes, I'd rather be a little less secure and have less code complexity. The problem isn't just the complexity of the code, but that it's too hard for future programmers to decide which permission set a directory needs. A lot of different people need to modify Filesystem.pm, and I don't want them all to have to have a PhD in suexec. I'm OK for now with just giving traversal to "other" on every directory, under suexec.

Comment 8

[Bradley Baetz (:bbaetz)]

(In reply to comment #7)
> (From update of attachment 377684 [details] [diff] [review])
> >Index: showdependencygraph.cgi
> >+    if (Bugzilla->localconfig->{'use_suexec'}) {
> >+        # We need to get the file mode (from stat)
> >+        # then mask it off to only look at the permission bits
> >+        # and then set the execute bit
> 
>   It doesn't look like you're setting the execute bit. And anyhow, why would we
> need to set the execute bit?

Err, yeah, that should be read bit.

> And couldn't we just set umask appropriately
> instead?

Not really - the umask is global and I don't want to have to play with it on every connection.

> 
> >Index: Bugzilla/Install/Filesystem.pm
> 
>   As far as all this goes, I'd rather be a little less secure and have less
> code complexity. The problem isn't just the complexity of the code, but that
> it's too hard for future programmers to decide which permission set a directory
> needs. A lot of different people need to modify Filesystem.pm, and I don't want
> them all to have to have a PhD in suexec. I'm OK for now with just giving
> traversal to "other" on every directory, under suexec.

The problem is that 'other' includes the webserver, which in some cases will give raw access over the web.

We do have .htaccess, but I'll have to double check that thats safe

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> Not really - the umask is global and I don't want to have to play with it on
> every connection.

  We have to play with it anyway. There are lots of situations in which the webserver creates files.

> The problem is that 'other' includes the webserver, which in some cases will
> give raw access over the web.
>
> We do have .htaccess, but I'll have to double check that thats safe

  Oh, hmm. Yeah, we should rely on .htaccess there.

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: v2.1

Fixed comment.

File::Temp passes in 0600 to sysopen, so the umask won't help - we have to explicitly chmod. I did change the chmod to add 0444 rather than just 0004, though.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 394715 [details] [diff] [review]
v2.1

I'm going to fix this in bug 561797.

Comment 12

[Max Kanat-Alexander]

So, the fix for this is involved enough that it's only going to make it to trunk, unfortunately. So dependency graphs just aren't going to work under suexec without manual intervention, on 3.6.

Comment 13

[Max Kanat-Alexander]

Okay, I'm going to take the major refactoring patch from bug 561797 and bring it over here once the security fix is done, and do the refactoring in this bug instead.

Comment 14

[Max Kanat-Alexander]

Attachment: Full Refactoring

Here's the full refactoring, from the other bug, that's now going to happen in this bug instead.

Comment 15

[Max Kanat-Alexander]

Attachment: Full Refactoring, v9

Okay, here's an updated version of the full patch. I'm granting myself review as module owner, but justdave also reviewed all these permissions in the blocker.

Comment 16

[Max Kanat-Alexander]

I checked this in, and apparently forgot to note it here in the bug.

revision 7249:

http://bzr.mozilla.org/bugzilla/trunk/revision/7249

Comment 17

[Max Kanat-Alexander]

Added to the release notes in bug 604256.