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

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[georgi - hopefully not receiving bugspam]

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.

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: memory corruption

Comment 2

[Daniel Veditz [:dveditz]]

Do you have venkman (the javascript debugger) installed? Just wondering if the corruption was from venkman-service.js, or happened even if not installed.

Comment 3

[georgi - hopefully not receiving bugspam]

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

Comment 4

[georgi - hopefully not receiving bugspam]

similar thunderbird related problem.
in html message:

<iframe src="mailbox://[e-mail]/?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

Comment 5

[georgi - hopefully not receiving bugspam]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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?  :(

Comment 7

[georgi - hopefully not receiving bugspam]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  So the jsd thing we're dying in GC.  Presumably we have a dead object around or something... not sure yet.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[georgi - hopefully not receiving bugspam]

(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

Comment 12

[georgi - hopefully not receiving bugspam]

(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?

Comment 13

[georgi - hopefully not receiving bugspam]

mailbox is Bug 328454

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[timeless]

*** Bug 324573 has been marked as a duplicate of this bug. ***

Comment 16

[:Gijs (he/him)]

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 ?

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[:Gijs (he/him)]

Attachment: [checked in] Patch to change x-jsd to a DenyProtocol

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I'm not a peer for that module.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 279337 [details] [diff] [review]
[checked in] Patch to change x-jsd to a DenyProtocol

r+sr=a=me

Comment 21

[Daniel Veditz [:dveditz]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

Yeah, I got carried away....

Comment 23

[:Gijs (he/him)]

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

Comment 24

[Daniel Veditz [:dveditz]]

I assume the bug remains open to deal with the non-async AsyncOpen?

Comment 25

[:Gijs (he/him)]

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

Comment 26

[:Gijs (he/him)]

Attachment: Patch -w (reviewable)

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.

Comment 27

[:Gijs (he/him)]

Attachment: Patch

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 30

[:Gijs (he/him)]

(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? :-(

Comment 31

[chris hofmann]

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.

Comment 32

[georgi - hopefully not receiving bugspam]

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.

Comment 33

[georgi - hopefully not receiving bugspam]

in addition manually loading x-jsd:help doesn't crash, just whines in js console

Comment 34

[Mike Schroepfer]

Given comment 32 and comment 33 taking off the FF3 blocker list.  Please re-nom if you ahve new data

Comment 35

[:Gijs (he/him)]

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

Comment 36

[Daniel Veditz [:dveditz]]

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

Comment 37

[georgi - hopefully not receiving bugspam]

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

Comment 38

[georgi - hopefully not receiving bugspam]

x-jsd:help invokes user supplied RegExp. do regexp-s allow code execution?

Comment 39

[Alexander Sack]

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.

Comment 40

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 279337 [details] [diff] [review]
[checked in] Patch to change x-jsd to a DenyProtocol

a=caillon for 1.8.0.15

Comment 41

[Christopher Aillon (sabbatical, not receiving bugmail)]

Committed attachment 279337 [details] [diff] [review] to the 1.8.0 branch

Comment 42

[Jesse Ruderman]

Sounds like this was FIXED, but I'm not sure.

Comment 43

[:Gijs (he/him)]

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