Bug 419014 (CVE-2010-3764)

[SECURITY] Old charts are not project specific, and product names are viewable in graphs/

RESOLVED FIXED in Bugzilla 3.2

Status

()

defect
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: philip.gillissen, Assigned: LpSolit)

Tracking

2.12
Bugzilla 3.2
Dependency tree / graph
Bug Flags:
approval +
approval4.0 +
blocking4.0 +
approval3.6 +
blocking3.6.3 +
approval3.4 +
blocking3.4.9 +
approval3.2 +
blocking3.2.9 +

Details

Attachments

(5 attachments)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: Bugzilla 3.0.3

If you run two or more projects of Bugzilla on one codebase, the chart created by reports.cgi are saved into ./graphs, not in ./graphs/<project>/.
So if you let create the chart of "project_1" (-All-_NEW_ASSIGNED_REOPENED_UNCONFIRMED) und then do the same procedure in "project_2", you will see _exactly_ the same chart! That is because the reports.cgi only creates new versions of chart, if there's no old file with the composed filename.


Reproducible: Always

Steps to Reproduce:
1. Execute reports.cgi with product "-All-" in one project
2. Execute reports.cgi with product "-All-" in another project.
3. Compare chart urls (spoiler: it will be the same)
Actual Results:  
Both charts are the same, even if there are _no similarities_ between the two projects.
Bugzilla creates for multiple projects just one chart (for each product).

Expected Results:  
Bugzilla should create the charts per project, so that the order hierarchie look like this:
*graphs
 *project_1
  *-All-_NEW_ASSIGNED_REOPENED_UNCONFIRMED.png
 *project_2
  *-All-_NEW_ASSIGNED_REOPENED_UNCONFIRMED.png
  *... some other charts ...
Severity: normal → major
Severity: major → normal
Summary: Charts are _not_ project specific! → Charts ignore $ENV{PROJECT} (they are not project specific)
Assignee: gerv → charting
Is there any chance there will be a reaction?
OK, I can reproduce using Bugzilla 3.7. There are two problems here, which are both security related:

1) The problem described in comment 0: you may get stats from another installation, which may lead to confidential data leak (maybe you don't have access to the other installation, but could see its generated graphs anyway).

2) reports.cgi (aka Old Charts) only let you view stats for products you can see. If you try to hack the URL, it correctly throws an error telling you that you don't have access to the product. But you can browse the graphs/ directory from your web browser directly, giving you access not only to product names you cannot see, but also to stats for these products. Maybe this part should be fixed as part of bug 561797.
Group: bugzilla-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking3.6.1+
Target Milestone: --- → Bugzilla 3.6
Version: unspecified → 3.0.3
Target Milestone: Bugzilla 3.6 → Bugzilla 3.2
I think graphs/ should be moved into data/, so that each project has its own data/<project>/graphs/ directory.
  Probably the webdot/ and graphs/ directory should be the same directory, and graphs/ should randomize names just like we do for webdot files.
(In reply to comment #4)
>   Probably the webdot/ and graphs/ directory should be the same directory, and
> graphs/ should randomize names just like we do for webdot files.

Preventing browsing the graphs/ directory would help too. Is this done as part of bug 561797?
(In reply to comment #5)
> Is this done as part of bug 561797?

  Nope.
FWIW, webdot/ is the only web-accessible thing in data/. I'd rather move the stuff in webdot/ to graphs/, and then make graphs/ into $graphsdir.

Also, I think that we should do this after 3.6.1. That is, we should fix the suexec stuff in the coming security release, and then fix this in a later one.
Depends on: CVE-2010-0180
OK, to avoid conflicts, let's go with 3.6.2 for this one.
Flags: blocking3.6.1+ → blocking3.6.2+
No longer depends on: CVE-2010-0180
Depends on: 457373
No longer depends on: 457373
mkanat, do you plan to fix it yourself, or should I try to play with it?
Flags: blocking4.0+
Flags: blocking3.4.8+
Flags: blocking3.2.8+
  You're welcome to play with it.

  I think the best solution would be to move them both into the graphs/ directory, but we should probably do that only for the trunk.
(In reply to comment #10)
>   You're welcome to play with it.

Haha, that's not the answer I expected. :-D
We missed these releases. Will block again when the bug get some traction.
Flags: blocking3.6.2+
Flags: blocking3.4.8+
Flags: blocking3.2.8+
This patch solves several problems:
1) A user is no longer allowed to browse the graphs/ directory as it may contain plots for products you are not allowed to see.
2) Files now have a random name to not disclose product names you cannot see (else it's too easy to guess product names by brute force). File names are constructed using the same logic as Bugzilla::Token::issue_hash_token().
3) File names are now based on the product ID instead of the product name, so that product renaming is not a problem anymore. This fixes bug 604388.
4) If several projects have the same product names, they will no longer share their data as $PROJECT is now part of the hash to generate the file names.
5) Bug statuses and resolutions are no longer concatenated, which avoids troubles when one of them has a colon in them.

This patch doesn't apply on branches as bug 595682 didn't land there.
Assignee: charting → LpSolit
Status: NEW → ASSIGNED
Attachment #485024 - Flags: review?(wurblzap)
Attachment #485024 - Flags: review?(mkanat)
Blocks: 604388
Comment on attachment 485024 [details] [diff] [review]
patch for tip, v1

All right... This looks good and appears to work well; r+ so far. I'm confused, though, about bug 604388 comment 1. It says this patch here will cure bug 604388, but it doesn't. Am I missing something?
(In reply to comment #14)
> 604388, but it doesn't. Am I missing something?

I no longer use the product name to generate the file name, but the product ID. So if you rename a product, the file name won't change.

Remember that everytime you run collectstats.pl, old files are deleted. So you have to run it first before testing my patch (because it won't do the name -> ID transition for you).
Comment on attachment 485024 [details] [diff] [review]
patch for tip, v1

Clarified on irc -- the other bug is independent after all. Clearing the second review as agreed.
Attachment #485024 - Flags: review?(wurblzap)
Attachment #485024 - Flags: review?(mkanat)
Attachment #485024 - Flags: review+
No longer blocks: 604388
Thanks Marc! :) Backports coming soon.
Flags: approval?
Summary: Charts ignore $ENV{PROJECT} (they are not project specific) → [SECURITY] Old charts are not project specific, and product names are viewable in graphs/
reed, could we have a CVE number for it, please?
Comment on attachment 485024 [details] [diff] [review]
patch for tip, v1

Out of curiosity, why not move the graphs dir into $datadir, and then also have separate directories for each PROJECT, so you don't have to worry about direct access to bz_locations()->{'project'} (this is the only part of the code that does that)?
Oh, nevermind about data/, I see what I said before.
Comment on attachment 485024 [details] [diff] [review]
patch for tip, v1

Wait, nothing moves the old name-based data/mining/ files to the new id-based system. That needs to happen in Bugzilla::Install::Filesystem, in checksetup. Users should not be forced to run --regenerate.
Attachment #485024 - Flags: review-
Comment on attachment 485024 [details] [diff] [review]
patch for tip, v1

Auugh, nevermind, I had the same confusion that Marc had.
Attachment #485024 - Flags: review-
Attachment #485627 - Flags: review?(wurblzap)
Same patch as for 4.0, except that Filesystem.pm uses $ws_readable instead of WS_SERVE.
Attachment #485629 - Flags: review?(wurblzap)
Attachment #485631 - Flags: review?(wurblzap)
Blocks: 606854
And here is the final backport. :)
Attachment #485635 - Flags: review?(wurblzap)
Backports are pretty trivial to review as they only fix bitrot compared to the one Marc already reviewed. So we can take this bug for the next releases this week.

Marc, could I have reviews asap, please? :)
Flags: blocking3.6.3+
Flags: blocking3.4.9+
Flags: blocking3.2.9+
assigning CVE CVE-2010-3764
Alias: CVE-2010-3764
Attachment #485627 - Flags: review?(wurblzap) → review+
Attachment #485629 - Flags: review?(wurblzap) → review+
Comment on attachment 485631 [details] [diff] [review]
patch for 3.4, v1

r+ provided it's safe to move the call to switch_to_shadow_db() around.
Attachment #485631 - Flags: review?(wurblzap) → review+
(In reply to comment #29)
> r+ provided it's safe to move the call to switch_to_shadow_db() around.

Yes, it is, because the code around it doesn't access the DB.
Comment on attachment 485635 [details] [diff] [review]
patch for 3.2, v1

All backport r+ by inspection, relying on your testing and comment 27.
Attachment #485635 - Flags: review?(wurblzap) → review+
(In reply to comment #31)
> All backport r+ by inspection, relying on your testing and comment 27.

Cool, thanks for the reviews. :) And yes, I tested all the backports.
Flags: approval4.0?
Flags: approval3.6?
Flags: approval3.4?
Flags: approval3.2?
As near as I can tell, this was introduced by bug 6682 in Bugzilla 2.12? Interestingly enough, Gerv actually originally proposed a secure solution in that bug (basically generating a temp file name) and somehow that was never done.
Version: 3.0.3 → 2.12
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval3.6?
Flags: approval3.6+
Flags: approval3.4?
Flags: approval3.4+
Flags: approval3.2?
Flags: approval3.2+
Flags: approval+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified collectstats.pl
modified reports.cgi
modified Bugzilla/Constants.pm
modified Bugzilla/Install/Filesystem.pm
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/reports/old-charts.html.tmpl
Committed revision 7589.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified collectstats.pl
modified reports.cgi
modified Bugzilla/Constants.pm
modified Bugzilla/Install/Filesystem.pm
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/reports/old-charts.html.tmpl
Committed revision 7471.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified collectstats.pl
modified reports.cgi
modified Bugzilla/Constants.pm
modified Bugzilla/Install/Filesystem.pm
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/reports/old-charts.html.tmpl
Committed revision 7199.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.4/
modified collectstats.pl
modified reports.cgi
modified Bugzilla/Constants.pm
modified Bugzilla/Install/Filesystem.pm
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/reports/old-charts.html.tmpl
Committed revision 6780.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.2/
modified collectstats.pl
modified reports.cgi
modified Bugzilla/Constants.pm
modified Bugzilla/Install/Filesystem.pm
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/reports/old-charts.html.tmpl
Committed revision 6401.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Security advisory sent, unlocking bug.
Group: bugzilla-security
Blocks: 313739
You need to log in before you can comment on or make changes to this bug.