235510 - Password exposed in URL to chart image if login required to access a chart

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Dave Miller [:justdave]]

Steps to reproduce:

1) View a new series-based chart.
2) Bookmark the results page.
3) Log out of Bugzilla.
4) Access the bookmark.  You'll be prompted to log in.
5) Log in.
6) View source on the resulting chart page.

Actual results:

The login ID and password are included in the query string of the URL to the
image (thus also showing up in web server logs).

Expected results:

The login ID and password should not be included.

Comment 1

[Gervase Markham [:gerv]]

Hmm. If we don't do that, will charting still work for people with cookies
turned off?

Gerv

Comment 2

[Bradley Baetz (:bbaetz)]

Storing the username/pw in the URL is bad.

Remind me why we can't just require cookies for this? 'privacy' is not an answer
unless you can explain exactly what bugzilla's cookies do to have privacy
concerns. Its not like cookies lack browser support.....

The cookie IP restrictions is more of an issue, though, for this.

We could do HTTP basic auth, I guess, but thats a lot less secure.

Comment 3

[Gervase Markham [:gerv]]

> Remind me why we can't just require cookies for this?

Because currently Bugzilla policy is that everything works (albeit less
conveniently) without cookies. If we think that in this age of decent browser
cookie control we want to relax that, then that's fine - but we need to do so
explicitly with discussion, rather than as the easy solution to a technical problem.

Gerv

Comment 4

[Dave Miller [:justdave]]

It would be better to issue a one-time use token and use that in the URL than
the expose the user's password.  We would only need to do this if
Bugzilla_password is present in the query string, as otherwise we can assume
they had a cookie already when they came in.

Note that uploading an attachment is currently impossible (and always has been)
if cookies are off, so this wouldn't be an isolated problem.

Comment 5

[Bradley Baetz (:bbaetz)]

Yes, I know it was our policy, I was asking why :)

A one-time use token may be useful for other reasons, though, I guess.

Comment 6

[Dave Miller [:justdave]]

Just for the record, given what's at stake here, I would be much happier having
cookieless operation broken for the time being than waiting for a while for some
kind of token interface to be implemented if it's going to take longer to do
that.  This is a new feature afterall, so we're not removing existing
functionality from cookieless users.

Comment 7

[Gervase Markham [:gerv]]

We also need the one-time tokens for attachments-on-a-different-host.

So the long term plan is: implement one-time tokens in a generic way, such that
"the presence of a valid token in a login_token param in the URL authenticates
that single request only, and invalidates the token." That way, we can use it
for almost anything to bridge the cookie gap.

In the short term, you are right - let's break it. It's fairly easy, I think -
just filter out Bugzilla_login and Bugzilla_password when we canonicalise the URL.

/me goes to look.

Gerv

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here we go.

Gerv

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 142418 [details] [diff] [review]
Patch v.1

bbaetz?

Gerv

Comment 10

[Dave Miller [:justdave]]

If you're grabbing $cgi->query_string, then just $cgi->delete() those two params
before grabbing $cgi->query_string.

Comment 11

[Dave Miller [:justdave]]

bah, my mail is slow. :)  I made that comment before I realized you'd posted a
patch :)  What you did works for me :)

Comment 12

[Gervase Markham [:gerv]]

canonicalise_query() does all the hard work for us - check the patch :-)

Gerv

Comment 13

[Christian Reis]

Comment on attachment 142418 [details] [diff] [review]
Patch v.1

Obvious enough. Open a follow-up bug for the token?

Comment 14

[Gervase Markham [:gerv]]

Requesting approval.

Follow-up bug 236497 filed.

Gerv

Comment 15

[Dave Miller [:justdave]]

Holding approval for 2.18rc1 release and security advisory.  (it's sooner than
you think ;)

Thanks guys!

Comment 16

[Gervase Markham [:gerv]]

Oops, sorry, flubbed the approval request.

Gerv

Comment 17

[Dave Miller [:justdave]]

sooner than we think.  hah!  Live and Learn.  Well, now it's here finally.

Comment 18

[Joel Peshkin]

checked in

Comment 19

[Dave Miller [:justdave]]

Clearing the security flag on disclosed bugs