718760 - Implement Java version of LoginManagerCrypto

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Wesley Johnston (:wesj)]

For native fennec password sync, we'll need some way for sync to encrypt passwords, even if Gecko is not running. We can do this by implementing the LoginManagerCrypto service for ourselves in JS, and having it call into Java to do the encryption (the fennec password manager can do the encryption in java for sync'ed passwords):

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsILoginManagerCrypto.idl#42

The Crypto service will either need to be made aware of the MasterPassword (or set up to fall back to the normal crypto service if MP is enabled?). For first draft, we're not enabling password sync if MP is enabled.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Example for ROT13

Here's a quick example for how to do ROT13 with nsILoginManagerCrypto.

A Java-crypto flavor of this would need to:

1) replace the _rot13() calls with something that does actual crypto
2) ship this code in Fennec as a component replacing the @mozilla.org/login-manager;1  [or we can do something smarter]
3) decide if Fennec wants to support a "master password/pin" kind of thing, and where the prompting et al for that should happen.
4) keep bsmith in the loop so SG doesn't freak out when they see this. :)

Doing step #1 is the real work here (and #3, I suppose). This patch should just help with not having to understand all the login manager and xpcom goop involved.

Comment 2

[Justin Dolske [:Dolske]]

(In reply to Justin Dolske [:Dolske] from comment #1)
> Created attachment 589333 [details] [diff] [review]
> Example for ROT13

I left in all the MP state-change goop, since it was just cut'n'paste and maybe Fennec will want it. Otherwise encrypt() and decrypt() would be 1-line wrappers around _rot13().

Comment 3

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

Kai, here's another example of libxul/NSS/PSM startup time causing us extreme pain and more work.

Comment 4

[Kai Engert (:KaiE:)]

startup time isn't pain.
missing security and broken functionality, that's pain.

I don't have time to help you improve startup time, and I don't believe reimplementing/duplicating crypto code is a good idea.

Comment 5

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

(In reply to Justin Dolske [:Dolske] from comment #1)
> 4) keep bsmith in the loop so SG doesn't freak out when they see this. :)

At first when I saw this I was like "OH NOES!!!!!!!!!!" but actually I don't think this change will be a bug deal, after talking with Justin about it. AFAICT, you will be able to use javax.crypto for everything here, right?

> 3) decide if Fennec wants to support a "master password/pin" kind of thing,
> and where the prompting et al for that should happen.

I am very curious about this to. It would be a good idea to talk with me, or to post on dev-tech-crypto, regarding how you are going to store the encryption key on disk and/or derive it from the master password or whatever.

As I mentioned to Justin, I am skeptical about the value of master password on mobile in general. But, especially, I am skeptical about the value that encryption adds (over sandboxing) for Mobile. It may be that you do not actually gain any real security by doing any encryption at all. In that case, your job would be much easier.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

This is just a WIP from some scattered bits I found around the web. Should work for the login manager though (need to hook it up to the password database, and going to meet with bsmith tomorrow to talk about what to do with master password and this (although we SHOULD be able to support MP here, its more a matter of how sync is going to ask for it I think).

Comment 7

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

(In reply to Wesley Johnston (:wesj) from comment #6)
> Created attachment 590061 [details] [diff] [review]
> WIP Patch
> 
> This is just a WIP from some scattered bits I found around the web. Should
> work for the login manager though (need to hook it up to the password
> database, and going to meet with bsmith tomorrow to talk about what to do
> with master password and this (although we SHOULD be able to support MP
> here, its more a matter of how sync is going to ask for it I think).

Just remember that syncing with Master Password set is not a requirement for Fx11. We can just have Sync ignore syncing passwords in this case. We can work on that feature post-Fx11.

At the same time we do want to support master password for Fx11, just not integrated with Sync right away.

Comment 8

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

If you don't need to support master password then you don't need any encryption at all, right? As long as you store signons.sqlite in the same place that you would store whatever encryption key you would use, in internal sandboxed storage, then there is not much security benefit to encrypting the signons, AFAICT.

Comment 9

[Justin Dolske [:Dolske]]

(In reply to Kai Engert (:kaie) from comment #4)
> startup time isn't pain.
> missing security and broken functionality, that's pain.

Just for the record: startup time is a _huge_ issue for mobile, and we've been dedicating huge resources to fixing it. This can not be overstated. (Desktop too, but to smaller degree.) Code that's unable to be performant on mobile can and will be bypassed -- see XUL and Places and two large examples.

I would also disagree with the implication that that the only choice here is to use NSS/PSM or be insecure/broken. It takes time and careful engineering to get security right, but there is plenty of software out there that manages to do so without relying on our legacy stack.

It's a bit disturbing to see a curt message from a module owner that seems unaware of the current state of things. We're going to need everyone helping out if Mozilla is to be popular -- or even just _viable_ -- on mobile.

Anyway, Brian and I had a very constructive conversation yesterday. It sounds like using PSM/NSS might not be as difficult as thought, and despite being overloaded like everyone else he's willing to help provide some guidance on how to make that happen. So I think we can come to a good solution, even if it's not yet clear what that will be.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

I decided exposing NSS is probably the best way to do this. This implements that, but needs cleanup (I think I'm leaking strings, and there are some pieces that could just generally be written better. Maybe we're better creating an NSSBridge object instead of having these static methods... ), and some additional work to ensure that we're always calling from a different process than Fennec.

Asking for feedback from blassey (for the java bits) and bsmith (for the nss bits) to get some of the initial reviewing started at least and find out if you see any big problems. I will put the separate process stuff in a different patch.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 592979 [details] [diff] [review]
WIP Patch

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

overall this is really good stuff. f- because of the startup perf issue though, let's get that figured out

::: mobile/android/base/GeckoThread.java
@@ +107,5 @@
> +        try {
> +            String encrypted = NSSBridge.encrypt(GeckoApp.mAppContext, "my password");
> +            Log.i(LOGTAG, "Encrypted " + encrypted);
> +            String decrypted = NSSBridge.decrypt(GeckoApp.mAppContext, encrypted);
> +            Log.i(LOGTAG, "Decrypted " + decrypted);

I'm assuming this is just test code that will be removed

@@ +137,5 @@
> +                } while(c.moveToNext());
> +            }
> +        } catch(Exception ex) {
> +            Log.e(LOGTAG, "xxx - exception", ex);
> +        }

this looks like a pretty big startup time hit (lots of disk access). It looks to me like we don't need this stuff at this point. Please move this off the critical start up path, presumably to the point where this is actually used.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: MultiProcess patch

Splits of the passwords provider to always run in its own process. This requires that our library loader stuff in GeckoAppShell doesn't depend on being run in the GeckoApp context, and some changes that go along with that.

It also means that we needed to use Intents to yell at Gecko to initialize the passwords database (which actually should also initialize key3/4.db I think?). I created an Intent that just has us send a message across the bridge to Gecko. Not completely sure if that's "safe" or not... We can do something more specific and maybe add a few checks on who sent the intent (I think?) if need be.

Comment 13

[Wesley Johnston (:wesj)]

Shoot. I think sending that intent will bring GeckoApp to the front which we don't want either (although it should only happen once...)

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 593277 [details] [diff] [review]
MultiProcess patch

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

::: mobile/android/base/AndroidManifest.xml.in
@@ +163,5 @@
>                    android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"/>
>  
>          <provider android:name="@ANDROID_PACKAGE_NAME@.db.PasswordsProvider"
>                    android:authorities="@ANDROID_PACKAGE_NAME@.db.passwords"
> +									android:process="org.mozilla.gecko.PasswordsProvider"

why change the permissions?

::: mobile/android/base/GeckoApp.java
@@ +1482,5 @@
>          Intent intent = getIntent();
> +        final String action = intent.getAction();
> +        if (ACTION_GECKO_MSG.equals(action)) {
> +            String msg = intent.getStringExtra("msg");
> +            GeckoAppShell.sendEventToGecko(new GeckoEvent(msg, null));

oh wow... this is a pretty huge security hole. Let's do something less open ended.

::: mobile/android/base/GeckoAppShell.java
@@ -370,3 @@
>          }
>  
> -        GeckoAppShell.putenv("HOME=" + homeDir);

are you sure dropping this doesn't break anything? I'd rather keep it to be safe

@@ -370,4 @@
>          }
>  
> -        GeckoAppShell.putenv("HOME=" + homeDir);
> -        GeckoAppShell.putenv("GRE_HOME=" + GeckoApp.sGREDir.getPath());

I've convinced myself that this is safe to drop because it was only used by xulrunner.

@@ -377,5 @@
> -        for (int c = 1; env != null; c++) {
> -            GeckoAppShell.putenv(env);
> -            env = i.getStringExtra("env" + c);
> -            Log.i(LOGTAG, "env"+ c +": "+ env);
> -        }

dropping this will break some forms of testing. Let's not do that

::: mobile/android/base/GeckoThread.java
@@ +93,5 @@
>          // so just save it to locale here and reset it as default after the join
>          Locale locale = Locale.getDefault();
>          String resourcePath = app.getApplication().getPackageResourcePath();
> +        GeckoAppShell.ensureSQLiteLibsLoaded((Context)app, resourcePath);
> +        GeckoAppShell.ensureNSSLibsLoaded((Context)app, resourcePath);

why does app need a cast? Its a subclass of Context

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch v1 1/2

I think I'm ready to start reviews on these.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch v1 2/2

> oh wow... this is a pretty huge security hole. Let's do something less open
> ended.

I moved this to a broadcaster which seems safer. Yay I'm learning!

> are you sure dropping this doesn't break anything? I'd rather keep it to be
> safe

> I've convinced myself that this is safe to drop because it was only used by
> xulrunner.

> dropping this will break some forms of testing. Let's not do that

For these three comments, I don't think I'm dropping anything here. Diff is just being strange about how it formats things. The only pieces I wanted to clue out were ones that required either an Intent or GeckoApp (i.e. the getPluginsDirectory bit I think). This should only happen if we're called from the PasswordProvider, not from a normal run... i think?

Comment 17

[Ian Melven :imelven]

Comment on attachment 593615 [details] [diff] [review]
Patch v1 2/2

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

::: mobile/android/base/db/PasswordsProvider.java.in
@@ +130,5 @@
>              if (dbNeedsSetup) {
>                  Log.i(LOGTAG, "Sending init to gecko");
>                  mBridge = null;
> +                Intent initIntent = new Intent(GeckoApp.ACTION_INIT_PW);
> +                context.sendBroadcast(initIntent);

I think this needs to be context.sendBroadcast(initIntent, @ANDROID_PACKAGE_NAME@.permissions.PASSWORD_PROVIDER) according to http://developer.android.com/reference/android/content/Context.html#sendBroadcast(android.content.Intent,%20java.lang.String)

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 593614 [details] [diff] [review]
Patch v1 1/2

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

bsmith, how well will NSS deal with "modified UTF-8"?

::: mobile/android/base/MutableMatrixCursor.java
@@ +24,5 @@
> +
> +    private final String[] columnNames;
> +    private Object[] data;
> +    private int rowCount = 0;
> +    private final int columnCount;

our java code is solidly anti-java in its style, use the m prefix for these

@@ +35,5 @@
> +     * @param initialCapacity in rows
> +     */
> +    public MutableMatrixCursor(String[] columnNames, int initialCapacity) {
> +        this.columnNames = columnNames;
> +        this.columnCount = columnNames.length;

with the m prefixes there is no need for the this.

@@ +170,5 @@
> +            int newSize = data.length * 2;
> +            if (newSize < size) {
> +                newSize = size;
> +            }
> +            this.data = new Object[newSize];

this ain't js, no need for "this." here or anywhere else

::: mobile/android/base/NSSBridge.java
@@ +22,5 @@
> +        String res = "";
> +        try {
> +            String path = GeckoDirProvider.getProfileDir(context).toString();
> +            res = nativeEncrypt(path, aValue);
> +        } catch(Exception ex) { }

log your exceptions

Comment 19

[Justin Dolske [:Dolske]]

(In reply to Brad Lassey [:blassey] from comment #18)

> bsmith, how well will NSS deal with "modified UTF-8"?

What's "modified" UTF8?

It generally shouldn't care, iirc it just treats it all as blob of bytes.

We feed it UTF-8 in JS: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/crypto-SDR.js#131

Comment 20

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #17)
> Comment on attachment 593615 [details] [diff] [review]
> Patch v1 2/2
> 
> Review of attachment 593615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/PasswordsProvider.java.in
> @@ +130,5 @@
> >              if (dbNeedsSetup) {
> >                  Log.i(LOGTAG, "Sending init to gecko");
> >                  mBridge = null;
> > +                Intent initIntent = new Intent(GeckoApp.ACTION_INIT_PW);
> > +                context.sendBroadcast(initIntent);
> 
> I think this needs to be context.sendBroadcast(initIntent,
> @ANDROID_PACKAGE_NAME@.permissions.PASSWORD_PROVIDER) according to
> http://developer.android.com/reference/android/content/Context.
> html#sendBroadcast(android.content.Intent,%20java.lang.String)

thinking about this more, actually i don't think we need this - adding the permission in sendBroadcast makes it so only receivers with this permission receive the broadcast. I don't think there's any sensitive data in the Intent we're broadcasting so it's ok for other processes to receive it. We already have the permission in the <receiver> tag in the manifest so only _senders_ with this permission can send this Intent - which is what we actually care about.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Ian Melven :imelven from comment #20)
> (In reply to Ian Melven :imelven from comment #17)
> > Comment on attachment 593615 [details] [diff] [review]
> > Patch v1 2/2
> > 
> > Review of attachment 593615 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/db/PasswordsProvider.java.in
> > @@ +130,5 @@
> > >              if (dbNeedsSetup) {
> > >                  Log.i(LOGTAG, "Sending init to gecko");
> > >                  mBridge = null;
> > > +                Intent initIntent = new Intent(GeckoApp.ACTION_INIT_PW);
> > > +                context.sendBroadcast(initIntent);
> > 
> > I think this needs to be context.sendBroadcast(initIntent,
> > @ANDROID_PACKAGE_NAME@.permissions.PASSWORD_PROVIDER) according to
> > http://developer.android.com/reference/android/content/Context.
> > html#sendBroadcast(android.content.Intent,%20java.lang.String)
> 
> thinking about this more, actually i don't think we need this - adding the
> permission in sendBroadcast makes it so only receivers with this permission
> receive the broadcast. I don't think there's any sensitive data in the
> Intent we're broadcasting so it's ok for other processes to receive it. We
> already have the permission in the <receiver> tag in the manifest so only
> _senders_ with this permission can send this Intent - which is what we
> actually care about.

The doc isn't all that clear, but my reading is that including that permissions string would be enforcing the permission on the receiver to receive the broadcast, so it would be unneeded here. Again, not super clear though.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch 3/2

Talked to bsmith about this briefly. Forgot to make sure we're shutting down NSS. One option is to just shut it down after every encrypt/decrypt. The other is to provide a way to manually shut it down like I'm doing here.

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch 3/2

Whoops. Forgot to make this method public.

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch 3/2

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 593614 [details] [diff] [review]
Patch v1 1/2

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

::: mozglue/android/NSSBridge.cpp
@@ +93,5 @@
> +    if (rv != SECSuccess) {
> +        char * err;
> +        getError("nativeEncrypt", &err);
> +        delete err;
> +        JNI_Throw(jenv, "org/mozilla/gecko/sqlite/SQLiteBridgeException", err);

You just deleted err, so you're referencing stale memory here.

@@ +119,5 @@
> +    if (rv != SECSuccess) {
> +        char * err;
> +        getError("nativeEncrypt", &err);
> +        delete err;
> +        JNI_Throw(jenv, "org/mozilla/gecko/sqlite/SQLiteBridgeException", err);

Same error.

@@ +204,5 @@
> +  if (!result)
> +    return;
> +
> +  asprintf(_retval, "%s", result);
> +  //delete(result);

Remove this.

@@ +212,5 @@
> +  PRUint32 len = strlen(data);
> +  int adjust = 0;
> +
> +  /* Compute length adjustment */
> +  if (data[len-1] == '=') {

This will break if you get passed an empty or short string.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 595121 [details] [diff] [review]
Patch 3/2

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

::: mozglue/android/NSSBridge.cpp
@@ +130,5 @@
> +Java_org_mozilla_gecko_NSSBridge_shutdownNSS(JNIEnv* jenv, jclass)
> +{
> +    if (initialized) {
> +        f_NSS_Shutdown();
> +        initialized = false;

This isn't thread safe, is there any possibility this gets called on different threads?

Comment 27

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

Comment on attachment 593614 [details] [diff] [review]
Patch v1 1/2

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

I reviewed only the usage of NSS, and some of the error handling and memory allocation logic. The general usage of NSS here seems right. The error handling and memory management both need work. Double-check the other error handling and other uses of free/delete to make sure I didn't miss any during the review. I will review the updated version.

::: mozglue/android/APKOpen.cpp
@@ +838,5 @@
> +
> +  if (!plc_handle)
> +    __android_log_print(ANDROID_LOG_ERROR, "GeckoLibLoad", "Couldn't get a handle to libplc4!");
> +
> +  setup_nss_functions(nss_handle, nspr_handle, plc_handle);

I don't understand the error handling strategy here. What if loading any of these libraries fails? What if setup_nss_functions fails? Does the program still give well-defined results?

@@ +877,5 @@
>    jenv->ReleaseStringUTFChars(jApkName, str);
>  }
>  
> +extern "C" NS_EXPORT void JNICALL
> +Java_org_mozilla_gecko_GeckoAppShell_loadNSSLibsNative(JNIEnv *jenv, jclass jGeckoAppShellClass, jstring jApkName, jboolean jShouldExtract) {

It seems like you must return a success/failure code to the caller, so the caller knows whether the function succeeded.

@@ +920,2 @@
>    loadSQLiteLibs(argv[i]);
>    loadGeckoLibs(argv[i]);

We must check for errors for all of these cases, unless these called functions exit() when they fail.

::: mozglue/android/NSSBridge.cpp
@@ +15,5 @@
> +
> +#define LOG(x...) __android_log_print(ANDROID_LOG_INFO, "GeckoJNI", x)
> +
> +static void
> +JNI_Throw(JNIEnv* jenv, const char* name, const char* msg)

This code would be much clearer if you replaced the "const char * msg" argument with a "PRErrorCode errorCode" argument, and did the PRErrorCode -> string conversion within JNI_Throw.

@@ +20,5 @@
> +{
> +    jclass cls = jenv->FindClass(name);
> +    if (cls == NULL) {
> +        printf("Couldn't find exception class (or exception pending)\n");
> +        return;

You must either throw an exception (e.g. ClassNotFoundException) or exit(). You cannot just silently return.

@@ +24,5 @@
> +        return;
> +    }
> +    int rc = jenv->ThrowNew(cls, msg);
> +    if (rc < 0) {
> +        printf("Error throwing exception\n");

Ditto. If ThrowNew fails, there's not much you can do besides exit(), and it isn't safe to just eat the error.

@@ +44,5 @@
> +NSS_WRAPPER_INT(PR_GetError)
> +NSS_WRAPPER_INT(PL_Base64Encode)
> +NSS_WRAPPER_INT(PL_Base64Decode)
> +
> +void setup_nss_functions(void *nss_handle,

The caller cannot tell whether this function succeeded or failed. This function should return an indicator of whether or not it succeeded.

Because of the other missing error handling in the program, this function should definitely check its input parameters and fail if any are null.

@@ +67,5 @@
> +  PLCFUNC(PL_Base64Decode);
> +#undef PLCFUNC
> +}
> +
> +static void getError(char * funcString, char** msg) {

Document that msg must be freed by the caller using free().

@@ +92,5 @@
> +    jenv->ReleaseStringUTFChars(jPath, path);
> +    if (rv != SECSuccess) {
> +        char * err;
> +        getError("nativeEncrypt", &err);
> +        delete err;

in addiction to the problem gcp pointed out, err must be freed using free(), not delete.

@@ +113,5 @@
> +
> +    char* result;
> +    SECStatus rv;
> +    rv = nativeEncrypt(path, value, &result, false);
> +    jenv->ReleaseStringUTFChars(jValue, value);

I assume that ReleaseStringUTFChars() frees the buffer it is passed in using free().

@@ +117,5 @@
> +    jenv->ReleaseStringUTFChars(jValue, value);
> +    jenv->ReleaseStringUTFChars(jPath, path);
> +    if (rv != SECSuccess) {
> +        char * err;
> +        getError("nativeEncrypt", &err);

You should not call *any* functions between the time you call a function that returns SECStatus, and the time that you call PR_GetError(), because those intermediate function calls may have replaced errno (or the NSPR equivalent) with something else.

@@ +118,5 @@
> +    jenv->ReleaseStringUTFChars(jPath, path);
> +    if (rv != SECSuccess) {
> +        char * err;
> +        getError("nativeEncrypt", &err);
> +        delete err;

err must be freed with free(), not delete.

@@ +125,5 @@
> +
> +    return jenv->NewStringUTF(result);
> +}
> +
> +SECStatus nativeEncrypt(const char *path, const char *value, char** result, bool encrypt) {

Document that the caller must free result with free().

@@ +168,5 @@
> +    reply.len = 0;
> +
> +    if (encrypt) {
> +      SECItem keyid;
> +      keyid.data = 0;

keyid.data = NULL;

@@ +188,5 @@
> +          asprintf(&errorMsg, "Error encoding result");
> +          goto error;
> +      }
> +    } else {
> +      *result = (char *)reply.data;

You should check here that reply.data[reply.len-1] == '\0', instead of assuming so.

You must make a copy (strcpy) of reply.data, because you need to de-allocate the memory in reply below.

You must free request.data with PR_Free here.

@@ +189,5 @@
> +          goto error;
> +      }
> +    } else {
> +      *result = (char *)reply.data;
> +    }

Before returning,you must free reply with SECItem_ZFreeItem(&reply, false);

(You should always use SECItem_[Z]FreeItem to free memory allocated by NSS functions that output SECItems), instead of using free() directly.

@@ +194,5 @@
> +    return SECSuccess;
> +
> +error:
> +    LOG("Error in NSSBridge: %s\n", errorMsg);
> +    delete errorMsg;

free(errorMsg);

@@ +198,5 @@
> +    delete errorMsg;
> +    return rv;
> +}
> +
> +void encode(const unsigned char *data, PRInt32 dataLen, char **_retval) {

Document that retval must be freed using free().

@@ +204,5 @@
> +  if (!result)
> +    return;
> +
> +  asprintf(_retval, "%s", result);
> +  //delete(result);

PR_Free(result);

@@ +207,5 @@
> +  asprintf(_retval, "%s", result);
> +  //delete(result);
> +}
> +
> +void decode(const char *data, unsigned char **result, PRInt32 * _retval) {

document that result must be freed using free().

@@ +217,5 @@
> +    adjust++;
> +    if (data[len-2] == '=') adjust++;
> +  }
> +
> +  *result = (unsigned char *)f_PL_Base64Decode(data, len, NULL);

PL_Base64Decode returns memory that must be freed using PR_Free. But, you need a buffer that can be freed using free(). So, you need to make a copy of the result of PL_Base64Decode, then PR_Free the result of PL_Base64Decode.

Comment 28

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

Comment on attachment 595121 [details] [diff] [review]
Patch 3/2

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

::: mozglue/android/NSSBridge.cpp
@@ +129,5 @@
> +extern "C" NS_EXPORT void JNICALL
> +Java_org_mozilla_gecko_NSSBridge_shutdownNSS(JNIEnv* jenv, jclass)
> +{
> +    if (initialized) {
> +        f_NSS_Shutdown();

You should, at least, log the result of NSS_Shutdown (PR_GetError()) when it returns something other than SECSuccess.

Comment 29

[Wesley Johnston (:wesj)]

Attachment: Patch v2 Parts 1+2

Uhmm.. I just folded these two patches because... I accidentally folded them.

> I don't understand the error handling strategy here. What if loading any of
> these libraries fails? What if setup_nss_functions fails? Does the program
> still give well-defined results?

So I added some error handling through here. We might want more. Or I may be too strict brad?

> This code would be much clearer if you replaced the "const char * msg"
> argument with a "PRErrorCode errorCode" argument, and did the PRErrorCode ->
> string conversion within JNI_Throw.

I did this somewhat now. We're doing the throw and most of the string work all in one place.

> Document that msg must be freed by the caller using free().

free() is used throughout, but I'm a bit confused about where I need to free strings and where SECITEM_ZfreeItem should be used to free things. I think I've got it right here, basically calling free any time I'm using malloc or aspritf, but I'd appreciate the extra eyes.

> I assume that ReleaseStringUTFChars() frees the buffer it is passed in using free().

Yep.

Comment 30

[Wesley Johnston (:wesj)]

I realized that I left some debugging code in GeckoThread again. Sorry 'bout that :(. Please ignore. All that should be in there is the ensureNSSLibsLoaded stuff. I half wonder if we should be passing a bool there as well so that we don't bother to call setup_nss_libraries unless we need to. Optimizations that can come later I think though...

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 595604 [details] [diff] [review]
Patch v2 Parts 1+2

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

::: mobile/android/base/GeckoAppShell.java
@@ +347,4 @@
>          if (oldHome.exists())
>              moveDir(oldHome, profileDir);
>  
> +        if (Build.VERSION.SDK_INT < 8 || (aContext instanceof Activity) &&

this needs to be &&

::: mobile/android/base/GeckoThread.java
@@ +121,5 @@
> +            ArrayList<Object[]> results = bridge.query("SELECT * FROM moz_logins");
> +            for (Object resultRow: results) {
> +                Object[] resultColumns = (Object[])resultRow;
> +                for (int i = 0; i < resultColumns.length; i++) {
> +                    Log.i(LOGTAG, "xxx Got result " + i + ": " + resultColumns[i]);

I don't think you ant to keep this logging, right?

@@ +132,5 @@
> +            Cursor c = app.getContentResolver().query(BrowserContract.Passwords.CONTENT_URI, cols, null, null, null);
> +            if (c != null && c.moveToFirst()) {
> +                do {
> +                    for (int i = 0; i < c.getColumnCount(); i++) {
> +                        Log.i(LOGTAG, "xxx Query result " + i + ": " + c.getString(i));

again, expensive debug logging

::: mozglue/android/NSSBridge.cpp
@@ +49,5 @@
> +  }
> +#define GETFUNC(name) f_ ## name = (name ## _t) __wrap_dlsym(nss_handle, #name)
> +  GETFUNC(NSS_Initialize);
> +  if (!f_NSS_Initialize)
> +    return FAILURE;

why not include the if () return FAILURE in the macro?

Comment 32

[Wesley Johnston (:wesj)]

Attachment: Patch v3

I rebased this on top of the form history patches (and fixed some bugs while writing tests). Very little is changed functionally, but things are reorganzied. Sorry blassey, but I didn't want to slip 'em by without giving you the chance to re-review.

Comment 33

[Wesley Johnston (:wesj)]

Attachment: Tests

Updates the password provider tests I wrote, as well as adding a new test for encrypting passwords.

Comment 34

[Geoff Brown [:gbrown]]

Test patch depends on test patch in bug 725881.

Comment 35

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 598042 [details] [diff] [review]
Patch v3

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

::: mobile/android/base/GeckoAppShell.java
@@ +348,5 @@
>              moveDir(oldHome, profileDir);
>  
> +        if (Build.VERSION.SDK_INT < 8 && (aContext instanceof Activity) &&
> +            (((Activity)aContext).getApplication().getPackageResourcePath().startsWith("/data") ||
> +             ((Activity)aContext).getApplication().getPackageResourcePath().startsWith("/system"))) {

This still isn't right, sorry if I wasn't clear (or just plain wrong) before. What you need is:

if (Build.VERSION.SDK_INT < 8 || ((aContext instanceof Activity) &&
(geckoApp.getApplication().getPackageResourcePath().startsWith("/data") ||		(((Activity)aContext).getApplication().getPackageResourcePath().startsWith("/data") ||
geckoApp.getApplication().getPackageResourcePath().startsWith("/system")) {		((Activity)aContext).getApplication().getPackageResourcePath().startsWith("/system")))))

@@ +377,4 @@
>              }
> +        }
> +        GeckoAppShell.putenv("HOME=" + homeDir);
> +        GeckoAppShell.putenv("GRE_HOME=" + aContext.getApplicationInfo().dataDir);

let's make this:

String gre_home = aContext instance of GeckoApp ? GeckoApp.sGREDir.getPath() : aContext.getApplicationInfo().dataDir;
GeckoAppShell.putenv("GRE_HOME=" + gre_home);

Comment 36

[Geoff Brown [:gbrown]]

Comment on attachment 598043 [details] [diff] [review]
Tests

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

::: mobile/android/base/tests/testPasswordEncrypt.java.in
@@ +8,5 @@
> +import android.database.Cursor;
> +import android.content.Context;
> +import android.net.Uri;
> +import java.io.File;
> +import android.util.Log;

nit - remove unused imports

@@ +49,5 @@
> +          mAsserter.is(uri, null, "Insert returned null correctly");
> +  
> +          // This should fail the first time round because there is no pw database
> +          // Wait for gecko to reply and then we'll try again
> +          contentEventExpecter = mActions.expectGeckoEvent("Passwords:Init:Return");

Race: Create the event expecter earlier, to avoid the possibility of missing the event and blocking indefinitely.

@@ +52,5 @@
> +          // Wait for gecko to reply and then we'll try again
> +          contentEventExpecter = mActions.expectGeckoEvent("Passwords:Init:Return");
> +          contentEventExpecter.blockForEvent();
> +  
> +          cr.insert(pwuri, cvs);

Perhaps check the return value here.

Comment 37

[Richard Newman [:rnewman]]

Any news on this, wesj?

Comment 38

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

Comment on attachment 598042 [details] [diff] [review]
Patch v3

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

I reviewed only the NSS-related parts of APKOpen.cpp and NSSBridge.cpp. The encryption/decryption and allocate/free logic seems reasonable. It seems like the error handling is about right as far as the NSS-related parts go. The thing that seems most iffy is the chdir() call in APKOpen.cpp.

::: mozglue/android/APKOpen.cpp
@@ +75,5 @@
>  #ifndef RUSAGE_THREAD
>  #define RUSAGE_THREAD 1
>  #endif
>  
> +typedef int nsresult;

nsresult is a bad name because we already use that name in Gecko for something similar but different.

@@ +658,5 @@
>  #ifdef MOZ_OLD_LINKER
>  extern "C" void simple_linker_init(void);
>  #endif
>  
> +static int

static nsresult (or whatever you rename nsresult).

But, it seems like this function never returns anything other than SUCCESS, so I don't understand what you are trying to accomplish here. It seems like this function should return FAILURE if any of the libraries fail to load.

@@ +694,2 @@
>    MOZLOAD("plds4");
>    MOZLOAD("nssutil3");

I don't understand why plds4 and nssutil3 are treated differently than the other NSS libraries.

@@ +696,2 @@
>    MOZLOAD("ssl3");
>    MOZLOAD("smime3");

Why do we need to load ssl3, smime3, and nssckbi?

@@ +800,5 @@
>    setup_sqlite_functions(sqlite_handle);
> +  return SUCCESS;
> +}
> +
> +static int loadNSSLibs(const char *apkName)

return type on separate line.
static nsresult (or whatever you rename nsresult to).

@@ +802,5 @@
> +}
> +
> +static int loadNSSLibs(const char *apkName)
> +{
> +  chdir(getenv("GRE_HOME"));

Somebody else should review the linking stuff, but using chdir() seems bogus to me.

::: mozglue/android/NSSBridge.cpp
@@ +49,5 @@
> +    LOG("missing handle\n");
> +    return FAILURE;
> +  }
> +#define GETFUNC(name) f_ ## name = (name ## _t) __wrap_dlsym(nss_handle, #name)
> +  GETFUNC(NSS_Initialize);

This code would be easier to read if you put the "if (!X) return FAILURE;" into the macro.

@@ +101,5 @@
> +  return SUCCESS;
> +}
> +
> +/* Throws the current NSS error. */
> +static void throwError(JNIEnv* jenv, const char * funcString) {

Put return type on a seperate line.

@@ +104,5 @@
> +/* Throws the current NSS error. */
> +static void throwError(JNIEnv* jenv, const char * funcString) {
> +    char *msg;
> +
> +    PRErrorCode  perr      = f_PR_GetError();

remove unnecessary whitespace between the identifiers in the above line.

@@ +108,5 @@
> +    PRErrorCode  perr      = f_PR_GetError();
> +    char * errString = f_PR_ErrorToString(perr, 0);
> +    asprintf(&msg, "%s returned error %d: %s\n", funcString, perr, errString);
> +    LOG("Throwing error: %s\n", msg);
> +    f_PR_Free(errString);

You are not supposed to free strings returned by PR_ErrorToString.

@@ +169,5 @@
> +}
> +
> +
> +/* Encrypts or decrypts a string. result should be freed with free() when done */
> +SECStatus doCrypto(JNIEnv* jenv, const char *path, const char *value, char** result, bool encrypt) {

Return type on seperate line, brace on separate line.

@@ +171,5 @@
> +
> +/* Encrypts or decrypts a string. result should be freed with free() when done */
> +SECStatus doCrypto(JNIEnv* jenv, const char *path, const char *value, char** result, bool encrypt) {
> +    SECStatus rv = SECFailure;
> +    PK11SlotInfo *slot = 0;

Do not initialize rv or slot here. (If you do, the compiler will not give you useful warnings about them being initialized if there were logic problems in the code below.)

@@ +238,5 @@
> +        throwError(jenv, "PK11SDR_Decrypt");
> +        goto done;
> +      }
> +
> +      *result = (char *)malloc(reply.len * sizeof(char));

sizeof(char) is always 1.

@@ +255,5 @@
> +
> +/*
> + * Base64 encodes the data passed in. The caller must deallocate _retval using free();
> + */
> +SECStatus encode(const unsigned char *data, PRInt32 dataLen, char **_retval) {

return type on separate line, brace on separate line.

@@ +278,5 @@
> +
> +/*
> + * Base64 decodes the data passed in. The caller must deallocate result using free();
> + */
> +SECStatus decode(const char *data, unsigned char **result, PRInt32 *length) {

return type, brace.

@@ +298,5 @@
> +    rv = SECFailure;
> +
> +  *length = (len*3)/4 - adjust;
> +
> +  *result = (unsigned char*)malloc((size_t)len * sizeof(char));

sizeof(char) is always 1.

I recommend:

*result = decoded ? strcpy(decoded) : NULL;
*length = *result ? strlen(*result) : 0;

if (decoded) {
    f_PR_Free(decoded);
}

return *result ? SUCCESS : FAILURE;

Then you can delete the calculation of adjust.

Comment 39

[Wesley Johnston (:wesj)]

Attachment: Patch for checkin.

Comment 40

[Wesley Johnston (:wesj)]

Attachment: Patch for checkin

Grr. This is the right one.

Comment 41

[Wesley Johnston (:wesj)]

Attachment: Tests

Didn't realize I didn't have an r+ here. Fixed the nits (and updated for all the other changes that have happened).

Comment 42

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c82cb6eb28

Comment 43

[Wesley Johnston (:wesj)]

Attachment: Follow up

bustage fix

Comment 44

[Wesley Johnston (:wesj)]

Comment on attachment 604129 [details] [diff] [review]
Follow up

Changing this because I need it now!

Comment 45

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95ab5c738512

Comment 46

[Matt Brubeck (:mbrubeck)]

Backed out on inbound to investigate XUL Fennec crashes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5940fe8d1af9

Comment 47

[Wesley Johnston (:wesj)]

Attachment: Patch for checkin

Just refactored to go below the patches from bug 732069.

Comment 48

[Wesley Johnston (:wesj)]

Attachment: Fixes

And this fixes the XUL startup crash I was seeing. There's... more than one error here. I forgot to update GeckoAppShell for XUL fennec while doing this. I also didn't realize that we would need to load more libraries in APKOpen for this to not crash when we use the old linker.

In addition, calling simple_linker_init twice is not legal. I just protected it with a boolean flag here. I also wrapped the initialization of lib_mapping (although I don't know that that is needed?), and reinserted the jShouldExtract bits, that I'm going to remove in bug 732069.

Pushing this to try and will have it report here. Asking for review now to speed up the process.

Comment 49

[Wesley Johnston (:wesj)]

Whoops. And I forgot to add the bug to my push:

https://tbpl.mozilla.org/?tree=Try&rev=a8357567362f

Comment 50

[Wesley Johnston (:wesj)]

And now I try again with the missing semicolon:
https://hg.mozilla.org/try/rev/40d1653cd196

Comment 51

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5031b59e031d
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eae96792a75

landed again, with tests and follow up fix folded (since without it we don't run).

Comment 52

[Matt Brubeck (:mbrubeck)]

Backed out temporarily because of broken Android builds: 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a536d22f312c

Comment 53

[Wesley Johnston (:wesj)]

Backout was caused by a mistake in my unbitrotting of the tests. Relanded
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9dc897118f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d643d069d11

Comment 54

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/9d643d069d11
https://hg.mozilla.org/mozilla-central/rev/c9dc897118f2

Comment 55

[Ian Melven :imelven]

Marking complete, as bsmith has reviewed the patches.