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

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

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

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

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();

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

With callback. Gerv

Comment on attachment 118967 [details] [diff] [review] Patch v.2 (initial implementation) Jesse: could you just check this is OK? Thanks. Gerv

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

already got a+, go for it.

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

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.

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

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.

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

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

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

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.

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

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

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?

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

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

[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

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.

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

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.

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

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

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

According to the Rhino book, typeof is JS 1.1 (ECMAScript 1.0.) So it should work in all browsers we support. 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