Closed Bug 561797 (CVE-2010-0180) Opened 12 years ago Closed 12 years ago
.pl with $use _suexec=1 sets localconfig as world readable
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv:18.104.22.168) 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Target Milestone: --- → Bugzilla 3.6
Hardware: x86 → All
Summary: checksetup.pl with $suexec=1 sets localconfig as world readable → checksetup.pl with $suexec=1 sets localconfig as world readable (suexec permissions have several problems)
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.
Assignee: installation → mkanat
Status: NEW → ASSIGNED
Attachment #441701 - Flags: review?(LpSolit)
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).
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.
the above comment midaired with your second attachment, so anything you already fixed can be ignored. :)
Ah, thanks for catching all that. This one fixes all the stuff justdave just pointed out.
(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)
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?
Whiteboard: [doesn't affect 3.4.x or older]
Assuming bug 123165 is indeed the culprit, Bugzilla 3.5.1 and newer are affected.
Summary: checksetup.pl with $suexec=1 sets localconfig as world readable (suexec permissions have several problems) → [SECURITY] checksetup.pl with $suexec=1 sets localconfig as world readable (suexec permissions have several problems)
Version: unspecified → 3.5.1
Comment on attachment 441703 [details] [diff] [review] v3 (bundle) per comment 8
Attachment #441703 - Flags: review?(LpSolit) → review-
Note that in most SUExec environments (shared hosting) other users can't get at your files anyway, so this is not extremely critical.
Severity: normal → minor
Depends on: 123165
Flags: blocking3.6.1? → blocking3.6.1+
Okay, fixed the default permissions of "graphs".
This is CVE-2010-0180.
Comment on attachment 441861 [details] [diff] [review] v4 (bundle) Looks good.
Attachment #441861 - Flags: review?(justdave) → review+
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?
(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?).
Okay, here is a super-simple patch for 3.6.
Attachment #445404 - Flags: review?(LpSolit)
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.
Attachment #441861 - Flags: review-
I guess the backport for 3.6 doesn't have this problem?
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).
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.
Oops, accidentally included an unnecessary change to a contrib/ file.
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.
Summary: [SECURITY] checksetup.pl with $suexec=1 sets localconfig as world readable (suexec permissions have several problems) → [SECURITY] checksetup.pl with $suexec=1 sets localconfig as world readable
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.
Attachment #452951 - Flags: review?(timello) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: [SECURITY] checksetup.pl with $suexec=1 sets localconfig as world readable → [SECURITY] checksetup.pl with $use_suexec=1 sets localconfig as world readable
Security advisory sent, unlocking bug.
(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.
Does this bug affect 3.4.5 or can I conclude from the detailed list of affected versions that it doesn't?
(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.
You need to log in before you can comment on or make changes to this bug.