Closed Bug 457373 Opened 14 years ago Closed 12 years ago

dependency graphs created with incorrect permissions under suexec

Categories

(Bugzilla :: Dependency Views, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: michaelc, Assigned: mkanat)

References

Details

(Whiteboard: [relnote extension web dir & stricter base perms])

Attachments

(1 file, 4 obsolete files)

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.
Depends on: 123165
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....
Assignee: dependency.views → bbaetz
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 3.6
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #375751 - Flags: review?(mkanat)
Attachment #375751 - Flags: review?(mkanat) → review-
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".
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)
Attached patch v2 (obsolete) — Splinter Review
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)
Attachment #375751 - Attachment is obsolete: true
Attachment #377684 - Flags: review?(mkanat)
Blocks: 491417
Whiteboard: [es-gnome]
(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 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.
Attachment #377684 - Flags: review?(mkanat) → review-
(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
(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.
Attached patch v2.1 (obsolete) — Splinter Review
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.
Attachment #377684 - Attachment is obsolete: true
Attachment #394715 - Flags: review?(mkanat)
Comment on attachment 394715 [details] [diff] [review]
v2.1

I'm going to fix this in bug 561797.
Attachment #394715 - Flags: review?(mkanat)
Depends on: CVE-2010-0180
Whiteboard: [es-gnome] → [blocker will fix]
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.
Assignee: bbaetz → mkanat
Hardware: x86 → All
Target Milestone: Bugzilla 3.6 → Bugzilla 3.8
Version: unspecified → 3.6
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.
Whiteboard: [blocker will fix]
No longer blocks: CVE-2010-3764
Attached patch Full Refactoring (obsolete) — Splinter Review
Here's the full refactoring, from the other bug, that's now going to happen in this bug instead.
Attachment #394715 - Attachment is obsolete: true
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.
Attachment #453891 - Attachment is obsolete: true
Attachment #453926 - Flags: review+
Flags: approval+
Keywords: relnote
Whiteboard: [relnote extension web dir & stricter base perms]
I checked this in, and apparently forgot to note it here in the bug.

revision 7249:

http://bzr.mozilla.org/bugzilla/trunk/revision/7249
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Added to the release notes in bug 604256.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.