Bug 314871 (CVE-2009-3989)

[SECURITY] Web browsers can see CVS/, contrib/, docs/, and t/ directory files

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Bugzilla-General
--
trivial
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Joel Peshkin, Assigned: Max Kanat-Alexander)

Tracking

2.21
Bugzilla 3.0
Bug Flags:
approval +
blocking3.6 +
approval3.4 +
blocking3.4.5 +
approval3.2 +
blocking3.2.6 +
approval3.0 +
blocking3.0.11 +

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
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

12 years ago
(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.
(Reporter)

Comment 3

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

Comment 4

12 years ago
Note that you can also display the content of contrib/ and skins/
(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.
Assignee: general → wicked
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.
Attachment #205438 - Flags: review?
Attachment #205438 - Flags: review? → review?(mkanat)
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..
Status: NEW → ASSIGNED
Flags: blocking2.22?
Flags: blocking2.20.2?
Flags: blocking2.18.5?
Flags: blocking2.16.11?
Summary: Web browsers can see CVS directory files → Web browsers can see CVS, contrib, t and docs directory files
Target Milestone: --- → Bugzilla 2.16
(Assignee)

Comment 8

12 years ago
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.
Flags: blocking2.22?
Flags: blocking2.22+
Flags: blocking2.20.2?
Flags: blocking2.20.2+
Flags: blocking2.18.5?
Flags: blocking2.18.5-
Flags: blocking2.16.11?
Flags: blocking2.16.11-
Target Milestone: Bugzilla 2.16 → Bugzilla 2.20

Comment 9

12 years ago
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
Attachment #205438 - Flags: review+
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.
Attachment #208194 - Flags: review?
(Assignee)

Comment 11

12 years ago
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.
Attachment #205438 - Flags: review?(mkanat) → review-

Comment 12

12 years ago
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.
(Assignee)

Comment 13

12 years ago
(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

12 years ago
(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?
(Assignee)

Comment 15

12 years ago
(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

12 years ago
(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.
(Assignee)

Comment 17

12 years ago
(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.
(Assignee)

Comment 18

12 years ago
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?
(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

12 years ago
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 on attachment 205438 [details] [diff] [review]
Fix permission and access right code in checksetup, V1

So, what's final verdict about this patch?
Attachment #205438 - Flags: review?
Attachment #205438 - Flags: review-
Attachment #205438 - Flags: review? → review?(mkanat)
(Assignee)

Comment 22

11 years ago
This isn't a significant-enough security issue to stop our release of 2.22.
Flags: blocking2.22+ → blocking2.22-
(Assignee)

Updated

11 years ago
Flags: blocking2.20.2+ → blocking2.20.2-
Comment on attachment 205438 [details] [diff] [review]
Fix permission and access right code in checksetup, V1

Rotted.
Attachment #205438 - Attachment is obsolete: true
Attachment #205438 - Flags: review?(mkanat)
Attachment #205438 - Flags: review?

Comment 24

11 years ago
Removing all these old flags to make the UI a bit better.
Flags: blocking2.22-
Flags: blocking2.20.2-
Flags: blocking2.18.5-
Flags: blocking2.16.11-
Attachment #208194 - Flags: review?
Looks like everything except contrib directory is now handled in 3.0 branch and trunk. Anyway, I don't care about this bug anymore.
Assignee: wicked+bz → general
Status: ASSIGNED → NEW
Group: webtools-security → bugzilla-security
Group: bugzilla-security → webtools-security
Group: webtools-security → bugzilla-security

Comment 26

9 years ago
Bugzilla 2.20 is no longer supported. Retargetting to 2.22.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22

Comment 27

8 years ago
Bugzilla 2.x is no longer supported. Retargetting to 3.0.
Target Milestone: Bugzilla 2.22 → Bugzilla 3.0
(Assignee)

Comment 28

8 years ago
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.
Assignee: general → mkanat
Attachment #208194 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #395261 - Flags: review?(LpSolit)
(Assignee)

Comment 29

8 years ago
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).
Attachment #395261 - Attachment is obsolete: true
Attachment #395268 - Flags: review?(LpSolit)
Attachment #395261 - Flags: review?(LpSolit)

Updated

8 years ago
Blocks: 515454
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
(Assignee)

Comment 31

8 years ago
(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.
(Assignee)

Updated

8 years ago
No longer blocks: 515454

Comment 32

8 years ago
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.

Updated

8 years ago
Attachment #395268 - Flags: review?(LpSolit)

Comment 33

8 years ago
Comment on attachment 395268 [details] [diff] [review]
v4

Canceling review till comment 32 is answered.
(Assignee)

Comment 34

8 years ago
(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.
(Assignee)

Comment 35

8 years ago
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.
Attachment #395268 - Attachment is obsolete: true
Attachment #411106 - Flags: review?(LpSolit)

Comment 36

8 years ago
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
Attachment #411106 - Flags: review?(LpSolit) → review+

Comment 37

8 years ago
Unless I miss something, t/ is already protected. Removing it from the bug summary.
Flags: approval?
Summary: Web browsers can see CVS, contrib, t and docs directory files → [SECURITY] Web browsers can see CVS/, contrib/ and docs/ directory files

Comment 38

8 years ago
The patch applies and works correctly with 3.4.4 and 3.2.5, but not with 3.0.10.
Flags: blocking3.6+
Flags: blocking3.4.5+
Flags: approval3.4?
Flags: approval3.2?

Updated

8 years ago
Blocks: 532517

Updated

8 years ago
Whiteboard: [backport needed for 3.0.11]
(Assignee)

Comment 39

8 years ago
There's no .htaccess in t/ before this patch.
Summary: [SECURITY] Web browsers can see CVS/, contrib/ and docs/ directory files → [SECURITY] Web browsers can see CVS/, contrib/, docs/, and t/ directory files

Comment 40

8 years ago
(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.
(Assignee)

Comment 41

8 years ago
(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

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

Comment 43

8 years ago
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.
Attachment #411106 - Attachment description: v5 → patch for 3.4.4 and 3.2.5, v5
This will be classified under CVE-2009-3989.
(Assignee)

Updated

8 years ago
Alias: CVE-2009-3989
(Assignee)

Comment 45

8 years ago
Created attachment 424493 [details] [diff] [review]
v5 (3.6)
Attachment #424493 - Flags: review?(LpSolit)
(Assignee)

Comment 46

8 years ago
Created attachment 424495 [details] [diff] [review]
v5 (3.0)
Attachment #424495 - Flags: review?(LpSolit)
(Assignee)

Updated

8 years ago
Flags: blocking3.0.11+

Comment 47

8 years ago
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
Attachment #424493 - Flags: review?(LpSolit) → review+

Comment 48

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

r=LpSolit
Attachment #424495 - Flags: review?(LpSolit) → review+

Updated

8 years ago
Flags: blocking3.2.6+
Flags: approval3.0?
Whiteboard: [backport needed for 3.0.11]

Updated

8 years ago
Flags: approval?
Flags: approval3.4?
Flags: approval3.4+
Flags: approval3.2?
Flags: approval3.2+
Flags: approval3.0?
Flags: approval3.0+
Flags: approval+
(Assignee)

Comment 49

8 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 50

8 years ago
Security advisory sent, removing from security group.
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.