Closed Bug 105366 Opened 19 years ago Closed 13 years ago

Provide suexec documentation

Categories

(Bugzilla :: Documentation, defect, P2)

2.14
x86
All
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: djm, Assigned: justdave)

References

Details

Attachments

(2 files, 5 obsolete files)

If you are using Bugzilla with Apache's suexec feature, checksetup.pl will break
bugzilla by resetting ownership of files to root (if checksetup.pl is run as
root). This can be rectified by adding a webserveruser parameter to localconfig.
Keywords: patch, review
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.16
Comment on attachment 54018 [details] [diff] [review]
Patch to add webserveruser to checksetup.pl and localconfig

small nit here, but since the primary benefit is being able to use suexec in
Apache, it probably wouldn't hurt to say so in the comment that goes into 
localconfig.

Otherwise, this looks really good.
Attached is a revised patch. This one mentions webserveruser's applicability to
Apache suexec. 

It also gives others read permissions to the html files. This is needed because
Apache suexec only resets the uid when executing CGI programs and the current
checksetup.pl will clear the other read bit on the html files - making them
unreadable by the webserver. I hope this is the best place for this, otherwise I
can open another bug.

I can't forsee any security issues with making the html files word readable [if
you didn't want the world to read them, then why did you put them on a web
server? :) ]
Comment on attachment 54151 [details] [diff] [review]
Revised patch: mentions suexec in comment, sets o+r on *.html

>+# This is the user your web server runs on. Settings this (along with 
>+# webservergroup) is useful if you are running Bugzilla under Apache's 
>+# suexec mechanism.

s/Settings/Setting/  ?

>+    fixPerms('*.html',033);

>+    fixPerms('*.html',033);

What's the reasoning on these?  They should already be correct based on 
whether $webservergroup was set.  (That's the chmod on glob("*"))
Attachment #54151 - Flags: review-
> What's the reasoning on these?  They should already be correct based on 
> whether $webservergroup was set.  (That's the chmod on glob("*"))

The normal checksetup.pl fixPerms('*',027) removes the world-readable bit on the
html and image files. Since Apache only invokes suexec when running CGIs, this
makes the html files, etc inaccessible to the webserver. If you haven't set
webservergroup, this change is bypassed.

I'll add a new patch which fixes the typo and clears up the perms a bit better.
I do wonder whether it would be better to leave the world-readable bit on all
the time.
Attached patch Revised patch (obsolete) — Splinter Review
*** Bug 87780 has been marked as a duplicate of this bug. ***
I don't feel good about reviewing the chmod portion of this patch right now, 
dave: can you review?
I am pretty ambivalent about the chmod potions. Why are different permissions
used when webservergroup is specified at all? 

Why not use the same permissions for both cases? This would solve the suexec
problem and simplify the code.
Comment on attachment 54450 [details] [diff] [review]
Revised patch

>+# If you set this to anything besides "", you will need to run checksetup.pl
>+# as root.

Or as the named user.  In fact, you could run it as that user without this 
patch and it would be set up correctly since it chowns everything to the 
current user without this patch.

>+    chmod 0644, glob('*.html');
>+    chmod 0644, glob('*.gif');
>+    chmod 0644, glob('*.jpg');

I don't want these files world-readable if there's a webservergroup.

Now that I've had a chance to sit down and look at the security implications 
here, I'm hesitant about leaving things owner-writable with suexec 
enabled...  seems to me like that's just asking for trouble.

>I am pretty ambivalent about the chmod potions. Why are different 
>permissions used when webservergroup is specified at all? 

>Why not use the same permissions for both cases? This would solve the 
>suexec problem and simplify the code.
And open up security holes, too.  The idea is that we set the permissions as 
restrictive as we can get away with for the environment that the script is 
running in.  This is the whole reason for knowing the webservergroup to 
begin with, because then we can make the files only readable to the web 
server, and not readable to every joe schmoe that has an account on the 
machine.You also don't want the web server to be able to write to any files that it 
doesn't absolutely have to write to, so the files should not be owned by 
the user Bugzilla is running as.

To that end, that means that, in an suexec environment, you would want only
files that are directly executed by the web server to be owned by the 
web server user.  Those files should also have the owner-writable bit turned
off.  All other files should be owned by someone else, and only readable to 
the group the running user belongs to.
Attachment #54450 - Flags: review-
> Or as the named user.  In fact, you could run it as that user without this 
> patch and it would be set up correctly since it chowns everything to the 
> current user without this patch.

This doesn't work. checksetup.pl currently does this:

    # get current gid from $( list
    my $gid = (split " ", $()[0];
    chown $chown_uid, $gid, glob('*');

Which breaks the group ownership of files if you you are using a supplemental
group (e.g. to get around Apache suexec minimum gid restrictions)

Why not just ditch all this in favor of an explicit umask or two (one for the
CGIs, another for everything else)?

> This doesn't work.

Did you actually try it?  I just did, just to make sure I wasn't losing my
sanity and it worked fine for me.

That section of code you're quoting is actually as follows:
    # get current gid from $( list
    my $gid = (split " ", $()[0];
    chown $<, $gid, glob('*');
    fixPerms('*',022);

This snippet of code only runs if you DON'T specify a webservergroup.

you asked "why not just use a umask?"  That's what that 022 is that's being
passed to fixPerms.  You'll notice in the section that runs if you do set a
webservergroup, it passes an 027 for the mask (then does some cleanup on things
that need to be less restrictive)

I won't argue that the file permissions setting needs a lot of help right now,
I'm just arguing that explicitly setting the .html, .js, and .gif files isn't
necessary because it's already done in the fixPerms() routine.
btw, it chowns to the current user whether or not you use webservergroup, so if
you need a specific group, set that group in $webservergroup, then run
checksetup.pl as the user you need it set to.
and another btw...   I'm not arguing against fixing checksetup.pl so it does all
this for you even if you run it as root.  I think it's a good idea.  I was only
suggesting that a lot more detail needs to go into what gets set where when
you're running it that way.  There's a bug somewhere on letting you specify the
umask to be used, too.  That whole section pretty much needs to be redone anyway.
I'm agreeing with Dave.  I currently run Bugzilla under mod_suexec, and 
realized from his comments that it can be a huge security hole since all files 
are owned by the suexec user.  Ack!  Better go fix my install.

The safest approach for this seems to be to specify a User and Group directive 
in the VirtualHost directive for bugzilla.  It may, perhaps, be better to have 
this be entirely a doc fix, and not patch checksetup.pl at all.  Here's the 
relevant code snippets for an apache config file to make this work in a secure 
fashion:

##
## Bugzilla
##
 
<VirtualHost 127.0.0.1>
ServerName bugzilla.domain.com
ServerAlias bugzilla
DocumentRoot (insert Bugzilla directory here)
 
<Directory (insert Bugzilla directory here)>
  Options +FollowSymLinks +Indexes +Includes +ExecCGI
  AllowOverride All
  User (insert Bugzilla user here)
  Group (insert Bugzilla group here)
</Directory>

##
## End Bugzilla
##
  The key to making this strategy work for suExec would be that checksetup.pl 
still leaves the file owned by root (or an unprivileged user), NOT owned by 
the suExec user.  This would preclue overwriting of the application files, 
which could really, really suck, but still allow group-writability of those 
files where required.

  I haven't looked at the patch attached, but are the VirtualHost and 
Directory directives above sufficient to do the job without further 
modification of checksetup.pl?  Note I'm not against modifying the 
checksetup.pl file, but I am in favor of having stronger security whenever 
possible.  I screwed up my own install, and am going to go fix it today in the 
manner suggested above.

  If the above fixes this issue, reassign to me and I'll bundle the doc fix in 
for 2.16.
We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
I don't see why suexec is insecure - if the webserver runs as you, then even if
you don't have write access to the file, if someone finds an exploit they can
just chmod the file first. What am I missing?
Is 'webserveruser' the correct name here?  I generally don't want these files to
be owned by the web server user, I want them to be owned by me.  The web server
group is instead used to let the web server access the files.
mattyt: it needs to be the user the webserver runs as, which may or may not be
the user editing localconfig.
Attachment #54018 - Attachment is obsolete: true
Why would I need it to be the web server user?  I have never needed this and
would much prefer it be set to me so if I run checksetup as root it makes sure I
can edit the files in my user account.
*** Bug 143110 has been marked as a duplicate of this bug. ***
To keep from messing up suexec installations, ant.jpg, index.html, and the CSS
files need to be world-readable.  Then, on those installations, the
webserveruser is simply the login account user and everything should be happy.
ok, moving this to documentation....

after re-reading this bug, I think it's more secure to run Bugzilla *without*
suexec than with it, if you have control over the server.  The only time anyone
should ever want to install Bugzilla under suexec is if they don't have control
over the server, and the server admins are forcing them to use suexec in order
to run CGIs at all.  And in those conditions, there's tons of other things to
worry about as well, so I think this is best handled as "here's some things you
need to be aware of if you want to run it this way" in the documentation.
Assignee: zach → documentation
Component: Installation & Upgrading → Documentation
Summary: webserveruser in localconfig, checksetup.pl → Provide suexec documentation
OK, just went through trying to set Bugzilla up under suexec with a user in IRC...

here's what we wound up doing to get it to work in Bugzilla 2.18rc3:

1) $webservergroup in localconfig is set to the suexec group for the user who
owns the webspace.

2) checksetup.pl is run as that user.

3) The following additional chmods were made:

[Sat 16:56:03] <justdave> chmod 644 .htaccess
[Sat 16:56:06] <justdave> chmod 644 */.htaccess
[Sat 16:56:09] <justdave> chmod 644 */*/.htaccess
[Sat 16:56:17] <justdave> chmod 644 *.html
[Sat 16:56:21] <justdave> chmod 644 *.js
[Sat 16:56:26] <justdave> chmod 644 *.gif
[Sat 16:56:28] <justdave> chmod 644 *.jpg
[Sat 16:56:46] <justdave> chmod 755 css
[Sat 16:56:48] <justdave> chmod 755 js
[Sat 16:56:57] <justdave> chmod 644 css/*
[Sat 16:57:02] <justdave> chmod 644 js/*
[Sat 16:58:06] <justdave> chmod 755 skins
[Sat 16:58:15] <justdave> chmod 755 skins/standard
[Sat 16:58:27] <justdave> chmod 644 skins/standard/*
[Sat 16:59:01] <justdave> I *think* that's it...

That pretty much got it working.
see also bug 123165 (not quite a dupe)
QA Contact: mattyt-bugzilla → default-qa
Severity: enhancement → normal
Flags: blocking3.0+
Attachment #259299 - Attachment mime type: application/octet-stream → text/plain
Flags: blocking3.0.1+
Flags: blocking3.0-
Flags: blocking3.0+
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
I recently struggled setting Bugzilla's permissions appropriately for suEXEC. I don't want all files world readabile or writeable, but my user id is not a member of the webserver group. My CGIs are all run as my user id using suEXEC, so I set $webservergroup to my primary group in localconfig. This correctly set the owner and group on all Bugzilla files, though in this case $webservergroup actually reffers to the user id Bugzilla is run as, vs. the user id Apache is run as. It also set the permissions on all Bugzilla files so they're readable or writable only by my user id (the user id Bugzilla is run as), which is also as I desired. Unfortunately it does mean the user id Bugzilla is run as has permission to edit the Bugzilla scripts, but I don't think there's any way to avoid this in my environment (I have only one user id with which I must both install Bugzilla and run Bugzilla). It also means the only thing keeping checksetup.pl and runtests.pl from being executed from the web is the .htaccess file.

The trouble is it also set the permissions on all Bugzilla static content so it's readable by only my user id. Consequently the webserver can't retrieve .htaccess or any of the CSS or images. I have to manually correct this every time I run checksetup.pl. I wish Bugzilla would leave static content world readable.

I haven't tried bmo2007's script yet.
Assignee: documentation → justdave
At our Bugzilla meeting today, we decided we wouldn't block the release on it, so this bug is not a blocker.
Flags: blocking3.0.1+ → blocking3.0.1-
Attached patch Docs patch v1 (obsolete) — Splinter Review
Here's a draft at the actual docs update for this. (applies to tip)
Attachment #54151 - Attachment is obsolete: true
Attachment #54450 - Attachment is obsolete: true
Attachment #284896 - Flags: review?(mkanat)
Comment on attachment 284896 [details] [diff] [review]
Docs patch v1

This looks good, but Colin should also take a look at it for docbook purposes and so forth.
Attachment #284896 - Flags: review?(mkanat)
Attachment #284896 - Flags: review?(colin.ogilvie)
Attachment #284896 - Flags: review+
Attached patch Docs patch v2 (obsolete) — Splinter Review
left out the data/webdot directory, which needs to be accessible to the webserver, too.
Attachment #284896 - Attachment is obsolete: true
Attachment #284900 - Flags: review?(mkanat)
Attachment #284896 - Flags: review?(colin.ogilvie)
This patch applies cleanly on all four maintained branches, fwiw.
Attached patch Docs patch v3Splinter Review
Good catch by mkanat... bugzilla directory itself needs to be readable by the webserver, too.
Attachment #284900 - Attachment is obsolete: true
Attachment #284901 - Flags: review?(mkanat)
Attachment #284900 - Flags: review?(mkanat)
Comment on attachment 284901 [details] [diff] [review]
Docs patch v3

Looks good. :-) Colin should sign off on the XML.
Attachment #284901 - Flags: review?(mkanat)
Attachment #284901 - Flags: review?(colin.ogilvie)
Attachment #284901 - Flags: review+
Comment on attachment 284901 [details] [diff] [review]
Docs patch v3

r=me from an XML perspective.

Bonus points if you fix the typo that's at the top of the patch too ;)

'every time it in run' 

which isn't part of the change, but is annoying me now I spotted it.
Attachment #284901 - Flags: review?(colin.ogilvie) → review+
Flags: approval?
Flags: approval3.0?
Flags: approval2.22?
Flags: approval2.20?
11:13:23 <@LpSolit> justdave: a+ for all branches

(technically I could do this myself but since it's my patch I figure I'd get a 2nd opinion ;)
Flags: approval?
Flags: approval3.0?
Flags: approval3.0+
Flags: approval2.22?
Flags: approval2.22+
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
(In reply to comment #37)
> (technically I could do this myself but since it's my patch I figure I'd get a
> 2nd opinion ;)

It's also documentation, so you don't need it anyway ;)

trunk:
Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.146; previous revision: 1.145
done

3.0:
Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.136.2.6; previous revision: 1.136.2.5
done

2.22:
Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.107.2.20; previous revision: 1.107.2.19
done

2.20:
Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.98.2.25; previous revision: 1.98.2.24
done

I've filed bug 399972 for having checksetup.pl just deal with this to begin with.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.