Make javascript version of buglists available

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
Query/Bug List
RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: gerv, Assigned: gerv)

Tracking

2.17.3
Bugzilla 2.18
Bug Flags:
approval +

Details

(Whiteboard: [fixed in 2.17.6])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

15 years ago
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
(Assignee)

Comment 1

15 years ago
Created attachment 115979 [details] [diff] [review]
Patch v.1

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

Comment 2

15 years ago
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
Attachment #115979 - Flags: review?(myk)
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.
Attachment #115979 - Flags: review?(myk) → review+
(Assignee)

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 2.18

Comment 4

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

Comment 5

14 years ago
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();
(Assignee)

Comment 6

14 years ago
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
(Assignee)

Comment 7

14 years ago
Created attachment 118967 [details] [diff] [review]
Patch v.2 (initial implementation)

With callback.

Gerv
Attachment #115979 - Attachment is obsolete: true
(Assignee)

Comment 8

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

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

Gerv
Attachment #118967 - Flags: review?(jruderman)

Comment 9

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

Looks ok.
Attachment #118967 - Flags: review?(jruderman) → review+
already got a+, go for it.
(Assignee)

Comment 11

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 12

14 years ago
Created attachment 119087 [details]
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.
(Assignee)

Comment 13

14 years ago
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

14 years ago
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.
(Assignee)

Comment 15

14 years ago
> 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
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.
(Assignee)

Comment 17

14 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
removing approval since this was reopened and there's nothing new to approve yet
Flags: approval+
(Assignee)

Comment 19

14 years ago
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
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.
(Assignee)

Comment 21

14 years ago
Created attachment 131024 [details] [diff] [review]
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
(Assignee)

Updated

14 years ago
Attachment #131024 - Flags: review?(bbaetz)

Comment 22

14 years ago
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

14 years ago
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?
(Assignee)

Comment 24

14 years ago
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
Could you not do the same thing by loading the HTML buglist in a hidden iframe
and using DOM scripting to extract the content?
OK, guess not.  Jesse pointed this out in IRC:
http://www.mozilla.org/projects/security/components/same-origin.html
:)
(Assignee)

Comment 27

14 years ago
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

14 years ago
[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+.
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
(Assignee)

Comment 30

14 years ago
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 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.
Attachment #131024 - Flags: review?(myk)
Attachment #131024 - Flags: review?(bbaetz)
Attachment #131024 - Flags: review+
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
Attachment #131024 - Flags: review?(myk)
Flags: approval+
Whiteboard: [ready for 2.17.6]
Attachment #118967 - Attachment description: Patch v.2 → Patch v.2 (initial implementation)
Attachment #131024 - Attachment description: Patch B v.1 → Patch B v.1 (security fix)
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.
(Assignee)

Comment 34

14 years ago
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
(Assignee)

Comment 35

14 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
(Assignee)

Comment 36

14 years ago
Created attachment 135063 [details] [diff] [review]
Patch B v.2 (security fix)
Attachment #131024 - Attachment is obsolete: true
Attachment #135063 - Flags: review+
Whiteboard: [ready for 2.17.6] → [fixed in 2.17.6]

Comment 37

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

Comment 38

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

Gerv
(Assignee)

Comment 39

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

Gerv
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.