memory corruption in mozilla <object data='x-jsd:help'>

RESOLVED FIXED

Status

defect
--
critical
RESOLVED FIXED
14 years ago
10 months ago

People

(Reporter: guninski, Assigned: Gijs)

Tracking

({fixed1.8.0.15, fixed1.8.1.8})

Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:investigate] requires js debugger installed.)

Attachments

(3 attachments, 1 obsolete attachment)

there is a memory corruption in mozilla <object data='x-jsd:help'>
Mozilla 1.7.12
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20060202

triggered by <object data='x-jsd:help'>
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1218234688 (LWP 26508)]
0xb75c0de3 in XPTC_InvokeByIndex () from /opt/mozilla-nightly/libxpcom.so
(gdb) x/i $eip
0xb75c0de3 <XPTC_InvokeByIndex+39>:     call   *(%edx)
(gdb) p/x $edx
$1 = 0x10
(gdb) 

crash place/$eip seems to depend on previous actions.

win32 on wine also crashes.

testcase to follow.
Do you have venkman (the javascript debugger) installed? Just wondering if the corruption was from venkman-service.js, or happened even if not installed.
(In reply to comment #2)
> Do you have venkman (the javascript debugger) installed? Just wondering if the
> corruption was from venkman-service.js, or happened even if not installed.
> 

yes, have venkman jsd - default nightly install.
similar thunderbird related problem.
in html message:

<iframe src="mailbox://a@none.tld/?create>f00kb1l"></iframe>

(gdb) x/i $eip
0xb76db6ba <_ZN18nsSimpleCharStringC1ERKS_+26>: mov    (%edx),%edx
(gdb) p/x $edx
$1 = 0x4
(gdb) info stack
#0  0xb76db6ba in nsSimpleCharString::nsSimpleCharString ()
   from /opt/thunder-nightly/libxpcom_compat.so
#1  0xb76ded8a in nsFileSpec::nsFileSpec ()
   from /opt/thunder-nightly/libxpcom_compat.so
#2  0xb76e255e in nsFileSpecImpl::nsFileSpecImpl ()
   from /opt/thunder-nightly/libxpcom_compat.so
#3  0xb76e2654 in nsFileSpecImpl::MakeInterface ()
   from /opt/thunder-nightly/libxpcom_compat.so
#4  0xb76e4daf in NS_NewFileSpecWithSpec ()
   from /opt/thunder-nightly/libxpcom_compat.so
#5  0x08845523 in non-virtual thunk to nsRandomAccessInputStream::set_at_eof(int) ()

in case the stack is correct, nsFileSpecImpl::MakeInterface seems dangerous

i mean why at all the mailbox protocol tries to be loaded?
> i mean why at all the mailbox protocol tries to be loaded?

Because it's just a protocol.  Same for mailto:, etc.  Known issue.

Is this a problem on trunk?  Or would I need to debug on the 1.7 branch?  :(
(In reply to comment #6)
> > i mean why at all the mailbox protocol tries to be loaded?
> 
> Because it's just a protocol.  Same for mailto:, etc.  Known issue.
> 

i believe the security manager is supposed to block the mailbox protocol the same way for "file:" ?

iirc there have been cases in the past where actions were performed on user's mailbox via specific mailbox urls.


> Is this a problem on trunk?  Or would I need to debug on the 1.7 branch?  :(
> 

looks like the mailbox problem (must be in mail message) affects both branch and trunk (thunderbird and seamonkey).

the jsd seems branch only, though not quite sure.

> i believe the security manager is supposed to block the mailbox protocol

See bug 305565.  It's fixed in 1.8, but apparently not in 1.7.  And even then, it only disallows linking from other protocols, not from mailbox:// itself, no?

> looks like the mailbox problem (must be in mail message) affects both branch
> and trunk (thunderbird and seamonkey).

Probably worth a separate bug on mailnews (cc me, mscott).

> the jsd seems branch only, though not quite sure.

OK.  :(
OK.  So the jsd thing we're dying in GC.  Presumably we have a dead object around or something... not sure yet.
OK, so one obvious problem is that the asyncOpen impl for JSDChannel is ... not async.  That would certainly cause various bustage....

I suspect that fixing that will fix this bug, but someone would need to actually post a jsd patch, I guess.

I also think we should:

1)  Make it so only chrome can link to x-jsd (in the security manager)
2)  Make it a priority for 1.9 (or even 1.8.1?) to let us ask protocol handlers
    for such information.
Assignee: general → rginda
Component: General → JavaScript Debugger
Product: Mozilla Application Suite → Other Applications
QA Contact: general → caillon
(In reply to comment #8)
> > i believe the security manager is supposed to block the mailbox protocol
> 
> See bug 305565.  It's fixed in 1.8, but apparently not in 1.7.  And even then,
> it only disallows linking from other protocols, not from mailbox:// itself, no?

can't see it.

> 
> Probably worth a separate bug on mailnews (cc me, mscott).
> 

will file a new bug, there are several strangenesses in mailbox, so probably the solution is to disable mailnews protocol.

iirc looking at code, there was a whitelist of safe internal protocol so either i am wrong or it is broken

(In reply to comment #10)
> OK, so one obvious problem is that the asyncOpen impl for JSDChannel is ... not
> async.  That would certainly cause various bustage....
> 

can you comment on the exploitability of it - is it provable null dereference?
> can't see it.

I cced you.

> can you comment on the exploitability of it - is it provable null dereference?

It's more likely to be what you ran into -- a deleted object reference.  :(

*** Bug 324573 has been marked as a duplicate of this bug. ***
To my knowledge, the 1.7 branch has been officially EOLed by now. bz, does Venkman still need something to do with this, or do we mark this invalid/fixed/something-else ?
> bz, does Venkman still need something to do with this

Yes.  This still gives memory corruption in 1.8 builds.  And Venkman is violating the AsyncOpen contract: see comment 10.

On trunk untrusted content can't link to x-jsd URLs, but chrome and external apps can still do it (and then will get this sort of corruption).

So in brief:

1)  On the 1.8 branch you need to change CheckLoadURI to make this a DenyProtocol.
2)  You need to fix your broken AsyncOpen impl.
Severity: normal → critical
Version: 1.7 Branch → Trunk
Not sure who I should ask for superreview (do I need to?).

Either way, r? on you, bz :-).

I'll look at the necessary venkman changes in a bit - probably not going to be as easy.
Assignee: rginda → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #279337 - Flags: review?(bzbarsky)
Attachment #279337 - Flags: approval1.8.1.7?
I'm not a peer for that module.
Comment on attachment 279337 [details] [diff] [review]
[checked in] Patch to change x-jsd to a DenyProtocol

r+sr=a=me
Attachment #279337 - Flags: superreview+
Attachment #279337 - Flags: review?(bzbarsky)
Attachment #279337 - Flags: review+
Attachment #279337 - Flags: approval1.9+
Comment on attachment 279337 [details] [diff] [review]
[checked in] Patch to change x-jsd to a DenyProtocol

approval1.9=bz, really? This wouldn't apply to the trunk, and this aspect appears to be handled on trunk already: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/venkman/js/venkman-service.js&rev=1.16&mark=284#277

approved for 1.8.1.7, a=dveditz
Attachment #279337 - Flags: approval1.9+
Attachment #279337 - Flags: approval1.8.1.7?
Attachment #279337 - Flags: approval1.8.1.7+
Yeah, I got carried away....
Checking in mozilla/caps/src/nsScriptSecurityManager.cpp;
/cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v  <--  nsScriptSecurityManager.cpp
new revision: 1.266.2.19; previous revision: 1.266.2.18
done
Keywords: fixed1.8.1.7
Attachment #279337 - Attachment description: Patch to change x-jsd to a DenyProtocol → [checked in] Patch to change x-jsd to a DenyProtocol
Attachment #279337 - Attachment is obsolete: true
I assume the bug remains open to deal with the non-async AsyncOpen?
Whiteboard: [sg:critical?] requires js debugger installed.
(In reply to comment #24)
> I assume the bug remains open to deal with the non-async AsyncOpen?
> 

Yes. I have a half-finished patch somewhere on my machine, it's a hard problem, I'm busy, and it's not exceptionally high on my priority list. If someone wants to take it and thinks they'll fix it within the next month or so, they probably should.
This should do, I believe. It's not very pretty, but to be honest I couldn't quite figure out how to do this Better.

The only question I have left is whether the callback timer should be calling onStopRequest immediately. I'm not sure how the interaction with the content handler defined later works here, so I was a bit cautious to touch any of it. AFAICT calling onStopRequest from the content handler is Hard as we don't know which channel invoked it, and may therefor not be able to figure out the context for that call. But maybe I'm missing something.


Also, my lack of understanding of the aforementioned interaction between the JSDChannel and JSDContentHandler stopped me from using an input stream pump for that case as well... If that's possible, I think it's probably still a better solution than this nsITimer hackery.

r? on a netwerk peer as they seem most likely to be able to figure out what the heck is *supposed* to happen. I'll request review from jsd/venkman people afterwards.
Attachment #291766 - Flags: review?(cbiesinger)
Posted patch PatchSplinter Review
Comment on attachment 291766 [details] [diff] [review]
Patch -w (reviewable)

-        !iid.equals(nsISupports))
+        !iid.equals(nsISupports) && !iid.equals(nsIRequestObserver) &&
+        !iid.equals(nsIStreamListener))

I'd have kept nsISupports at the end, but it's your module :)

+    this.streamListener.onDataAvailable(this, this.context, this.stringStream,
+                                        offset, count);

that looks somewhat strange and I'm not sure it will necessarily work correctly. just pass on inputstream here instead of this.stringStream.

+OpenVnkListener.prototype.QueryInterface =

you don't need this, though I guess it doesn't harm to have it

+    //XXXgijs: call onStopRequest here?

yes. you must do that.

and you should also remove the channel from the loadgroup or the throbber will never stop.

+    // Null out to stop leaks:

if it's necessary here to prevent leaks, shouldn't jsdch_onstoprequest also do it?

is there a reason that you're naming the context this.myContext here while it's this.context in JSDChannel? prefixing properties of this with "my" seems redundant.
Attachment #291766 - Flags: review?(cbiesinger) → review+
and fwiw, you can't call onStopRequest from the content handler. you can't be sure that it was an x-jsd channel that triggered it.
(In reply to comment #29)
> and fwiw, you can't call onStopRequest from the content handler. you can't be
> sure that it was an x-jsd channel that triggered it.
> 

Yeah, that's exactly what I was struggling with. I'm not sure how the handoff from the JSDChannel to the content handler works, exactly, and whether or not I could get away with just calling onStopRequest immediately after calling onStartRequest. I would guess not, but if that's not possible, then where else am I supposed to do it? :-(
anyone know if this is a problem on the current trunk?  does a patch need to land there?  marking as a blocker for 1.9 until we know fore sure.  go ahead and minus if this is branch only.
Flags: blocking1.9?
trunk and 1.1.7 seem safe.
x-jsd: doesn't load at all - this seems sane:
Security Error: Content at http://localhost:8080/bugjsd.html may not load or link to x-jsd:help.
in addition manually loading x-jsd:help doesn't crash, just whines in js console
Given comment 32 and comment 33 taking off the FF3 blocker list.  Please re-nom if you ahve new data
Flags: blocking1.9? → blocking1.9-
biesi: could you give some advice re: comment #30? That way I can come up with an updated patch to get this out of the world. :-)
Looks like this is no longer critical because the protocol handler is not exposed to content. How serious is the remaining problem (in a security sense)?
Whiteboard: [sg:critical?] requires js debugger installed. → [sg:investigate] requires js debugger installed.
i suppose this bug is fixed not counting disabling the protocol.

one way to reproduce something similar to it now is to trick the user to paste in location bar "x-jsd:help" - it displays debugger help if debugger is running
x-jsd:help invokes user supplied RegExp. do regexp-s allow code execution?
Comment on attachment 279337 [details] [diff] [review]
[checked in] Patch to change x-jsd to a DenyProtocol

we have this in distro patches. requesting approval for 1.8.0.15.
Attachment #279337 - Flags: approval1.8.0.15?
Comment on attachment 279337 [details] [diff] [review]
[checked in] Patch to change x-jsd to a DenyProtocol

a=caillon for 1.8.0.15
Attachment #279337 - Flags: approval1.8.0.15? → approval1.8.0.15+
QA Contact: caillon → venkman
Sounds like this was FIXED, but I'm not sure.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Group: core-security
(In reply to comment #42)
> Sounds like this was FIXED, but I'm not sure.

Technically, the asyncopen contract is still violated by the current cmdline handler, I think. :-(
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.