Closed
Bug 314871
(CVE-2009-3989)
Opened 19 years ago
Closed 15 years ago
[SECURITY] Web browsers can see CVS/, contrib/, docs/, and t/ directory files
Categories
(Bugzilla :: Bugzilla-General, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: bugreport, Assigned: mkanat)
References
Details
Attachments
(3 files, 4 obsolete files)
(deleted),
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
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•19 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•19 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•19 years ago
|
||
When I set it as a trivial security bug, I wasn't kidding :-)
Comment 4•19 years ago
|
||
Note that you can also display the content of contrib/ and skins/
Comment 5•19 years ago
|
||
(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
Comment 6•19 years ago
|
||
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?
Updated•19 years ago
|
Attachment #205438 -
Flags: review? → review?(mkanat)
Comment 7•19 years ago
|
||
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•19 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•19 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+
Comment 10•19 years ago
|
||
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•19 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•19 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•19 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•19 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•19 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•19 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•19 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•19 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?
Comment 19•19 years ago
|
||
(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•19 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 21•19 years ago
|
||
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-
Updated•19 years ago
|
Attachment #205438 -
Flags: review? → review?(mkanat)
Assignee | ||
Comment 22•19 years ago
|
||
This isn't a significant-enough security issue to stop our release of 2.22.
Flags: blocking2.22+ → blocking2.22-
Assignee | ||
Updated•19 years ago
|
Flags: blocking2.20.2+ → blocking2.20.2-
Comment 23•18 years ago
|
||
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•18 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-
Updated•18 years ago
|
Attachment #208194 -
Flags: review?
Comment 25•18 years ago
|
||
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
Updated•16 years ago
|
Group: webtools-security → bugzilla-security
Updated•16 years ago
|
Group: bugzilla-security → webtools-security
Updated•16 years ago
|
Group: webtools-security → bugzilla-security
Comment 26•16 years ago
|
||
Bugzilla 2.20 is no longer supported. Retargetting to 2.22.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Comment 27•15 years ago
|
||
Bugzilla 2.x is no longer supported. Retargetting to 3.0.
Target Milestone: Bugzilla 2.22 → Bugzilla 3.0
Assignee | ||
Comment 28•15 years ago
|
||
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•15 years ago
|
||
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)
Comment 30•15 years ago
|
||
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•15 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.
Comment 32•15 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•15 years ago
|
Attachment #395268 -
Flags: review?(LpSolit)
Comment 33•15 years ago
|
||
Comment on attachment 395268 [details] [diff] [review] v4 Canceling review till comment 32 is answered.
Assignee | ||
Comment 34•15 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•15 years ago
|
||
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•15 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•15 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•15 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•15 years ago
|
Whiteboard: [backport needed for 3.0.11]
Assignee | ||
Comment 39•15 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•15 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•15 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•15 years ago
|
||
An unbitrotten patch is needed for 3.6 due to bug 519858.
Comment 43•15 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
Comment 44•15 years ago
|
||
This will be classified under CVE-2009-3989.
Assignee | ||
Updated•15 years ago
|
Alias: CVE-2009-3989
Assignee | ||
Comment 45•15 years ago
|
||
Attachment #424493 -
Flags: review?(LpSolit)
Assignee | ||
Comment 46•15 years ago
|
||
Attachment #424495 -
Flags: review?(LpSolit)
Assignee | ||
Updated•15 years ago
|
Flags: blocking3.0.11+
Comment 47•15 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•15 years ago
|
||
Comment on attachment 424495 [details] [diff] [review] v5 (3.0) r=LpSolit
Attachment #424495 -
Flags: review?(LpSolit) → review+
Updated•15 years ago
|
Flags: blocking3.2.6+
Flags: approval3.0?
Whiteboard: [backport needed for 3.0.11]
Updated•15 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•15 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
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 50•15 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.
Description
•