Remove window.crypto for e10s

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: jduell, Assigned: MikeK)

Tracking

unspecified
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(4 attachments, 5 obsolete attachments)

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

Updated

8 years ago
Depends on: 559711

Comment 1

8 years ago
-> Me

Updated

8 years ago
Assignee: nobody → josh

Comment 2

8 years ago
Created attachment 441447 [details] [diff] [review]
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?
Attachment #441447 - Flags: feedback?(jduell.mcbugs)
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.

Updated

8 years ago
Attachment #441447 - Flags: feedback?(jduell.mcbugs) → feedback?(kaie)

Comment 4

8 years ago
Comment on attachment 441447 [details] [diff] [review]
WIP

Kai, same feedback request from comment 2 applies.

Comment 5

8 years ago
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

8 years ago
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

8 years ago
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

8 years ago
s/find/fine/

Updated

8 years ago
Attachment #441447 - Flags: feedback?(kaie)
(Reporter)

Updated

8 years ago
Blocks: 516730
(Reporter)

Comment 9

8 years ago
This blocks turning off NSS in content, not vice versa, unless I'm missing something.
Blocks: 559711
No longer depends on: 559711
(Reporter)

Comment 10

8 years ago
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.
I'm focusing on other bugs right now, so I won't get upset if somebody else wants to run with this.
Assignee: josh → nobody
I'll probably take this bug.
-> me
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
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?
tests+try server, then we can get a review request going.  jeremias, could you also take a first pass through the code for correctness?
One major issue: nobody ever checked to ensure that the changes worked correctly.
part of tests!
Created attachment 459258 [details] [diff] [review]
crypto_patch V2

Updated version of the patch for latest upstream
Created attachment 459574 [details] [diff] [review]
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.
Attachment #459258 - Attachment is obsolete: true
See comment 5 for more suggestions for tests.
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?
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
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?
(Reporter)

Comment 24

8 years ago
> 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.
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.
OS: Linux → Windows Server 2003
Created attachment 459912 [details] [diff] [review]
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.
Attachment #459574 - Attachment is obsolete: true
Attachment #459912 - Flags: review?(doug.turner)
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.
Attachment #459912 - Flags: superreview?(jst)
Attachment #459912 - Flags: review?(jst)
Attachment #459912 - Flags: review?(doug.turner)

Updated

8 years ago
Attachment #459912 - Flags: superreview?(jst)
Attachment #459912 - Flags: superreview+
Attachment #459912 - Flags: review?(jst)
Attachment #459912 - Flags: review+

Updated

8 years ago
Keywords: checkin-needed

Updated

8 years ago
blocking2.0: --- → ?
tracking-fennec: --- → 2.0a1+

Updated

8 years ago
Blocks: 582297
http://hg.mozilla.org/mozilla-central/rev/2335c842c06c

marking fixed.

Bug 582297 can track bring crypto back.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

8 years ago
tracking-fennec: 2.0a1+ → 2.0+

Comment 30

8 years ago
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
timeless, patches accepted.

Comment 32

8 years ago
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.
OS: Windows Server 2003 → All
Comment on attachment 459912 [details] [diff] [review]
Removes Crypto Object from nsGlobalWindow

MOZ_IPC is enabled also in FF.  :(
Attachment #459912 - Flags: superreview-
Attachment #459912 - Flags: superreview+
Attachment #459912 - Flags: review-
Attachment #459912 - Flags: review+

Updated

8 years ago
Keywords: checkin-needed

Comment 34

8 years ago
Technically this doesn't block Firefox, though it does block fennec. I think that means it doesn't need the blocking2.0 flag.
blocking2.0: ? → ---

Updated

8 years ago
Assignee: honzab.moz → mkristoffersen
(Assignee)

Comment 35

8 years ago
Created attachment 478093 [details] [diff] [review]
Part 1/3 - Adding uniq define to disable crypto object
Attachment #459912 - Attachment is obsolete: true
(Assignee)

Comment 36

8 years ago
Created attachment 478094 [details] [diff] [review]
Part 2/3 - Making it possible to control the define by options
(Assignee)

Comment 37

8 years ago
Created attachment 478095 [details] [diff] [review]
Part 3/3 - Setting the default behavior for Fennec to have the crypto object disabled
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.
Attachment #478093 - Flags: review?(jst)

Updated

8 years ago
Attachment #478094 - Flags: review?(khuey)

Updated

8 years ago
Attachment #478095 - Flags: review+
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
Attachment #478093 - Flags: review?(jst) → review+
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.
(Assignee)

Comment 41

8 years ago
(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
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.
(Assignee)

Comment 43

8 years ago
(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...
(Assignee)

Comment 44

8 years ago
(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 :)
(Assignee)

Updated

8 years ago
Attachment #478094 - Flags: review?(khuey)
(Assignee)

Comment 45

8 years ago
Created attachment 479425 [details] [diff] [review]
Part 1/3 - Adding uniq define to disable crypto object (v2 - updated after review)
Attachment #478093 - Attachment is obsolete: true
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.
Attachment #479425 - Flags: review+
mikek - do we just need the patches named something 1/3 and 3/3?
(Assignee)

Comment 48

8 years ago
(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.
(Assignee)

Comment 49

8 years ago
Created attachment 479516 [details] [diff] [review]
Part 2/3 - Making it possible to control the define by options (v2 - updated after review)
Attachment #478094 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #479516 - Flags: review?(khuey)

Updated

8 years ago
tracking-fennec: 2.0+ → 2.0b2+
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
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Duplicate of this bug: 582297
This bug ended up being about removing window.crypto on e10s. Bug 582297 is about bringing this functionality back.
Summary: e10s: forward JS 'crypto' calls from content->chrome → Remove window.crypto for e10s
You need to log in before you can comment on or make changes to this bug.