314871 - (CVE-2009-3989) [SECURITY] Web browsers can see CVS/, contrib/, docs/, and t/ directory files

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Joel Peshkin]

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]]

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]

(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]

When I set it as a trivial security bug, I wasn't kidding :-)

Comment 4

[Frédéric Buclin]

Note that you can also display the content of contrib/ and skins/

Comment 5

[Teemu Mannermaa (:wicked)]

(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)]

Attachment: 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)]

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]

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]

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)]

Attachment: 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]

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]

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]

(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]]

(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]

(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]

(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]

(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]

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)]

(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]

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)]

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]

This isn't a significant-enough security issue to stop our release of 2.22.

Comment 23

[Teemu Mannermaa (:wicked)]

Comment on attachment 205438 [details] [diff] [review]
Fix permission and access right code in checksetup, V1

Rotted.

Comment 24

[Frédéric Buclin]

Removing all these old flags to make the UI a bit better.

Comment 25

[Teemu Mannermaa (:wicked)]

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]

Bugzilla 2.20 is no longer supported. Retargetting to 2.22.

Comment 27

[Frédéric Buclin]

Bugzilla 2.x is no longer supported. Retargetting to 3.0.

Comment 28

[Max Kanat-Alexander]

Attachment: 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]

Attachment: 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]]

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]

(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]

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]

Comment on attachment 395268 [details] [diff] [review]
v4

Canceling review till comment 32 is answered.

Comment 34

[Max Kanat-Alexander]

(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]

Attachment: 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]

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]

Unless I miss something, t/ is already protected. Removing it from the bug summary.

Comment 38

[Frédéric Buclin]

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]

There's no .htaccess in t/ before this patch.

Comment 40

[Frédéric Buclin]

(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]

(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]

An unbitrotten patch is needed for 3.6 due to bug 519858.

Comment 43

[Frédéric Buclin]

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]]

This will be classified under CVE-2009-3989.

Comment 45

[Max Kanat-Alexander]

Attachment: v5 (3.6)

Comment 46

[Max Kanat-Alexander]

Attachment: v5 (3.0)

Comment 47

[Frédéric Buclin]

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]

Comment on attachment 424495 [details] [diff] [review]
v5 (3.0)

r=LpSolit

Comment 49

[Max Kanat-Alexander]

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]

Security advisory sent, removing from security group.