Closed Bug 152701 Opened 22 years ago Closed 22 years ago

More eavesdropping in mailnews...

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.2beta

People

(Reporter: jst, Assigned: security-bugs)

References

Details

(Whiteboard: [ADT2 RTM][sg:blocker])

Attachments

(1 file, 1 obsolete file)

all.js fix
4.93 KB, patch
harishd
: review+
dveditz
: superreview+
roc
: approval+
Details | Diff | Splinter Review 
Same problem that is described in bug 66938 only this time XMLSerializer can be
used to turn a DOM into a string. Same approach applies for this problem, we
just need to write a patch and check it in...
Keywords: nsbeta1+
Whiteboard: [ADT2 RTM]
Target Milestone: --- → mozilla1.0.1
What properties/functions do we need to block in mail messages?
Enabling javascript in mail and news is quite dangerous for probably more
reasons than just sniffing the replies/forwards.
In addition to the properties we already blocked in bug 66938 we need to block
all access to XMLSerializer, i.e. we don't want to even let people create
XMLSerializer objects so we should block access to window.XMLSerializer, and
XMLSerializer.*

Hmm, what about XMLHttpRequest? Heikki, can that also be used to steal data?
Hmm, not sure... send() can take a document as an argument, but you should be
able to open() only to same host. But if you can only open to same host, I don't
see what good you could do with a mail or news server, so I am fine with
disabling XMLHttpRequest in Mail as well.

Btw, JS is/will be disabled in Mail by default so this thing is only to protect
those users who enable it themselves...
removing (+) for re-nomination.
Keywords: nsbeta1+ → nsbeta1
Target Milestone: mozilla1.0.1 → mozilla1.1beta
Johnny, didn't we fix this already?
No, this bug is about blocking Mail from using XMLSerializer at all. I don't
think we've done this. We should figure out some way to whitelist the functions
available to scripts in mail without having to list hundreds of properties in prefs.
Status: NEW → ASSIGNED
I don't know that this was fixed yet. I.e. AFAIC this was *not* fixed yet.
Target Milestone: mozilla1.1beta → mozilla1.2beta
What if we turn completely off JS in the message display pane, but allow authors
to include IFRAMEs in which JS will run (subject to checking the 'allow JS in
mail/news' pref)?
Attached patch Potential fix (obsolete) — — Splinter Review 
I am wondering if this would be the fix, haven't tested yet but will do asap.
Comment on attachment 100942 [details] [diff] [review]
Potential fix

sr=jst
Attachment #100942 - Flags: superreview+
Comment on attachment 100942 [details] [diff] [review]
Potential fix

This did not work :(

When I changed the policies for the leaf properties then it worked (for example
XMLSerializer.serializeToString).

Shouldn't there be a simpler way to "noAccess" the whole object and all its
methods?
Attachment #100942 - Attachment is obsolete: true
Whiteboard: [ADT2 RTM] → [ADT2 RTM][sg:blocker]
Blocks: 84545
We discussed this with mstoltz, and as far as we know there is currently no
wildcard support for property and all its subproperties. That, in addition to
what I have attached here, would be the reasonable solution.

A last resort solution in the short term would be to list all the leaf
properties in the all.js file, but there would need to be around 50 lines (rough
estimate) added to all.js. The file is currently 783 lines so maybe 50 or so new
lines wouldn't make a noticeable different in startup. I'll write this last
resort patch as soon as I have some time from meetings. Please note that this
approach is also fragile in the sense that adding/changing properties will make
them accessible.
Ray, Harish: did I catch all properties? What about listeners? Also, if this is
the approach we take, every time we change the XMLExtras/SOAP interfaces the
changes need to be reflected here.

The patch does not include WSDL changes because it is not built by default; when
that happens those changes would need to be added.
Comment on attachment 102980 [details] [diff] [review]
all.js fix

What about nsIOnReadystatechangeHandler's
 handleEvent? Isn't that scriptable too?
From JS that property is 'undefined'.
Comment on attachment 102980 [details] [diff] [review]
all.js fix

Heikki found out that handleEvent is not scriptable and hence
r=harishd.
Attachment #102980 - Flags: review+
This is on the 1.2 list.  How is it coming along?
Comment on attachment 102980 [details] [diff] [review]
all.js fix

sr=dveditz

Is the 1.0 branch affected by this bug?
Attachment #102980 - Flags: superreview+
Yes. I don't see this as high priority for the branch since JS is disabled by
default in mail, but this seems like a safe fix if it is wanted.
It's not that serious a problem now that JS is off by default in mail, but the
fix is easy and safe so it might be good to take on the branch. nominating.

(does having all these caps entries slow us down?)
Keywords: adt1.0.2
I don't have any performance data to back this up, but I think the slowdown is
unnoticeable.

I have emailed ADT and drivers for approvals to trunk & branch.
Not vulnerable by default, adt- per triage meeting
Keywords: adt1.0.2 → adt1.0.2-
Comment on attachment 102980 [details] [diff] [review]
all.js fix

This is good for the 1.0 branch too -- a=brendan@mozilla.org on behalf of
drivers.

/be
Fixed on trunk & branch.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Keywords: fixed1.0.2
Resolution: --- → FIXED
Verified on 2002-11-20-branch on Win 2000.

Ran the test cases under
http://slip.mcom.com/projects/security/XMLExtras-Acceptance/xmlextras-tests.html

and the test cases passed.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.2 → verified1.0.2
Group: security
Summary: More eavsdropping in mailnews... → More eavesdropping in mailnews...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: