Last Comment Bug 314871 - (CVE-2009-3989) [SECURITY] Web browsers can see CVS/, contrib/, docs/, and t/ directory files
(CVE-2009-3989)
: [SECURITY] Web browsers can see CVS/, contrib/, docs/, and t/ directory files
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: 2.21
: All All
: -- trivial (vote)
: Bugzilla 3.0
Assigned To: Max Kanat-Alexander
: default-qa
:
Mentors:
Depends on:
Blocks: 532517
  Show dependency treegraph
 
Reported: 2005-11-02 19:35 PST by Joel Peshkin
Modified: 2010-02-03 10:10 PST (History)
7 users (show)
LpSolit: approval+
LpSolit: blocking3.6+
LpSolit: approval3.4+
LpSolit: blocking3.4.5+
LpSolit: approval3.2+
LpSolit: blocking3.2.6+
LpSolit: approval3.0+
mkanat: blocking3.0.11+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix permission and access right code in checksetup, V1 (4.84 KB, patch)
2005-12-09 16:24 PST, Teemu Mannermaa (:wicked)
LpSolit: review+
Details | Diff | Splinter Review
Fix permission and access right code in checksetup, V1 - v2.20 branch (5.61 KB, patch)
2006-01-11 03:44 PST, Teemu Mannermaa (:wicked)
no flags Details | Diff | Splinter Review
v3 - contrib/ (1.27 KB, patch)
2009-08-18 22:09 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
v4 (4.77 KB, patch)
2009-08-18 23:00 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
patch for 3.4.4 and 3.2.5, v5 (5.68 KB, patch)
2009-11-08 16:55 PST, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Splinter Review
v5 (3.6) (5.68 KB, patch)
2010-01-31 09:25 PST, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Splinter Review
v5 (3.0) (5.40 KB, patch)
2010-01-31 09:31 PST, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Splinter Review

Description Joel Peshkin 2005-11-02 19:35:08 PST
This is actually a trivial security problem.  We should block access to the CVS/Root, CVS/Repository, and CVS/Entries files.  

It is almost silly to mark this as a security bug, but it should be very easy to take care of.
Comment 1 Zach Lipton [:zach] 2005-11-02 19:43:02 PST
I agree that we should fix this, but I'm not sure what the security implications are. After all, the software is open source; anyone can find out the filenames we use and the location of the CVS respoitory. 
Comment 2 A. Karl Kornel 2005-11-02 19:57:42 PST
(In reply to comment #1)
> I agree that we should fix this, but I'm not sure what the security
> implications are.

I can think of one, although it's not much.  The Entries file contains the revision numbers for every file in the directory.  If you are somehow blocked from determining which version of Bugzilla is in use, you could use this to find out.
Comment 3 Joel Peshkin 2005-11-02 21:08:22 PST
When I set it as a trivial security bug, I wasn't kidding :-)
Comment 4 Frédéric Buclin 2005-11-03 06:44:39 PST
Note that you can also display the content of contrib/ and skins/
Comment 5 Teemu Mannermaa (:wicked) 2005-11-20 20:02:43 PST
(In reply to comment #2)
> I can think of one, although it's not much.  The Entries file contains the
> revision numbers for every file in the directory.  If you are somehow blocked

This could also be used to find out if there's a revision of some file in use at a site that has a known security hole. Although, why not just try to exploit the hole directly without checking file revisions first..

(In reply to comment #1)
> implications are. After all, the software is open source; anyone can find out
> the filenames we use and the location of the CVS respoitory. 

What about if somebody checks their copy from their local repository? I don't think that even if this is the case a CVS repository password won't be revealed.
Comment 6 Teemu Mannermaa (:wicked) 2005-12-09 16:24:35 PST
Created attachment 205438 [details] [diff] [review]
Fix permission and access right code in checksetup, V1

Here's my first attempt to protect CVS directories. This patch modifies checeksetup.pl to:
 1) Add a deny rule for Root, Repository and Entries files to the generated top level .htaccess file (stops also subdirectories).
 2) Generate deny all .htaccess file also to contrib, t and docs subdirectories (and simplifies same generating code for $attachdir, Bugzilla and $templatedir subdirectories).
 3) Always deny Apache's access to any and all CVS directories that are encountered in the fixPerms() sub. Previously non-directory recursing calls to that sub ignored CVS. This left top level CVS directory accessible.
 4) Change owners and permissions of contrib, t and docs subdirectories just like any other (note that new .htaccess they have).

Note that this diff has been done over the approved patch in bug 319241, which I assume is committed soonish.
Comment 7 Teemu Mannermaa (:wicked) 2005-12-26 12:37:09 PST
As a security bug, I think this needs to be fixed on all branches. Skins subdirectory isn't affected because I think everything should be retrievable from there anyhow. But t, contrib and docs subdirectories are affected. Especially contrib is frightening because it can contain some nasty scripts..
Comment 8 Max Kanat-Alexander 2006-01-05 13:25:21 PST
I don't see any actual security implications for the 2.18 or 2.16 branches -- finding out what version of Bugzilla is in use is not a security issue, particularly.

However, 2.20 and 2.22 have bzdbcopy.pl in the contrib/ directory, which can contain a DB password.
Comment 9 Frédéric Buclin 2006-01-08 09:27:27 PST
Comment on attachment 205438 [details] [diff] [review]
Fix permission and access right code in checksetup, V1

>+  # Generate .htaccess files in subdirectories to deny access to all files
>+  foreach my $dir ("$attachdir", "Bugzilla", "$templatedir",
>+                   "contrib", "t", "docs") {

Nit: I think we shouldn't prevent the access to docs/ from the web. If I were customising my installation, I would probably point my customers to my local docs/ instead of bugzilla.org. My vote to fix this on checkin.


>@@ -1396,6 +1387,9 @@
>         fixPerms('css', $<, $webservergid, 027, 1);
>         fixPerms('skins', $<, $webservergid, 027, 1);
>         fixPerms('js', $<, $webservergid, 027, 1);
>+        fixPerms('t', $<, $webservergid, 027, 1);
>+        fixPerms('contrib', $<, $webservergid, 027, 1);
>+        fixPerms('docs', $<, $webservergid, 027, 1);
>         chmod 0644, 'globals.pl';

Could someone explain me why globals.pl is world readable?? (question unrelated to this patch)


Tested on tip (doesn't apply cleanly on 2.20); looks good. And it's a nice cleanup too. r=LpSolit
Comment 10 Teemu Mannermaa (:wicked) 2006-01-11 03:44:17 PST
Created attachment 208194 [details] [diff] [review]
Fix permission and access right code in checksetup, V1 - v2.20 branch

Backport to 2.20 branch with two changes. Firstly fixes adding localconfig.js and localconfig.rdf allow clauses (won't be duplicated anymore) and secondly access to docs subdirectory is no longer denied.
Comment 11 Max Kanat-Alexander 2006-01-11 17:33:09 PST
Comment on attachment 205438 [details] [diff] [review]
Fix permission and access right code in checksetup, V1

I hate to r- for this, but this isn't a blocker of our next release... I really think that we need to do that "removable block" thing for the .htaccess.

That is, we're finding that we need to modify the .htaccess from time to time, and I think that we just need to create a block inside some comments that says "do not remove or modify anything in this block."

In order to create that block, what we can do is check whether or not the .htaccess has that block in it. If it doesn't have the block, move the old .htaccess out of the way and replace it with a valid one. Make sure that you print a very obvious warning to the sysadmin that you've done this.

From then on out, we can easily modify the .htaccess.
Comment 12 Frédéric Buclin 2006-01-11 17:38:16 PST
I don't see how this would simplify the problem. If an admin edit the "do not touch" section anyway, checksetup.pl would be unable to fix it.
Comment 13 Max Kanat-Alexander 2006-01-11 17:57:04 PST
(In reply to comment #12)
> I don't see how this would simplify the problem. If an admin edit the "do not
> touch" section anyway, checksetup.pl would be unable to fix it.

  checksetup should replace the "do not touch" section every time it runs.
Comment 14 Zach Lipton [:zach] 2006-01-11 17:59:10 PST
(In reply to comment #13)
>   checksetup should replace the "do not touch" section every time it runs.
> 

And if the admin, being a human being that is smarter than our perl script, happens to want to customize the "do not touch" section somehow, they would find this feature useful how?
Comment 15 Max Kanat-Alexander 2006-01-11 18:07:17 PST
(In reply to comment #14)
> And if the admin, being a human being that is smarter than our perl script,
> happens to want to customize the "do not touch" section somehow, they would
> find this feature useful how?

  They can still customize the .htaccess, they just do it below the section.
Comment 16 Frédéric Buclin 2006-01-12 01:16:14 PST
(In reply to comment #15)
>   They can still customize the .htaccess, they just do it below the section.
> 

I still see no reason to deny review based on this. We could as well do it separately, in another bug.

Moreover, how do you expect to include your "do not touch" part for existing (customised) .htaccess? You would have to write an even bigger hack to extract the meaningful parts, both the "official" one set up by checksetup.pl and the customised one.
Comment 17 Max Kanat-Alexander 2006-01-12 11:46:02 PST
(In reply to comment #16)
> Moreover, how do you expect to include your "do not touch" part for existing
> (customised) .htaccess?

  See comment 11:

> In order to create that block, what we can do is check whether or not the
> .htaccess has that block in it. If it doesn't have the block, move the old
> .htaccess out of the way and replace it with a valid one. Make sure that you
> print a very obvious warning to the sysadmin that you've done this.
Comment 18 Max Kanat-Alexander 2006-02-25 15:30:40 PST
Okay, I'm fine with not having the "do not touch" block. But won't this patch fail anyhow if somebody has modified our parts of the .htaccess?
Comment 19 Teemu Mannermaa (:wicked) 2006-02-26 06:41:15 PST
(In reply to comment #18)
> Okay, I'm fine with not having the "do not touch" block. But won't this patch
> fail anyhow if somebody has modified our parts of the .htaccess?

If they remove the section denying CVS access, then it will be added back when checksetup is run. This same problem happens with all the other .htaccess manipulations. Also our own manipulations introduce bugs in the .htaccess files (duplicated entries). :(

In the long run, somekind of "do not touch" block should probably be developed. Maybe for all generated .htaccess files? Could that be fixed in separate bug so this security bug could be fixed now?
Comment 20 Frédéric Buclin 2006-02-28 17:07:22 PST
Comment on attachment 208194 [details] [diff] [review]
Fix permission and access right code in checksetup, V1 - v2.20 branch

Is this patch still the way we want to fix the problem? I remember a discussion with mkanat and justdave about having a block in .htaccess we could overwrite.
Comment 21 Teemu Mannermaa (:wicked) 2006-03-08 15:55:36 PST
Comment on attachment 205438 [details] [diff] [review]
Fix permission and access right code in checksetup, V1

So, what's final verdict about this patch?
Comment 22 Max Kanat-Alexander 2006-03-14 11:24:10 PST
This isn't a significant-enough security issue to stop our release of 2.22.
Comment 23 Teemu Mannermaa (:wicked) 2006-08-19 14:39:12 PDT
Comment on attachment 205438 [details] [diff] [review]
Fix permission and access right code in checksetup, V1

Rotted.
Comment 24 Frédéric Buclin 2006-11-04 06:12:15 PST
Removing all these old flags to make the UI a bit better.
Comment 25 Teemu Mannermaa (:wicked) 2007-03-18 07:04:17 PDT
Looks like everything except contrib directory is now handled in 3.0 branch and trunk. Anyway, I don't care about this bug anymore.
Comment 26 Frédéric Buclin 2008-12-07 06:25:28 PST
Bugzilla 2.20 is no longer supported. Retargetting to 2.22.
Comment 27 Frédéric Buclin 2009-07-28 13:08:15 PDT
Bugzilla 2.x is no longer supported. Retargetting to 3.0.
Comment 28 Max Kanat-Alexander 2009-08-18 22:09:16 PDT
Created attachment 395261 [details] [diff] [review]
v3 - contrib/

Okay, so we were still exposing the contrib/ directory to the world, which can be a big problem, because bzdbcopy.pl can have passwords in it. However, I can't seem to access bzdbcopy.pl on any Bugzilla that I've configured, so I assume that somehow we're blocking it already. Other files in contrib/ are accessible, though, and probably shouldn't be.

This sets Unix permissions correctly on the contrib/ directory.

The only question remaining is--should we also be putting a .htaccess in all of these directories? I think we probably should be, because that protects Windows as well.
Comment 29 Max Kanat-Alexander 2009-08-18 23:00:17 PDT
Created attachment 395268 [details] [diff] [review]
v4

Okay, this also creates a .htaccess file for every directory that shouldn't be accessed by the webserver. It *also* fixes a bug--CVS directories weren't correctly being set as 0700 (only their contents had their permissions set correctly).
Comment 30 Reed Loden [:reed] (use needinfo?) 2009-09-09 14:55:49 PDT
Comment on attachment 395268 [details] [diff] [review]
v4

>+use constant HT_DEFAULT_DENY => <<EOT;
>+# nothing in this directory is retrievable unless overridden by an .htaccess
>+# in a subdirectory
>+deny from all
>+EOT

I prefer the form:
order deny, allow
deny from all
Comment 31 Max Kanat-Alexander 2009-09-09 14:58:36 PDT
(In reply to comment #30)
> I prefer the form:
> order deny, allow
> deny from all

  That would be a separate bug--this is the exact text already in use, just being moved from a different place in the file.
Comment 32 Frédéric Buclin 2009-10-07 15:32:14 PDT
Could you explain why I'm able to visit https://landfill.bugzilla.org/bugzilla-tip/docs/en/xml/customization.xml despite the code specifically forbirds this? Please update your patch accordingly.
Comment 33 Frédéric Buclin 2009-11-08 15:25:17 PST
Comment on attachment 395268 [details] [diff] [review]
v4

Canceling review till comment 32 is answered.
Comment 34 Max Kanat-Alexander 2009-11-08 16:26:36 PST
(In reply to comment #32)
> Could you explain why I'm able to visit
> https://landfill.bugzilla.org/bugzilla-tip/docs/en/xml/customization.xml
> despite the code specifically forbirds this? Please update your patch
> accordingly.

  Ahh...because recurse_dirs was not calling glob() on its keys. Will fix in the next patch. That's probably worth backporting at least to 3.2 even if the rest of the patch doesn't turn out to be worth backporting.
Comment 35 Max Kanat-Alexander 2009-11-08 16:55:56 PST
Created attachment 411106 [details] [diff] [review]
patch for 3.4.4 and 3.2.5, v5

Okay, this fixes it.

I decided against creating .htaccess files in all the CVS directories. Their contents aren't *so* important that it justifies creating like 100 .htaccess files just to hide them.
Comment 36 Frédéric Buclin 2009-12-13 12:30:50 PST
Comment on attachment 411106 [details] [diff] [review]
patch for 3.4.4 and 3.2.5, v5

Looks good, and works as expected for 3.6 (didn't test on branches yet). r=LpSolit
Comment 37 Frédéric Buclin 2009-12-13 12:35:15 PST
Unless I miss something, t/ is already protected. Removing it from the bug summary.
Comment 38 Frédéric Buclin 2009-12-13 12:48:40 PST
The patch applies and works correctly with 3.4.4 and 3.2.5, but not with 3.0.10.
Comment 39 Max Kanat-Alexander 2009-12-13 14:44:49 PST
There's no .htaccess in t/ before this patch.
Comment 40 Frédéric Buclin 2009-12-13 14:48:57 PST
(In reply to comment #39)
> There's no .htaccess in t/ before this patch.

Maybe, but you still cannot access t/ from your web browser, so this is not an issue.
Comment 41 Max Kanat-Alexander 2009-12-13 14:54:48 PST
(In reply to comment #40)
> Maybe, but you still cannot access t/ from your web browser, so this is not an
> issue.

  No, that's only true on *nix. checksetup doesn't set permissions on Windows.
Comment 42 Frédéric Buclin 2009-12-23 10:20:42 PST
An unbitrotten patch is needed for 3.6 due to bug 519858.
Comment 43 Frédéric Buclin 2010-01-02 10:56:17 PST
Comment on attachment 411106 [details] [diff] [review]
patch for 3.4.4 and 3.2.5, v5

Max, we need new patches for 3.6 and 3.0.10.
Comment 44 Reed Loden [:reed] (use needinfo?) 2010-01-26 00:54:31 PST
This will be classified under CVE-2009-3989.
Comment 45 Max Kanat-Alexander 2010-01-31 09:25:58 PST
Created attachment 424493 [details] [diff] [review]
v5 (3.6)
Comment 46 Max Kanat-Alexander 2010-01-31 09:31:41 PST
Created attachment 424495 [details] [diff] [review]
v5 (3.0)
Comment 47 Frédéric Buclin 2010-01-31 11:54:11 PST
Comment on attachment 424493 [details] [diff] [review]
v5 (3.6)

Hum, despite what I said in comment 42, the patch still applies (with some fuzz). Strange. So r=LpSolit
Comment 48 Frédéric Buclin 2010-01-31 12:05:05 PST
Comment on attachment 424495 [details] [diff] [review]
v5 (3.0)

r=LpSolit
Comment 49 Max Kanat-Alexander 2010-01-31 16:37:27 PST
tip:

Checking in Bugzilla/Install/Filesystem.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Filesystem.pm,v  <--  Filesystem.pm
new revision: 1.46; previous revision: 1.45
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/contrib/.cvsignore,v
done
Checking in contrib/.cvsignore;
/cvsroot/mozilla/webtools/bugzilla/contrib/.cvsignore,v  <--  .cvsignore
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/t/.cvsignore,v
done
Checking in t/.cvsignore;
/cvsroot/mozilla/webtools/bugzilla/t/.cvsignore,v  <--  .cvsignore
initial revision: 1.1
done

3.4:

Checking in Bugzilla/Install/Filesystem.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Filesystem.pm,v  <--  Filesystem.pm
new revision: 1.34.2.3; previous revision: 1.34.2.2
done

3.2:

Checking in Bugzilla/Install/Filesystem.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Filesystem.pm,v  <--  Filesystem.pm
new revision: 1.29.2.3; previous revision: 1.29.2.2
done

3.0:

Checking in Bugzilla/Install/Filesystem.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Filesystem.pm,v  <--  Filesystem.pm
new revision: 1.18.2.4; previous revision: 1.18.2.3
done
Comment 50 Max Kanat-Alexander 2010-01-31 19:07:49 PST
Security advisory sent, removing from security group.

Note You need to log in before you can comment on or make changes to this bug.