1188750 - webcrypto needs to ensure NSS has been initialized before deserializing anything

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Viga]

Attachment: The app

Steps to reproduce…
Deploy app to flame device via WebIDE
Run App
Comment out lines 148 –150 (namely the “then” clause that purges the database)
Re-run app without stopping it (hit refresh button).
Verify in console that the array of keys printed out is not empty.
Stop app (hit stop button).
Run App.
Observe the app has stopped suddenly and that a Mozilla Crash Report is shown on screen

Comment 1

[Warren WU]

Dear Wesly:
Only the Key generated by RSASSA-PKCS1-v1_5 can cause this problem,the object of CryptoKey can save Successful,but when we re open the database, we can't read it and the app has stopped suddenly.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

We didn't implement PKCS#8 export for private (EC)DH keys yet, so those can't currently be stored (i.e. serialized) in IndexedDB. What type of keys exactly are you trying to store?

Comment 3

[Viga]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> We didn't implement PKCS#8 export for private (EC)DH keys yet, so those
> can't currently be stored (i.e. serialized) in IndexedDB. What type of keys
> exactly are you trying to store?
Can you take a look at the app.js in attachment 8640326 [details]?
We use 
    asymmetric_: {
      name: 'RSASSA-PKCS1-v1_5',
      modulusLength: 2048,
      // Equivalent to 65537
      publicExponent: new Uint8Array([0x01, 0x00, 0x01]),
      hash: { name: 'SHA-256' }
    }

Comment 4

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

If CryptoKey is going to be stored in IndexedDB it needs to be supported on worker threads too, otherwise you're not going to be able to read from the database on a worker thread.

Comment 5

[Viga]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> If CryptoKey is going to be stored in IndexedDB it needs to be supported on
> worker threads too, otherwise you're not going to be able to read from the
> database on a worker thread.
I don't think we use it in worker thread. It's in an app process.

Comment 6

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

(In reply to Viga from comment #5)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> > If CryptoKey is going to be stored in IndexedDB it needs to be supported on
> > worker threads too, otherwise you're not going to be able to read from the
> > database on a worker thread.
> I don't think we use it in worker thread. It's in an app process.

Sure, that comment is directed at ttaubert.

Comment 7

[Viga]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> (In reply to Viga from comment #5)
> > (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> > > If CryptoKey is going to be stored in IndexedDB it needs to be supported on
> > > worker threads too, otherwise you're not going to be able to read from the
> > > database on a worker thread.
> > I don't think we use it in worker thread. It's in an app process.
> 
> Sure, that comment is directed at ttaubert.
That means, RSASSA-PKCS1-v1_5 can't be used at the moment?
Do you have any suggestion to substitute it?

Comment 8

[Guoqiang.CHEN]

Dear Taubert,

The Key generated by RSASSA-PKCS1-v1_5 can be successful save into indexDB and read directly without quitting the app.
But if we reopen app, we are unable read data from indexDB, a crash happens。

Comment 9

[Wesly Huang (EPM)]

Hi Taubert:

Not sure if the problem TCL met about RSASSA-PKCS1-v1_5 key storing in indexDB, is related to the fact that we didn't implement PKCS#8 export for private (EC)DH keys yet?

Per their description it's ok to store and read the key, the problem (crash) happened only after reopen the app, is that expected result if stop app by use "stop button" in WebIDE?

Comment 10

[Wesly Huang (EPM)]

Hi Taubert:

Following comment#9, TCL's intention is to "store crypto key for future use", so if you know other way then indexDB it should be useful for them too. Thank you!

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

1) What is this app doing? Why are we using AES-CBC and not an AEAD construction like AES-GCM?

2) AES-CBC with a zero IV is just AES-ECB. You should at least generate let's say 8 random bytes. But then you're still missing authentication, so maybe use GCM?

3) What's the exact crash that you're seeing? Do you have a stack trace? A crash ID?

4) Can you come up with a test case that runs/fails in a normal browser?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

It would help if someone could get a crash report from the device:

https://developer.mozilla.org/en-US/Firefox_OS/Debugging/Firefox_OS_crash_reporting

Comment 13

[Wesly Huang (EPM)]

Thanks Tim, and ni Guoqiang & Viga.

(In reply to Tim Taubert [:ttaubert] from comment #11)
> 1) What is this app doing? Why are we using AES-CBC and not an AEAD
> construction like AES-GCM?
> 
> 2) AES-CBC with a zero IV is just AES-ECB. You should at least generate
> let's say 8 random bytes. But then you're still missing authentication, so
> maybe use GCM?
> 
> 3) What's the exact crash that you're seeing? Do you have a stack trace? A
> crash ID?
> 
> 4) Can you come up with a test case that runs/fails in a normal browser?

Comment 14

[Viga]

We have additional information about the CryptoKey IndexedDB storage issue we’ve previously discussed. 

Specifically, this bug occurs when we attempt to retrieve a CryptoKey from indexedDB without previously calling any other methods on SubtleCrypto, IndexedDB throws an “UnknownErr” and the application crashes.

Alternatively, when we first call a method like ImportKey on SubtleCrypto, we can successfully retrieve the CryptoKey from IndexedDB.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

As I said before...

(In reply to Tim Taubert [:ttaubert] from comment #12)
> It would help if someone could get a crash report from the device:
> 
> https://developer.mozilla.org/en-US/Firefox_OS/Debugging/Firefox_OS_crash_reporting

So, please, if you have a Flame device and can reproduce it then you can as well get a crash ID out of your device. This should be the first thing you do whenever you file a bug about a crash anywhere in the platform.

Comment 16

[Warren WU]

Attachment: dmp_result.txt

Dear Taubert:
This file is parsed from crash minidump file, it contains complete stack backtrace，hope useful for you.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

That helps a lot, thanks! Now I just need to find some time to dig deeper...

Comment 18

[Guoqiang.CHEN]

Dear Tauber,

The crash only happened while reading 'RSASSA-PKCS1-v1_5' indexedDB, but read 'AES-CBC' is ok.

And we find use window.crypto.subtle.generateKey to generate 'RSASSA-PKCS1-v1_5' key need 5~10 seconds, is it a normal time?
Generate 'AES-CBC' key only need within 0.05 second.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Guoqiang.CHEN from comment #18)
> And we find use window.crypto.subtle.generateKey to generate
> 'RSASSA-PKCS1-v1_5' key need 5~10 seconds, is it a normal time?

Yes, finding/generating two large ~2047-bit primes will take some time, esp. on slow hardware like a phone.

> Generate 'AES-CBC' key only need within 0.05 second.

AES keys are simply 128 (or more) random bits. They have no structure and an be generated in no time.

Comment 20

[Viga]

Thank you Tim.
Can you please re-read my comment #14?


Alternatively, when we first call a method like ImportKey on SubtleCrypto, we can successfully retrieve the CryptoKey from IndexedDB.

Does this help to ping down the problem?

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Viga from comment #20)
> Can you please re-read my comment #14?

I have read your comments and now new information is needed. All we needed was a stack trace, that Warren then finally submitted.

I simply don't have time to tackle this at the moment, sorry.

Comment 22

[Warren WU]

Dear Tim:

I also reproduce this problem on flame device, you can view the crash report in the following address:

https://crash-stats.mozilla.com/report/index/f44d1379-4e5d-4f86-9bca-d60a42150806

Comment 23

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

I had a quick look, and it doesn't look like that code path is guaranteeing that NSS has been initialized. Essentially, EnsureNSSInitializedChromeOrContent() has to be called before any webcrypto objects are deserialized.

Comment 24

[Viga]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #23)
> I had a quick look, and it doesn't look like that code path is guaranteeing
> that NSS has been initialized. Essentially,
> EnsureNSSInitializedChromeOrContent() has to be called before any webcrypto
> objects are deserialized.

Hello, based on app.js I attached. Can you specify a little more detailed?

Comment 25

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

The bug is in the webcrypto implementation, not in the app. I'll see what resources we have for fixing this.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1188750-CryptoKey-ReadStructuredClone-needs-to-e.patch

ReadStructuredClone() is always called from the main thread, and it's the only method that needs to ensure NSS has been initialized afaict.

Do you think having a test for this is necessary? I could think of using a <browser remote=true> to create a new process and try to immediately deserialize a CryptoKey there? Not totally sure if that would be successful.

Comment 27

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

Comment on attachment 8644919 [details] [diff] [review]
0001-Bug-1188750-CryptoKey-ReadStructuredClone-needs-to-e.patch

Review of attachment 8644919 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. I'm concerned there are other similar entrypoints, but I couldn't find any. Another concern is potentially deadlocking when we attempt to initialize NSS after acquiring the shutdown prevention lock, but I couldn't find a concrete indication that that would happen. We should probably make a test build and try out the attached app before landing.

Comment 28

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

(In reply to Tim Taubert [:ttaubert] from comment #26)
> Created attachment 8644919 [details] [diff] [review]
> 0001-Bug-1188750-CryptoKey-ReadStructuredClone-needs-to-e.patch
> 
> ReadStructuredClone() is always called from the main thread, and it's the
> only method that needs to ensure NSS has been initialized afaict.
> 
> Do you think having a test for this is necessary? I could think of using a
> <browser remote=true> to create a new process and try to immediately
> deserialize a CryptoKey there? Not totally sure if that would be successful.

A test would be nice, if we could get it to be reliable.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1188750-Add-test-to-ensure-NSS-is-initialized-be.patch

Here's a test. It properly fails without the patch applied when run on desktop saying that "The object could not be cloned.", in contrast to B2G which seems to crash.

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf8db542b3d1

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1188750-Add-test-to-ensure-NSS-is-initialized-be.patch, v2

Oops.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0b8f752954f

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

Viga, Warren, can any of you please confirm that the first patch listed here solves the problem for you?

Comment 33

[Warren WU]

(In reply to Tim Taubert [:ttaubert] from comment #32)
> Viga, Warren, can any of you please confirm that the first patch listed here
> solves the problem for you?

It works well，thanks！

Comment 34

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

Comment on attachment 8645365 [details] [diff] [review]
0002-Bug-1188750-Add-test-to-ensure-NSS-is-initialized-be.patch, v2

Review of attachment 8645365 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me (and appears to work as expected). I think someone with a better understanding of indexedDB should review it, though.

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8645365 [details] [diff] [review]
0002-Bug-1188750-Add-test-to-ensure-NSS-is-initialized-be.patch, v2

Kyle, can you maybe take a look at this? Thanks!

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

I'm out for a few weeks soon. If this gets r+ it would be great if someone could land it. Thanks :)

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec8b34e2bc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf71f3a4465

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android 4.3 dom/crypto/test/test_indexedDB.html timeouts.
https://treeherder.mozilla.org/logviewer.html#?job_id=12836281&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b35236fc76e

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

The test at least isn't timing out because it takes too long. It properly times out waiting for something. Couldn't figure it out before leaving unfortunately, if anyone wants to pick this up, please do :)

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

Will re-land the patches here and skip the test on Android for now. Bug 1200570 will/can handle enabling this test for Android, if that's at all possible?

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/480bb91340d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b15d4086b2ce

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/480bb91340d3
https://hg.mozilla.org/mozilla-central/rev/b15d4086b2ce