1068897 - functions of CPOW'd objects that return a pointer instead of nsresult don't appear to work

Status: RESOLVED DUPLICATE of bug 1069518

(Core :: XPConnect, defect)

Description

[Dana Keeler (she/her) [:keeler]]

I'm not exactly sure where the bug is here, but this is the setup:

nsIX509Cert defines the following:

%{ C++
 /* forward declaration */
 typedef struct CERTCertificateStr CERTCertificate;
%}
[ptr] native CERTCertificatePtr(CERTCertificate);

...

[notxpcom, noscript] CERTCertificatePtr getCert();

There are two classes that implement nsIX509Cert: nsNSSCertificate and nsNSSCertificateFakeTransport. The latter is used in child processes so we don't have to initialize NSS. It basically returns NS_ERROR_NOT_IMPLEMENTED for most of its implementation. In the case of GetCert, it returns nullptr. However, if the parent process receives a CPOW around a nsNSSCertificateFakeTransport in the child process and calls GetCert, nullptr is not returned: NS_ERROR_FAILURE is. This is then interpreted as a pointer and dereferenced, which is not what we want.

After single-stepping through the execution, it appears that PrepareAndDispatch xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp needs to do something slightly different, hence filing this in the XPCOM component. However, maybe we just need to not call such functions on CPOW'd objects.

I think this is potentially exploitable, so marking security-sensitive.

Comment 1

[Dana Keeler (she/her) [:keeler]]

Attachment: xpcshell test that exposes the bug

This demonstrates the issue. Apply the patch, build, and then run with `./mach xpcshell-test security/manager/ssl/tests/unit/test_cert_roundtrip.js'

Comment 2

[Dana Keeler (she/her) [:keeler]]

I reproduced this in 33, so I don't think it's a regression. Note that this specific issue really only affects e10s (b2g currently handles certificate overrides slightly differently, and I don't think it runs into this). However, if there are any other similar interfaces, this could be a problem.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Uh... calling noscript stuff from script should throw, right?

Comment 4

[Dana Keeler (she/her) [:keeler]]

I assume so, but this is an indirect call. The script calls nsICertOverrideService.rememberValidityOverride and passes in the CPOW'd object. The native implementation then calls the noscript function.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This code is pretty foreign to me, but the message manager part doesn't seem to use CPOWs. To get a CPOW, you would have to use an extra argument on lines 29 and 66, and you would have to retrieve the object via message.objects rather than message.data. The normal message.data stuff is sent via structured clone. Do we have any kind of special handling of certificates in the structured clone code? Could that be involved here?

Comment 6

[Dana Keeler (she/her) [:keeler]]

Hmmm - maybe the example needs fixing. In any case, you can reproduce the issue using the patches in bug 1055238 (which fix another issue that prevents this from being encountered) and (in an e10s tab), visiting https://example.com (or any other site with a certificate error), and attempting to add a certificate exception.

Comment 7

[Benjamin Smedberg]

The problem here seems to be that we're implementing nsIX509Cert in JS when we clearly can't (because some of the methods are notxpcom). I think in this case we should just make xpconnect refuse to implement any interface with a [notxpcom] method descriptor.

We can't fix this in xptcstubs.

Comment 8

[Benjamin Smedberg]

Or we could do this checking in NS_GetXPTCallStub, although that might get expensive. I'll try to make a prototype of that patch for somebody to try.

Comment 9

[Benjamin Smedberg]

Moved the xptcall experiment into bug 1069518.

Comment 10

[Benjamin Smedberg]

keeler, bug 1069518 should have fixed this crash by throwing instead of implementing nsINSSCertificate. Could you verify?

Comment 11

[Dana Keeler (she/her) [:keeler]]

Yep - that fixed it. Thanks!