If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

eval() in XBL may be dangerous; consider an alternative

RESOLVED FIXED in mozilla1.4final

Status

()

Core
XBL
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: neil@parkwaycc.co.uk)

Tracking

({fixed1.4, fixed1.4.1})

Trunk
mozilla1.4final
x86
Windows 2000
fixed1.4, fixed1.4.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed on branch and trunk])

Attachments

(2 attachments, 1 obsolete attachment)

5.34 KB, patch
jag (Peter Annema)
: review+
(not reading, please use seth@sspitzer.org instead)
: superreview+
(not reading, please use seth@sspitzer.org instead)
: approval1.4+
Details | Diff | Splinter Review
555 bytes, patch
jag (Peter Annema)
: review+
(not reading, please use seth@sspitzer.org instead)
: superreview+
Details | Diff | Splinter Review
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
(Reporter)

Comment 1

15 years ago
Assigning to bryner.
Assignee: hyatt → bryner
(Reporter)

Comment 2

15 years ago
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.
Flags: blocking1.4b?
(Reporter)

Updated

15 years ago
Flags: blocking1.4b? → blocking1.4?

Comment 3

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

Comment 5

15 years ago
belt and braces but not a blocker. If the chrome is already protected from
content then this shouldn't block us.
Flags: blocking1.4? → blocking1.4-
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.
Flags: blocking1.4- → blocking1.4?

Comment 7

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

Comment 8

15 years ago
Neil, didn't you have a patch for this?
(Assignee)

Comment 9

15 years ago
Bug 179050
(Assignee)

Comment 10

15 years ago
Created attachment 123394 [details] [diff] [review]
Proposed patch

Updated for bitrot from bug 179050.
Great! Can we get reviews for that patch etc.?
Attachment #123394 - Flags: review?(varga)

Comment 12

15 years ago
Comment on attachment 123394 [details] [diff] [review]
Proposed patch

r=varga
Attachment #123394 - Flags: review?(varga) → review+

Comment 13

15 years ago
heikki is finding a good super reviewer. we should try and get this in 1.4 final
Flags: blocking1.4? → blocking1.4+
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...
Attachment #123394 - Flags: superreview?(sspitzer)
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.
Attachment #123394 - Flags: superreview?(sspitzer) → superreview-

Comment 17

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

Comment 18

15 years ago
Created attachment 124821 [details] [diff] [review]
Removed bits as requested
Attachment #123394 - Attachment is obsolete: true
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.
Attachment #124821 - Flags: review?(jaggernaut)

Comment 20

15 years ago
Comment on attachment 124821 [details] [diff] [review]
Removed bits as requested

r=jag
Attachment #124821 - Flags: superreview?
Attachment #124821 - Flags: review?(jaggernaut)
Attachment #124821 - Flags: review+
Attachment #124821 - Flags: approval1.4?
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.
Attachment #124821 - Flags: superreview?
Attachment #124821 - Flags: superreview+
Attachment #124821 - Flags: approval1.4?
Attachment #124821 - Flags: approval1.4+
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.
Target Milestone: --- → mozilla1.4final
I'll test and land this patch on the trunk and branch this afternoon.

re-assign to neil, though.
Assignee: bryner → neil.parkwaycc.co.uk
Whiteboard: [sspitzer: will test and land today, 6/9/03]
fixed on trunk and branch.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Keywords: fixed1.4
Resolution: --- → FIXED
Whiteboard: [sspitzer: will test and land today, 6/9/03] → [fixed on branch and trunk]
(Assignee)

Comment 25

15 years ago
Created attachment 125568 [details] [diff] [review]
Extraneous line

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

Comment 26

15 years ago
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.
Attachment #125568 - Flags: superreview?(sspitzer)
Attachment #125568 - Flags: review?(jaggernaut)
Attachment #125568 - Flags: approval1.4?

Updated

15 years ago
Attachment #125568 - Flags: review?(jaggernaut) → review+
(Assignee)

Updated

15 years ago
Blocks: 209656

Comment 27

14 years ago
Comment on attachment 125568 [details] [diff] [review]
Extraneous line

moving approval request forward.
Attachment #125568 - Flags: approval1.4? → approval1.4.x?
Comment on attachment 125568 [details] [diff] [review]
Extraneous line

sr=sspitzer
Attachment #125568 - Flags: superreview?(sspitzer) → superreview+

Updated

14 years ago
Blocks: 216482

Comment 29

14 years ago
does this last bit need to go in 1.4.1?
(Assignee)

Comment 30

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

Updated

14 years ago
Attachment #125568 - Flags: approval1.4.x? → approval1.4.x+
(Assignee)

Updated

14 years ago
Keywords: fixed1.4.1

Updated

14 years ago
Blocks: 224532
Removing confidential flag for bugs fixed over a year ago
Group: security
You need to log in before you can comment on or make changes to this bug.