561797 - (CVE-2010-0180) [SECURITY] checksetup.pl with $use_suexec=1 sets localconfig as world readable

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Daniel Piddock]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv:1.9.1.9) Gecko/20100330 Fedora/3.5.9-2.fc12 Firefox/3.5.9
Build Identifier: 3.6

localconfig is set as world readable (0644) by checksetup.pl's "fixing permissions" section if $suexec is enabled.

Reproducible: Always

Steps to Reproduce:
1. Extract bugzilla 3.6
2. Run ./checksetup.pl to generate localconfig
3. Edit localconfig setting suexec=1 (and other necessary details)
4. Run ./checksetup.pl again
Actual Results:  
$ ls -l localconfig
-rw-r--r-- 1 bugzilla bugzilla 5638 2010-04-26 03:33 localconfig

Expected Results:  
$ ls -l localconfig
-rw-r----- 1 bugzilla bugzilla 5638 2010-04-26 03:33 localconfig

Although the webserver is stopped from serving localconfig via .htaccess (if $create_htaccess = 1) this will not stop other users with shell access gaining the database password.

Comment 1

[Dave Miller [:justdave]]

The only files that need to be readable by the webserver are the files that are directly served by the webserver (i.e. html, css, javascript, etc).  Files which are only data files for the CGI scripts only need to be readable by the CGI scripts (thus the bugzilla user in this case)

There appears not to be any mechanism for this situation in Bugzilla/Install/Filesystem.pm ...  localconfig must be group readable to the webservergroup when not using suexec or the webserver can't read it at all.  Sounds like we need an additional category to put files into which are considered non-served data files.

Comment 2

[Max Kanat-Alexander]

Okay. There are actually some other changes that need to be made to the set of permissions in Filesystem.pm, so I can do this.

Comment 3

[Max Kanat-Alexander]

Attachment: v1 (bundle)

Okay, I decided to overhaul the permissions system in general. This was the only reasonable way that I could get secure suexec permissions on all files and still get everything working. You'll notice that this patch contains a *lot* of changes--I don't think I can split them out and still get a secure system under both suexec and non-suexec.

Note that I don't care that much about the security of installations that don't specify a $webservergroup, so they will still be relatively insecure.

Here's a list of some important changes:

* All the permission types are now constants, so they can be accessed from
  outside of Filesystem.pm, and because they really should be constants anyway.

* I added a bunch of new permission types, to differentiate between files
  that the webserver serves, and files that cgi scripts read or write to.

* I added a constant DIR_ALSO_WS_SERVE, which can be OR'ed into any
  directory permission type to indicate that cgis write to that directory
  but then its contents are directly served to the user. I did this instead
  of adding a new constant, because I would have had to add two new constants
  that would have been hard to name, and probably confusing. This is pretty
  readable.

* Before this patch, the "web" directory in extensions probably wasn't working
  under suexec. This patch makes the "web" directory an official part of
  extensions and makes Filesystem.pm correctly set permissions on it. It also
  sets permissions on the rest of the extension much more tightly.

* The default permission of files in the Bugzilla root directory is now
  OWNER_READ instead of the former $ws_readable. This should be a more secure
  default--now we have to explicitly specify any file that we want the webserver
  to be able to read, in the root dir.

* I fixed the suexec problems with dependency graphs. Once those permissions
  were fixed, I noticed that files in data/webdot were world-readable, so
  I added some code to fix their permissions properly while
  showdependencygraph runs, and delete the png file briefly after the script
  finishes. (And always delete the .map and .dot files, because we don't need
  those once the cgi is done.)

* Old Charts are also world-readable under suexec, but I think those are
  basically public information anyway, so I'm not so concerned about it.
  Also, it would be much more complicated to delete them after a period
  of time, because they have fixed names (they aren't regenerated on
  every hit).

* I added permission-setting for the .bzr dir. I've made it OWNER_WRITEABLE.
  It's hard to say whether or not somebody would put anything secure in there,
  but it's always possible that their bzr history is private.

Comment 4

[Max Kanat-Alexander]

Attachment: v2 (bundle)

Oh, actually, I realized that I could simply make ALSO_WS_SERVE into 00001 and then people couldn't list the data/webdot directory, but the webserver could still serve files from it, because we always know the names of the files we're serving. So this eliminated the need for the unlinking in showdependencygraph.cgi (it's unlikely anybody's going to guess the names of our temp files).

Comment 5

[Dave Miller [:justdave]]

ok, at first glance most of this looks good to me.  I have a couple issues with the showdependencygraph stuff though.

In %create_dirs:

+        $webdotdir              => DIR_CGI_WRITE,

That should be | DIR_ALSO_WS_SERVE, right?

The "wait 5 seconds before exiting" thing sounds hoaky to me.  Some user-agents might wait for the connection to close downloading the initial page before they start loading images, which would make it moot.  Pretty sure the major browsers aren't among those, but still...  don't we already have a cleanup for this directory somewhere? (cleans stuff over a certain age?).  If this is a concern, we should make the time shorter on that instead of doing it here.

If we do leave the unlink here, the .map file is served to the user-agent, so you need to use $unlink_png on the UNLINK for that one, also, otherwise it won't be there when the user-agent comes back for it.  It also needs to have WS_SERVE permissions, regardless whether we unlink it or not when we're done.

Comment 6

[Dave Miller [:justdave]]

the above comment midaired with your second attachment, so anything you already fixed can be ignored. :)

Comment 7

[Max Kanat-Alexander]

Attachment: v3 (bundle)

Ah, thanks for catching all that. This one fixes all the stuff justdave just pointed out.

Comment 8

[Dave Miller [:justdave]]

(In reply to comment #5)
> In %create_dirs:
> 
> +        $webdotdir              => DIR_CGI_WRITE,
> 
> That should be | DIR_ALSO_WS_SERVE, right?

Same for 'graphs' immediately above it (sorry I didn't catch that one before)

Comment 9

[Frédéric Buclin]

So if I understand correctly, this leak is new in Bugzilla 3.6 and is due to bug 123165? In that case, I suppose this blocks 3.6.1?

Comment 10

[Frédéric Buclin]

Assuming bug 123165 is indeed the culprit, Bugzilla 3.5.1 and newer are affected.

Comment 11

[Dave Miller [:justdave]]

Comment on attachment 441703 [details] [diff] [review]
v3 (bundle)

per comment 8

Comment 12

[Max Kanat-Alexander]

Note that in most SUExec environments (shared hosting) other users can't get at your files anyway, so this is not extremely critical.

Comment 13

[Max Kanat-Alexander]

Attachment: v4 (bundle)

Okay, fixed the default permissions of "graphs".

Comment 14

[Reed Loden [:reed]]

This is CVE-2010-0180.

Comment 15

[Dave Miller [:justdave]]

Comment on attachment 441861 [details] [diff] [review]
v4 (bundle)

Looks good.

Comment 16

[Max Kanat-Alexander]

Okay, so we have somewhat of a dilemma here: this is kind of a huge refactoring that's somewhat hard to test. I've tested it fairly thoroughly, but when you mess with permissions, it's hard to say what's going to break on people's installs.

We have a few options:

1. Don't fix this in the 3.6 branch, and just warn people about it. This would still give 3.6 suexec support than 3.4 and earlier had.

2. Check this in on trunk only, let it "bake" for a while, and if things are OK, then backport it to a later 3.6 release.

3. Check this in to the 3.6 branch and risk breakage.

4. Do a backport that only fixes the localconfig issue. I think that this might be silly, because there are probably lots of other important files that should also be protected. But it's a possibility.

I'm most in favor of #1 or #4. 

LpSolit and/or justdave?

Comment 17

[Frédéric Buclin]

(In reply to comment #16)
> 4. Do a backport that only fixes the localconfig issue.
> I'm most in favor of #1 or #4. 

I'm in favor of #4 too. That's the single critical file which contains passwords and other sensitive data (I think data/params is already correctly protected, right?).

Comment 18

[Max Kanat-Alexander]

Attachment: 3.6 v1

Okay, here is a super-simple patch for 3.6.

Comment 19

[Frédéric Buclin]

Comment on attachment 445404 [details] [diff] [review]
3.6 v1

Looks good (untested). r=LpSolit

Comment 20

[Frédéric Buclin]

Comment on attachment 441861 [details] [diff] [review]
v4 (bundle)

OK, this patch breaks installations using the PROJECT environment variable. I have two localconfig files in the same bugzilla/ directory: the default one named localconfig, and the one used by PROJECT=foo named localconfig.foo. When running checksetup.pl, it sets permissions to 640 for the localconfig file corresponding to the PROJECT env variable, setting the other one to 400, making it unreadable by the web server, and so breaking all other installations.

Comment 21

[Frédéric Buclin]

I guess the backport for 3.6 doesn't have this problem?

Comment 22

[Max Kanat-Alexander]

Attachment: v5 (bundle)

Okay, I fixed the localconfig $PROJECT problem, and I also eliminated DIR_OWNER_READ, because it was preventing me from using "bzr up" (because I couldn't write to certain directories).

Comment 23

[Max Kanat-Alexander]

Attachment: v6 (bundle)

I also eliminated OWNER_READ--it was problematic on docs/bugzilla.ent, and I think we didn't really need it. Using OWNER_WRITE is fine.

Comment 24

[Max Kanat-Alexander]

Attachment: v7 (bundle)

Oops, accidentally included an unnecessary change to a contrib/ file.

Comment 25

[Max Kanat-Alexander]

Attachment: v8 (simple fix)

Okay, I've decided that I'm just going to give this security bug a simple fix, here, and then I will do the major refactoring in a separate bug.

This is basically the "3.6 fix" from above, but taking into account the PROJECT problem. It should apply to both trunk and the 3.6 branch.

Comment 26

[Tiago Mello [:timello]]

Comment on attachment 452951 [details] [diff] [review]
v8 (simple fix)

It looks ok. I didn't test it for multiple projects though, but it should ok as it works for a single project.

Comment 27

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Install/Filesystem.pm
Committed revision 7245.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Install/Filesystem.pm
Committed revision 7111.

Comment 28

[Max Kanat-Alexander]

Security advisory sent, unlocking bug.

Comment 29

[Reed Loden [:reed]]

(In reply to comment #3)
> * I fixed the suexec problems with dependency graphs. Once those permissions
>   were fixed, I noticed that files in data/webdot were world-readable, so
>   I added some code to fix their permissions properly while
>   showdependencygraph runs, and delete the png file briefly after the script
>   finishes. (And always delete the .map and .dot files, because we don't need
>   those once the cgi is done.)
...
> * I added permission-setting for the .bzr dir. I've made it OWNER_WRITEABLE.
>   It's hard to say whether or not somebody would put anything secure in there,
>   but it's always possible that their bzr history is private.

These two issues are now classified as CVE-2010-2470 and have been filed separately as bug 576060.

Comment 30

[Amos Shapira]

Does this bug affect 3.4.5 or can I conclude from the detailed list of affected versions that it doesn't?

Comment 31

[Frédéric Buclin]

(In reply to comment #30)
> Does this bug affect 3.4.5 or can I conclude from the detailed list of affected
> versions that it doesn't?

3.4.5 is not affected. The first affected version is 3.5.1.