sqlcipher / sqlcipher-android-tests

A collection of tests that can be run on an emulator or device to verify SQLCipher for Android.
Other
94 stars 65 forks source link

DO NOT MERGE: Reproduce CRASH on null password char array #12

Closed brodycj closed 8 years ago

brodycj commented 8 years ago

The fix should be VERY easy. My first choice would be to attempt to open the database without a password:

index 97262f1..d393d8b 100644
--- a/src/net/sqlcipher/database/SQLiteDatabase.java
+++ b/src/net/sqlcipher/database/SQLiteDatabase.java
@@ -2329,7 +2329,9 @@ public class SQLiteDatabase extends SQLiteClosable {
             databaseHook.preKey(this);
         }

-        native_key(password);
+        if (password != null) {
+            native_key(password);
+        }

         if(databaseHook != null){
             databaseHook.postKey(this);

The alternative would be to throw something like an IllegalArgumentException.

Also if I open a database with a null password String I would get a NullPointerException:

        try {
            String nullPasswordString = null;
            database = SQLiteDatabase.openOrCreateDatabase(unencryptedDatabase, nullPasswordString, null);

            Log.e(ZeteticApplication.TAG, "BEHAVIOR CHANGED please update this test and check results of a raw SELECT query");
            return false;
        } catch (NullPointerException e) {
            // ACTUAL:
            Log.v(ZeteticApplication.TAG, "IGNORED: null pointer exception when opening database with null String password", e);
        } catch (Exception e) {
            Log.e(ZeteticApplication.TAG, "NOT EXPECTED: exception", e);
            return false;
        }

Again my preference would be to attempt to open the database without an encryption password. The alternative would be to throw an IllegalArgumentException. The solution would be straightforward; the challenge is to test all cases where the user may pass a null password String.

I would be happy to raise PRs with the fixes along with additional testing over the few weeks.

developernotes commented 8 years ago

Hi @brodybits

Thanks for catching this, we will merge it in prepare a new release. Thanks again!