195530 - Make javascript version of buglists available

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Gervase Markham [:gerv]]

If there was a JS version of buglists, with each bug having the elements as a JS
array, that would make it possible for people to have buglinks on their page
which reflected the correct state of the relevant Bugzilla bug. Also, Jesse's
buglinkify bookmarklet (http://www.squarefree.com/bookmarklets/mozilla.html)
would be able to make up-to-the-minute live bookmarklets.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This is a new template which allows &ctype=js on buglists. Simple, yet
effective.

Comment 2

[Gervase Markham [:gerv]]

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

Myk - you might think this is cool enough to spend time reviewing :-) It's a
nine-line standalone template, so it shouldn't take too long...

Gerv

Comment 3

[Bradley Baetz (:bbaetz)]

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

r=bbaetz. Gerv convinced me that this is better as an array then as a hash. I
guess we can always change it later if needed.

Note that this can't be included inline in a <script> as is, in cae a result
has </ in it. I don't think that thats hte intended use here, though.

Comment 4

[timeless]

gerv: i don't think netscape should be the initial developer.

Comment 5

[Jesse Ruderman]

Can you add a callback to the bottom, so a bookmarklet (or a web page using the
defer attribute on the script element) can know when the bug list is available?
 Something like
if (window.buglistCallback) buglistCallback();

Comment 6

[Gervase Markham [:gerv]]

timeless: I hacked this template around from the RDF version, and so on. It's a
derivative work.

jesse: yes, sorry, forgot about that. Will do.

Gerv

Comment 7

[Gervase Markham [:gerv]]

Attachment: Patch v.2 (initial implementation)

With callback.

Gerv

Comment 8

[Gervase Markham [:gerv]]

Comment on attachment 118967 [details] [diff] [review]
Patch v.2 (initial implementation)

Jesse: could you just check this is OK? Thanks.

Gerv

Comment 9

[Jesse Ruderman]

Comment on attachment 118967 [details] [diff] [review]
Patch v.2 (initial implementation)

Looks ok.

Comment 10

[Dave Miller [:justdave]]

already got a+, go for it.

Comment 11

[Gervase Markham [:gerv]]

Fixed.

Checking in template/en/default/list/list.js.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.js.tmpl,v  <--
 list.js.tmpl
initial revision: 1.1
done

Gerv

Comment 12

[Jesse Ruderman]

Attachment: first shot at a bookmarklet

This page grabs basic info about two bugs from landfill, and uses it for
tooltips and the strike-through effect.  The script is set up so you can easily
turn it into a bookmarklet using the Brainjar Crunchinator or by removing the
line breaks.

I'll add this to http://www.squarefree.com/bookmarklets/mozilla.html (as
"bugtips"?) after b.m.o updates.

Comment 13

[Gervase Markham [:gerv]]

Nice :-) 

I'm vaguely worried that people will build it into pages instead of using it as
a bookmarklet, thereby DOSing Bugzilla accidentally if a lot of people access
that page. Could you add a note to the source asking people not to do that? :-)

Gerv

Comment 14

[Jesse Ruderman]

Btw, does this template allow hidden bugs to be listed?  It shouldn't.

It also might make sense to disable this template by default, in case bugzillas
are used behind firewalls.

Comment 15

[Gervase Markham [:gerv]]

> Btw, does this template allow hidden bugs to be listed?  It shouldn't.

It does the same search as you get when you get an HTML buglist - so, if the
user is logged in, it'll show their hidden bugs, but if they aren't, it won't.

Hmm... you are right. Thinking about it, this does provide a way for a malicious
website owner, who entices a Bugzilla user with access to confidential bugs to
visit their site, to steal information about those bugs. Drat.

So, what can we do about it?

Gerv

Comment 16

[Dave Miller [:justdave]]

Check the Referer header, and if it's not there or starts with something other
than Param("urlbase") then pretend the user isn't logged in.

If they have access to forge the Referer header, they'd have to steal the cookie
as well for it to do any good.

Comment 17

[Gervase Markham [:gerv]]

I doubt this template would be used for info on a Bugzilla install itself; we
have better mechanisms for getting the data.

So, the conclusion we come to is that all we can do is always assume the user is
not logged in. Would doing that make this template useless? I don't think so -
it's still handy for Bugzillas which are mostly public.

This would be a security issue, so reopening bug.

Gerv

Comment 18

[Dave Miller [:justdave]]

removing approval since this was reopened and there's nothing new to approve yet

Comment 19

[Gervase Markham [:gerv]]

We need bbaetz and his login expertise here.

bbaetz: we need, at the top of buglist.cgi, to tell our new auth mechanism to
log in the person as an anonymous user (if they are using ctype=js). Ideally,
one would just make the cookie param and URL params undef. However, something
like this:

if ($::FORM{'ctype'} eq "js") {
    $cgi->param("Bugzilla_login", undef);
    $cgi->param("Bugzilla_password", undef);
    $cgi->cookie("Bugzilla_login", undef);
    $cgi->cookie("Bugzilla_logincookie", undef);
}

doesn't seem to do the trick - for the cookies, at least.

Any ideas how we can do this within the new auth framework?

Gerv

Comment 20

[Bradley Baetz (:bbaetz)]

You can't overrwite cookies that way; what you're doing is creating a
CGI::Cookie object, but then ignoring the result.

The real fix is to just not call ->login, but since thats happening in the .pl
at the moment you can't really arrange that.

You could call Bugzilla->logout afterwards, I guess. That doesn't actually
remove the cookie/DB entry yet, and when it does all the cgi files will have
been converted already. Add an appropriate comment, though.

Comment 21

[Gervase Markham [:gerv]]

Attachment: Patch B v.1 (security fix)

This makes things secure by always logging you out when you are using the JS
format. It also adds a comment warning to the JS template, so that people
aren't surprised.

Gerv

Comment 22

[Christian Reis]

Gerv: your comment might be more specific in saying that *accessing a link* will
log out the user; that's very surprising behaviour, honestly,

Comment 23

[Christian Reis]

Gerv wrote:

>> Btw, does this template allow hidden bugs to be listed?  It shouldn't.
>
> It does the same search as you get when you get an HTML buglist - so, if the
> user is logged in, it'll show their hidden bugs, but if they aren't, it won't.
>
> Hmm... you are right. Thinking about it, this does provide a way for a 
> malicious website owner, who entices a Bugzilla user with access to 
> confidential bugs to visit their site, to steal information about those bugs. 
> Drat.

Just so I understand this, what is the security problem mentioned? That a user
logged in when visiting somebody's site will get correct buglinks? And the
impact of this is that through Javascript this correct, private, buglink (with
status and description) can be somehow transmitted to the malicious site operator?

Can you describe a viable exploit here?

Comment 24

[Gervase Markham [:gerv]]

kiko: accessing the link will not log the user out; it only logs them out for
that transaction. In other words, it doesn't delete their cookies, just ignores
them that once. 

The security hole is as follows:

The JS buglist file has a callback implanted in it. This allows site
http://www.happy.com to do <script src="bugzilla/...&format=js">, get the
buglist as an Array object, and then get the data passed to a JS function stored
in navigator.buglistCallback. This function can do whatever site-specific stuff
they want to with it - like updating bug link tooltips.

However, it also allows http://www.evil.com, when a Bugzilla user visits it
while logged in, to do <script src="bugzilla/...&group is not null&format=js">
and have JS in the callback which sends the summaries of secure bugs somewhere
where Dr. Evil can read them at leisure.

Gerv

Comment 25

[Dave Miller [:justdave]]

Could you not do the same thing by loading the HTML buglist in a hidden iframe
and using DOM scripting to extract the content?

Comment 26

[Dave Miller [:justdave]]

OK, guess not.  Jesse pointed this out in IRC:
http://www.mozilla.org/projects/security/components/same-origin.html
:)

Comment 27

[Gervase Markham [:gerv]]

bbaetz, justdave: if we released 2.17.5 with this bug unfixed, we would be
releasing it with another hole with the same or worse severity as one in the
security advisory (other people can access the summaries of secure bugs.) So we
need to review and check in Patch B v.1 :-)

Gerv

Comment 28

[Christian Reis]

[Bradley, from comment 20]
> You could call Bugzilla->logout afterwards, I guess. That doesn't actually
> remove the cookie/DB entry yet, and when it does all the cgi files will have
> been converted already. Add an appropriate comment, though.

[Gerv, from comment 24]
> kiko: accessing the link will not log the user out; it only logs them out for
> that transaction. In other words, it doesn't delete their cookies, just 
> ignores them that once. 

Gerv, Bradley said "yet". AIUI, this means that in the future, logout() will/may
log the person off, or am I mistaken? 

And if so, does this mean that a malicious user may place Javascript somewhere
to annoy a user by logging out? Hmm, that's probably possible by embedding a
link to relogin.cgi, anyway.

I was also considering Dave's remark in comment 16, but I think that might
introduce another security issue where you open the "internal" page inside an
iframe and accessed *its* variable from an external site.

I guess I'd be happier if credentials were guaranteed to be ignored just for the
specific page (never killing cookies), but if you don't think (or any of the
above remarks) are relevant, tell me and I'll try marking an r+.

Comment 29

[Bradley Baetz (:bbaetz)]

I want login to do that (rather than relogin.cgi). If that changes, I'd
obviously have to fix/update all the callers to keep the old semantics where
appropriate

Comment 30

[Gervase Markham [:gerv]]

kiko said:
> I guess I'd be happier if credentials were guaranteed to be ignored just for the
> specific page (never killing cookies)

That's what this patch does currently. If the semantics of logout() change, I'm
sure bbaetz will, as he says, update the places it is used.

IMO, this is good to go.

Gerv

Comment 31

[Dave Miller [:justdave]]

Comment on attachment 131024 [details] [diff] [review]
Patch B v.1 (security fix)

bbaetz is on vacation until Monday, but this looks good to me.

Requesting a second review due to security implications.

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 131024 [details] [diff] [review]
Patch B v.1 (security fix)

>Index: buglist.cgi

>+# The js ctype presents a security risk; a malicious site could use it to 
>+# gather information about secure bugs. So, we only allow public bugs to be
>+# retrieved with this format.

Nit: keeping prepositions together with their prepositional phrases makes
multi-line text easier to read, i.e.:

# The js ctype presents a security risk; a malicious site could use it
# to gather information about secure bugs. So, we only allow public bugs
# to be retrieved with this format.


>Index: template/en/default/list/list.js.tmpl

>+// Note: only publically-accessible bugs (those not in any group) will be
>+// listed when using this JavaScript format. This is to prevent malicious
>+// sites stealing information about secure bugs.

Nit: publically -> publicly.

Otherwise looks good and is the right fix for the problem stated.  There's no
exploit against public data besides users being tricked into thinking the data
belongs to the third-party site, which is already possible (although harder)
without this feature.

The only other possible exploit I can think of is if the code called by the
callback could find a way to run in the Bugzilla installation's domain; we
should check on that, but that doesn't affect this fix.

r=myk

Comment 33

[Dave Miller [:justdave]]

Re: comment 27, I sure wish I'd seen the bugmail about that comment before we
released 2.17.5.

For future reference there's three things that could have been done that would
have gained this bug visibility during the release process:

1) set it to block the 2.17.5 release bug
2) put "[wanted for 2.17.5]" on the status whiteboard
3) place it in the webtools-security group

If any one of those three had been done, this would have stood out like a sore
thumb during the release process.

The original implementation probably should have been backed out when this bug
was reopened.  With it being left in place this should have been opened as a new
bug.

But live and learn.  Draft security advisory for this issue has been mailed to
reviewers for review.  I'd like to release 2.17.6 tomorrow.  Let's get this
checked in.

Comment 34

[Gervase Markham [:gerv]]

Dave: it would have definitely been better if I'd done one or more of those
things you list.

I remember vaguely wondering whether this bug was in the security group, but not
checking. I also remember vaguely rejecting the idea of adding status whiteboard
stuff, because I was under the impression that it was you who decided whether
something was wanted for 2.17.5. Is that not how it works?

Anyway, final patch coming up for recording purposes.

Gerv

Comment 35

[Gervase Markham [:gerv]]

Fixed.

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.237; previous revision: 1.236
done
Checking in template/en/default/list/list.js.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.js.tmpl,v  <--
 list.js.tmpl
new revision: 1.2; previous revision: 1.1
done

Gerv

Comment 36

[Gervase Markham [:gerv]]

Attachment: Patch B v.2 (security fix)

Comment 37

[[e-mail]]

Nit: if (window.buglistCallback) generates strict JS warnings in scripts without
the callback. Is if (typeof buglistCallback == "function") portable?

Comment 38

[Gervase Markham [:gerv]]

According to the Rhino book, typeof is JS 1.1 (ECMAScript 1.0.) So it should
work in all browsers we support.

Gerv

Comment 39

[Gervase Markham [:gerv]]

neil: if you want to fix the warnings, file another bug and attach a patch based
on "typeof". Get me to review it.

Gerv