419014 - (CVE-2010-3764) [SECURITY] Old charts are not project specific, and product names are viewable in graphs/

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[philip.gillissen]

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

Comment 1

[philip.gillissen]

Is there any chance there will be a reaction?

Comment 2

[Frédéric Buclin]

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.

Comment 3

[Frédéric Buclin]

I think graphs/ should be moved into data/, so that each project has its own data/<project>/graphs/ directory.

Comment 4

[Max Kanat-Alexander]

  Probably the webdot/ and graphs/ directory should be the same directory, and graphs/ should randomize names just like we do for webdot files.

Comment 5

[Frédéric Buclin]

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

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> Is this done as part of bug 561797?

  Nope.

Comment 7

[Max Kanat-Alexander]

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.

Comment 8

[Frédéric Buclin]

OK, to avoid conflicts, let's go with 3.6.2 for this one.

Comment 9

[Frédéric Buclin]

mkanat, do you plan to fix it yourself, or should I try to play with it?

Comment 10

[Max Kanat-Alexander]

  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.

Comment 11

[Frédéric Buclin]

(In reply to comment #10)
>   You're welcome to play with it.

Haha, that's not the answer I expected. :-D

Comment 12

[Frédéric Buclin]

We missed these releases. Will block again when the bug get some traction.

Comment 13

[Frédéric Buclin]

Attachment: patch for tip, v1

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.

Comment 14

[Marc Schumann [:Wurblzap]]

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?

Comment 15

[Frédéric Buclin]

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

[Marc Schumann [:Wurblzap]]

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.

Comment 17

[Frédéric Buclin]

Thanks Marc! :) Backports coming soon.

Comment 18

[Frédéric Buclin]

reed, could we have a CVE number for it, please?

Comment 19

[Max Kanat-Alexander]

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

Comment 20

[Max Kanat-Alexander]

Oh, nevermind about data/, I see what I said before.

Comment 21

[Max Kanat-Alexander]

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.

Comment 22

[Max Kanat-Alexander]

Comment on attachment 485024 [details] [diff] [review]
patch for tip, v1

Auugh, nevermind, I had the same confusion that Marc had.

Comment 23

[Frédéric Buclin]

Attachment: patch for 4.0, v1

Comment 24

[Frédéric Buclin]

Attachment: patch for 3.6, v1

Same patch as for 4.0, except that Filesystem.pm uses $ws_readable instead of WS_SERVE.

Comment 25

[Frédéric Buclin]

Attachment: patch for 3.4, v1

Comment 26

[Frédéric Buclin]

Attachment: patch for 3.2, v1

And here is the final backport. :)

Comment 27

[Frédéric Buclin]

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? :)

Comment 28

[Daniel Veditz [:dveditz]]

assigning CVE CVE-2010-3764

Comment 29

[Marc Schumann [:Wurblzap]]

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.

Comment 30

[Frédéric Buclin]

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

[Marc Schumann [:Wurblzap]]

Comment on attachment 485635 [details] [diff] [review]
patch for 3.2, v1

All backport r+ by inspection, relying on your testing and comment 27.

Comment 32

[Frédéric Buclin]

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

Comment 33

[Max Kanat-Alexander]

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.

Comment 34

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/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://[e-mail]/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://[e-mail]/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://[e-mail]/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://[e-mail]/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.

Comment 35

[Max Kanat-Alexander]

Security advisory sent, unlocking bug.