561244 - Remove window.crypto for e10s

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jason Duell]

We're turning off NSS in the content process.  We thus need to forward any calls to 
  
  https://developer.mozilla.org/en/JavaScript_crypto

to chrome.  

This is needed for fennec 2.0, so must land by mid-June.  Any takers?

Comment 1

[Josh Matthews [:jdm]]

-> Me

Comment 2

[Josh Matthews [:jdm]]

Attachment: WIP

This is completely untested, but theoretically complete.  I'd like an executive decision as to whether it's worth splitting nsCrypto into separate chrome and content objects, or whether I can leave it as the #ifdef/conditional setup I have right now.  Any other general comments appreciated/what's the best way to test whether this still works?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

So when for example signText is called, it opens a modal dialog, right?
Content process would then send a sync message to chrome which opens a dialog.
This would mean that anything in the content process doesn't get painted while
the dialog is open in chrome.

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 441447 [details] [diff] [review]
WIP

Kai, same feedback request from comment 2 applies.

Comment 5

[Kai Engert (:KaiE:)]

I understand e10s is separate-process-per-tab. This patch doesn't appear to be complete, for example I see no caller to RecvSetEnableSmartCardEvents. 


(In reply to comment #2)
> 
> This is completely untested, but theoretically complete.

If you want to change things, I'll request that you do full regression testing of all touched features.


> I'd like an executive
> decision as to whether it's worth splitting nsCrypto into separate chrome and
> content objects, or whether I can leave it as the #ifdef/conditional setup I
> have right now.  

I'm not picky on this detail, making sure it still works is more important.


> Any other general comments appreciated/what's the best way to
> test whether this still works?

You need special setup.

For smartcards, you need to have a smartcard that's configured to work with encryption. Receive an encrypted smartcard. Go to thunderbird. Make sure that inserting a smartcard will trigger decrypting of a message.

For CRMF request, the ultimate test is to test against a certificate system (such as dogtag) that uses this request to enroll for certificates. You might be able to test using a simple test case that I created on kuix.de, we could dig it up. It is mentioned in some bug around crmf.

Importing user certificates? That's probably when you have enrolled for a certificate via web, and when it's ready, you pick up the public part of your new user certificate from a web download. You could test this using Verisign's testdrive enrollment available at https://digitalid.verisign.com/client/class1Netscape.htm

You probably also must handle the <keygen> code that's also seen in Verisign's enrollment. The content processing of <keygen> tag triggers NSS to create a new public/private key pair.

Comment 6

[Kai Engert (:KaiE:)]

PSM has too few tests, this is an area were we seriously need help. If you want to change the core of PSM, please add new tests for the functionality you touch.

Comment 7

[Kai Engert (:KaiE:)]

jdm, so you're asking, is it find to have the initial-forwarding-code in the body of each existing function, or do you need a separate class for the forwarding stubs? IMHO, if it's limited to such an initial block in each forwarder, which exits after forwarding and doesn't interact with the remainder of the function, I'm absolutely find with having the forwarder inside the same function (for simplicity)

Comment 8

[Kai Engert (:KaiE:)]

s/find/fine/

Comment 9

[Jason Duell]

This blocks turning off NSS in content, not vice versa, unless I'm missing something.

Comment 10

[Jason Duell]

Note that we can implement this in parts:  fennec isn't going to need smartcard support, so we can leave that for a spinoff bug.  Right now the entire JS crypto interface is unsupported from content processes under e10s, so any patches that implement features should probably be considered progress.

Comment 11

[Josh Matthews [:jdm]]

I'm focusing on other bugs right now, so I won't get upset if somebody else wants to run with this.

Comment 12

[Honza Bambas (:mayhemer)]

I'll probably take this bug.

Comment 13

[Honza Bambas (:mayhemer)]

-> me

Comment 14

[Jeremias Bosch (:jbos)]

Hey,
i just got a first look on that wip. actually i'm not 100% sure whats holding it up for fennec. Is it about missing tests? How should we proceed here?

Comment 15

[Doug Turner (:dougt)]

tests+try server, then we can get a review request going.  jeremias, could you also take a first pass through the code for correctness?

Comment 16

[Josh Matthews [:jdm]]

One major issue: nobody ever checked to ensure that the changes worked correctly.

Comment 17

[Doug Turner (:dougt)]

part of tests!

Comment 18

[Jeremias Bosch (:jbos)]

Attachment: crypto_patch V2

Updated version of the patch for latest upstream

Comment 19

[Jeremias Bosch (:jbos)]

Attachment: crypto_patch V3

Found a crash while manual testing with the script found on  https://developer.mozilla.org/en/javascript_crypto

mRemote wasn't initialized to nsnull. thats fixed with V3

If there are more tests around please point me to it.

Comment 20

[Josh Matthews [:jdm]]

See comment 5 for more suggestions for tests.

Comment 21

[Jeremias Bosch (:jbos)]

Thanks
i checked out i.e. http://kuix.de/misc/test3/crmf-rsa.php which is generating nothing in e10s at all, even with the patch.

It ends up with NS_ERROR_NOT_AVAILABLE because xpc->GetCurrentNativeCallContext(&ncc); is putting ncc to be 0.

Is something like that a known issue? How to handle that?

Comment 22

[Jeremias Bosch (:jbos)]

The callstack:



#0  nsXPConnect::GetCurrentNativeCallContext (this=0xb7b56f20, aCurrentNativeCallContext=0xbfff86d0)
    at /scratchbox/users/jbos/home/jbos/mozilla-central/js/src/xpconnect/src/nsXPConnect.cpp:1824
#1  0xb5fb73d8 in nsCrypto::GenerateCRMFRequest (this=0xaaefe9a0, aReturn=0xbfff8794)
    at /scratchbox/users/jbos/home/jbos/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:1854
#2  0xb6200ab2 in mozilla::dom::TabParent::RecvGenerateCRMFRequest (this=0xb623ef2a, rv=0xbfff884c, request=0xbfff8810)
    at /scratchbox/users/jbos/home/jbos/mozilla-central/dom/ipc/TabParent.cpp:593
#3  0xb623ef2a in mozilla::dom::PBrowserParent::OnMessageReceived (this=0xaae51830, __msg=..., __reply=@0xbfff89bc) at PBrowserParent.cpp:1107
#4  0xb62453c0 in mozilla::dom::PContentParent::OnMessageReceived (this=0xb7b2c280, __msg=..., __reply=@0xbfff89bc) at PContentParent.cpp:408
#5  0xb6223b99 in mozilla::ipc::SyncChannel::OnDispatchMessage (this=0xb7b2c288, msg=...) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/glue/SyncChannel.cpp:169
#6  0xb6220140 in mozilla::ipc::RPCChannel::OnMaybeDequeueOne (this=0xb7b2c288) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/glue/RPCChannel.cpp:436
#7  0xb6220b52 in DispatchToMethod<mozilla::ipc::RPCChannel, bool (mozilla::ipc::RPCChannel::*)()> (arg=<value optimized out>, method=<value optimized out>,
    obj=<value optimized out>) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/tuple.h:383
#8  RunnableMethod<mozilla::ipc::RPCChannel, bool (mozilla::ipc::RPCChannel::*)(), Tuple0>::Run (arg=<value optimized out>, method=<value optimized out>,
    obj=<value optimized out>) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/task.h:307
#9  0xb6220bbb in Run (this=<value optimized out>) at ../../dist/include/mozilla/ipc/RPCChannel.h:448
#10 mozilla::ipc::RPCChannel::DequeueTask::Run (this=<value optimized out>) at ../../dist/include/mozilla/ipc/RPCChannel.h:473
#11 0xb638b18a in MessageLoop::RunTask (this=0xb7b1b980, task=0xaae1eee0) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:339
#12 0xb638b562 in MessageLoop::DeferOrRunPendingTask (this=0x0, pending_task=...)
    at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:347
#13 0xb638b7d5 in MessageLoop::DoWork (this=0xb7b1b980) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:447
#14 0xb621ddc7 in mozilla::ipc::MessagePump::Run (this=0xb7ba46a0, aDelegate=0xb7b1b980) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/glue/MessagePump.cpp:122
#15 0xb638bb01 in MessageLoop::RunInternal (this=0xb7b1b980) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:219
#16 0xb638bbb0 in RunHandler (this=0xb7b1b980) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:202
#17 MessageLoop::Run (this=0xb7b1b980) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:176
#18 0xb612996e in nsBaseAppShell::Run (this=0xb17f6600) at /scratchbox/users/jbos/home/jbos/mozilla-central/widget/src/xpwidgets/nsBaseAppShell.cpp:175
#19 0xb5f69304 in nsAppStartup::Run (this=0xb14d2760) at /scratchbox/users/jbos/home/jbos/mozilla-central/toolkit/components/startup/src/nsAppStartup.cpp:191
#20 0xb54b4ddf in XRE_main (argc=1, argv=0xbfffe334, aAppData=0xb7b1fb40) at /scratchbox/users/jbos/home/jbos/mozilla-central/toolkit/xre/nsAppRunner.cpp:3630
#21 0x08049ff8 in main (argc=1, argv=0xbfffe334) at /scratchbox/users/jbos/home/jbos/mozilla-central/xulrunner/stub/nsXULStub.cpp:583

Comment 23

[Jeremias Bosch (:jbos)]

Ok I looked more into that and its getting complicated.

The issue is, that the context is in the content process but the work done is in the chrome. Basically I think whats need to be done is:

The parsing of the parameters must happen within in the content process, giving them into the chrome and doing there all the crypto work. Since i.e. for GenerateCRMFRequest there can also be a callback back into the javascript this must happen again within the content process.

Whats the best way to do that?

Comment 24

[Jason Duell]

> Whats the best way to do that?

When we need to go back to content from chrome, we'll need to send more IPDL traffic.  Chrome can't block, so the messages need to be async.  For dialogs, we have one case where we've handled that already (HTTP auth: see the patch for bug 537782), so maybe you can get what you need from there.

Round-trip IPDL traffic is expensive time-wise, so if there's a way to avoid it, we should do it.  For instance, if the callbacks in GenerateCRMFRequest are just to grab values from the JS engine, we should grab those earlier if possible, and send them as part of the initial IPDL msg from content->chrome, and save an IPDL round trip.

Comment 25

[Josh Matthews [:jdm]]

It looks to me like GenerateCRMFRequest returns a solitary object:

>  nsCRMFObject *newObject = new nsCRMFObject();
>  if (newObject == nsnull) {
>    JS_ReportError(cx, "%s%s\n", JS_ERROR, "could not create crmf JS object");
>
>    nsFreeKeyPairInfo(keyids,numRequests);
>    return NS_ERROR_OUT_OF_MEMORY;
>  }
>  newObject->SetCRMFRequest(encodedRequest);
>  *aReturn = newObject;

If the chrome does the crypto generation and passes back the encodedRequest, which is just a string, then it should be easy to return the proper object.  For the callback, you should have everything but the principals, which are serializable which gives me hope.  dwitte or somebody else certainly knows more about those than I do, however.

You should be able to factor out the bit starting after the xpc->WrapNative into a separate function that could run in chrome with some help.  Note that cryptojs_ReadArgsAndGenerateKey just turns the js args into things like strings and then calls cryptojs_generateOneKeyPair which doesn't actually do anything with the JS context it receives.  This gives me hope that you can just send some strings over the wire synchronously, do some work and just get back the final encoded value.

But man, this is not fun code to read.

Comment 26

[Jeremias Bosch (:jbos)]

Attachment: Removes Crypto Object from nsGlobalWindow

While doing this patch I was realizing that its way more complicated than I thought. The goal - which is getting rid of the nss lib in the child process isn't easily done by just ripping the object apart, basically to achieve that you need two classes, one in xul and one in the nss. its also not just ripping the function in two parts, this cut goes way deeper and you need carefully decide which parts can end up in the one and which in the other side. 
Since i have no Idea about the internals of the nss lib and not the time to get into them and because its just a fennec alpha blocker because of the child process loading the nss lib, i propose this patch to just remove the crypto object from global window and returning NS_ERROR_NOT_IMPLEMENTED for now.

We should take care about bringing the crypto back in another bug, to a later time.

Comment 27

[Doug Turner (:dougt)]

Comment on attachment 459912 [details] [diff] [review]
Removes Crypto Object from nsGlobalWindow

jst - can you review?  I do not think anyone on mobile is going to cry to long about not having access to the crypto object in 2.0.

Comment 28

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/2335c842c06c

marking fixed.

Bug 582297 can track bring crypto back.

Comment 29

[Doug Turner (:dougt)]

test_windowProperties.html needs window.crypto

also.  MOZ_IPC is probably the wrong #define because firefox desktop has it set.  backing this out for now.

Comment 30

[timeless]

sorry, removing window.crypto is not ok. it's used by the web.

please note that the n900 and potential successors want to be able to do certificate enrollment in enterprise environments.

for information about window.crypto, you can read:
https://developer.mozilla.org/en/JavaScript_crypto

Comment 31

[Doug Turner (:dougt)]

timeless, patches accepted.

Comment 32

[timeless]

fwiw, we have an activclient smart card reader + the software for it here. i specifically acquired it in order to be able to deal w/ this stuff.

Comment 33

[Doug Turner (:dougt)]

Comment on attachment 459912 [details] [diff] [review]
Removes Crypto Object from nsGlobalWindow

MOZ_IPC is enabled also in FF.  :(

Comment 34

[Benjamin Smedberg]

Technically this doesn't block Firefox, though it does block fennec. I think that means it doesn't need the blocking2.0 flag.

Comment 35

[Mike Kristoffersen (:MikeK)]

Attachment: Part 1/3 - Adding uniq define to disable crypto object

Comment 36

[Mike Kristoffersen (:MikeK)]

Attachment: Part 2/3 - Making it possible to control the define by options

Comment 37

[Mike Kristoffersen (:MikeK)]

Attachment: Part 3/3 - Setting the default behavior for Fennec to have the crypto object disabled

Comment 38

[Doug Turner (:dougt)]

Comment on attachment 478093 [details] [diff] [review]
Part 1/3 - Adding uniq define to disable crypto object

jst, similar to the last patch, but using a better #define.

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 478093 [details] [diff] [review]
Part 1/3 - Adding uniq define to disable crypto object

- In nsGlobalWindow::GetCrypto(nsIDOMCrypto** aCrypto):

+#ifdef MOZ_DISABLE_DOMCRYPTO
+    return NS_ERROR_NOT_IMPLEMENTED;
+#else

Use 2-space indentation here.

r=jst

Comment 40

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 478094 [details] [diff] [review]
Part 2/3 - Making it possible to control the define by options

Does this really need to be an actual configure option?  Isn't having it exposed to an app's confvars enough?

Also, lose the AC_SUBST.

Comment 41

[Mike Kristoffersen (:MikeK)]

(In reply to comment #40)
> Comment on attachment 478094 [details] [diff] [review]
> Part 2/3 - Making it possible to control the define by options
> 
> Does this really need to be an actual configure option?  Isn't having it
> exposed to an app's confvars enough?

What does it hurt to have it as a configure option?

> 
> Also, lose the AC_SUBST.

ok

Comment 42

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We don't like adding configure options that aren't necessary.  They just cause trouble when somebody decides to set a bunch of bizarre ones and then comes on #developers wondering why their build is broke.

Comment 43

[Mike Kristoffersen (:MikeK)]

(In reply to comment #42)
> We don't like adding configure options that aren't necessary.  They just cause
> trouble when somebody decides to set a bunch of bizarre ones and then comes on
> #developers wondering why their build is broke.

When I finish biting my tongue I'll remove it...

Comment 44

[Mike Kristoffersen (:MikeK)]

(In reply to comment #43)
> When I finish biting my tongue I'll remove it...

btw, that comment was about process and group behavior, not about Kyle, he is doing the right thing by pointing this out to me, and as I said, I'll update it :)

Comment 45

[Mike Kristoffersen (:MikeK)]

Attachment: Part 1/3 - Adding uniq define to disable crypto object (v2 - updated after review)

Comment 46

[Doug Turner (:dougt)]

Comment on attachment 479425 [details] [diff] [review]
Part 1/3 - Adding uniq define to disable crypto object (v2 - updated after review)

basically the same as the patch jst reviewed -- the difference here is one ws line @ 3097.

Comment 47

[Doug Turner (:dougt)]

mikek - do we just need the patches named something 1/3 and 3/3?

Comment 48

[Mike Kristoffersen (:MikeK)]

(In reply to comment #47)
> mikek - do we just need the patches named something 1/3 and 3/3?

No we need 2/3 too - I have one that is updated with the comments from the review, just doing a sanity check on it - will upload it soon.

Comment 49

[Mike Kristoffersen (:MikeK)]

Attachment: Part 2/3 - Making it possible to control the define by options (v2 - updated after review)

Comment 50

[Doug Turner (:dougt)]

fennec bit:
  http://hg.mozilla.org/mobile-browser/rev/6596de4dd091

http://hg.mozilla.org/mozilla-central/rev/2b512173219b
http://hg.mozilla.org/mozilla-central/rev/4c2cf1947818

Comment 52

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This bug ended up being about removing window.crypto on e10s. Bug 582297 is about bringing this functionality back.