From Georgi Guninski: This may be dangerous: "./xpfe/global/resources/content/bindings/textbox.xml" line 132 of 200 --66%-- c ol 13 --------------- <handler event="keypress"> <![CDATA[ if (event.keyCode == 13) { if (this._timer) clearTimeout(this._timer); eval(this.callback); --------------- In general, we should try to eliminate eval() from the chrome whenever possible; it's slow and can lead to serious security problems. There's probably an alternative in this case. - Mitch

Assigning to bryner.

Waldemar, could you work on trying to replace eval() calls in chrome JS with safer alternatives (particularly the case mentioned above). Let's try to do this by 1.4b if we can.

Could an attacker created one of these textboxes outside of content? Or is this just a bad api that could be dangerous for some other app or extension?

asa: I don't have working exploit, but the potential problem is injecting js into chrome via eval()

belt and braces but not a blocker. If the chrome is already protected from content then this shouldn't block us.

I don't have a patch, but a general idea for how we could fix this is to have the consumer of <textbox> set the callback function as a JS property, rather than using an attribute to store text to be eval()'d.

I remember seeing a patch for this somewhere doing what bryner proposes.

Neil, didn't you have a patch for this?

Updated for bitrot from bug 179050.

Great! Can we get reviews for that patch etc.?

Comment on attachment 123394 [details] [diff] [review] Proposed patch r=varga

heikki is finding a good super reviewer. we should try and get this in 1.4 final

Comment on attachment 123394 [details] [diff] [review] Proposed patch Most of the changes are in files that Seth has previously sr'd so asking from him first. Jag might be the best person to look over the textbook.xml file if you feel unsure...

is this the minimal patch to avoid using eval() in XBL? it doesn't look like it, and this late in 1.4, I'd rather see the minimal patch for 1.4, and the full patch (with the extra cleanup) on the trunk.

Comment on attachment 123394 [details] [diff] [review] Proposed patch this patch from neil seems like something we'd want on the trunk, for 1.5 alpha. for 1.4 final, let's just take a minimal patch to address the eval issue.

Comment on attachment 123394 [details] [diff] [review] Proposed patch This patch also converts other code to use the timed textbox instead of rolling their own. If you ignore that part of the patch then the only question I have is why the patch makes it so that hitting enter selects the text you just typed?

Comment on attachment 124821 [details] [diff] [review] Removed bits as requested seeing review from jag. this is looking pretty minimal. once we get r=jag, I can sr.

Comment on attachment 124821 [details] [diff] [review] Removed bits as requested r=jag

Comment on attachment 124821 [details] [diff] [review] Removed bits as requested sr=sspitzer this looks pretty isolated, so assuming that neil tested these two instances, let's get this on trunk and branch. but we should let QA (chris peterson, for bookmarks) know, so he can keep an eye out for regressions.

should we re-assign to neil.parkwaycc.co.uk@myrealbox.com? since it blocks 1.4, let's set TM to 1.4 cc petersen@netscape.com, so he can look out for any bookmark regression on these timed text boxes. fyi, for netscape folks: I checked the ns tree, and there aren't any timed text boxes, so we are ok.

I'll test and land this patch on the trunk and branch this afternoon. re-assign to neil, though.

fixed on trunk and branch.

Jan's just spotted an extraneous line - I thought I had removed it; maybe I copied from the wrong patch from bug 179050. The code path isn't used for 1.4, but it would be nice to have consistent behaviour when correctly fixing bug 179050.

Comment on attachment 125568 [details] [diff] [review] Extraneous line Patch to remove extraneous line from unused code to sync with correct patch for bug 179050.

Comment on attachment 125568 [details] [diff] [review] Extraneous line moving approval request forward.

Comment on attachment 125568 [details] [diff] [review] Extraneous line sr=sspitzer

does this last bit need to go in 1.4.1?

Well, it's code that's not used by 1.4 but if it was used it would crash :-) (see bug 216482 for a firebird crash using that code)

Removing confidential flag for bugs fixed over a year ago