717481 - login manager storage should use default encryption type from the crypto provider

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Justin Dolske [:Dolske]]

Login Manager's crypto code is already largely split into nsILoginManagerCrypto / crypto-SDR.js. However, the SQL storage backend (storage-mozStorage.js) still makes assumptions about how crypto-SDR works, which makes it difficult to actually replace it with a different implementation.

Should be pretty simple to fix this. Due to product/sync requirements, fennec is likely to end up doing this encryption through Java APIs, and this will be the first step to making that easier to do. Makes sense to have even if they don't!

Patch shortly.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Passed tests (locally) on first attempt, so presumably there is something wrong with it. :-)

[A quick glance at tests/ surprisingly seems to indicate we don't test for base64 conversion or correct enctype columns, so either I'm missing that or this should be checked more carefully by hand.]

Comment 2

[Justin Dolske [:Dolske]]

wesj: I _think_ that with this patch, a Fennec build with a custom nsILoginManagerCrypto provider that forwards to a Java implementation should Just Work. Probably easiest, for the moment, to just replace the guts of crypto-SDR.js to test. Better to override @mozilla.org/login-manager/crypto/SDR;1. Eventually we should make storage-mozStorage.js use "@mozilla.org/login-manager/crypto?type=#" as needed.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 587915 [details] [diff] [review]
Patch v.1

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

So if somebody implements nsILoginManagerCrypto, they still need to actually encode in a compatible type, which really just means SDR, right?

Is the native Fennec work being done such that we would be able to take an encrypted pw and decode it with SDR on desktop?

::: toolkit/components/passwordmgr/nsILoginManagerCrypto.idl
@@ +36,5 @@
>  
>  
>  #include "nsISupports.idl"
>  
>  [scriptable, uuid(73f85239-421d-4d34-8d9c-79cf820ea1e6)]

Don't we need to rev the uuid here?

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Justin Dolske [:Dolske] from comment #1)
> Created attachment 587915 [details] [diff] [review]
> Patch v.1
> 
> Passed tests (locally) on first attempt, so presumably there is something
> wrong with it. :-)
> 
> [A quick glance at tests/ surprisingly seems to indicate we don't test for
> base64 conversion or correct enctype columns, so either I'm missing that or
> this should be checked more carefully by hand.]

It looks like we did add a few tests in bug 316084.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 587915 [details] [diff] [review]
Patch v.1

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

We talked IRL, so my concerns were addressed.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Why has this not landed?

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Bumped IID, and fixed trivial bitrot.

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a939d0ba3075

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/a939d0ba3075