Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getting error in Google Pre-launch report #63

Closed
Tanuja0049 opened this issue Jun 22, 2021 · 8 comments
Closed

getting error in Google Pre-launch report #63

Tanuja0049 opened this issue Jun 22, 2021 · 8 comments

Comments

@Tanuja0049
Copy link

Version of rn-secure-storage
2.0.6

Version of react-native
0.63.2

Platforms you faced the error (IOS or Android or both?)
Android

Details:

We are using this library to store user details at app side. The app gets deployed successfully in Google play console with internal/closed track. But we are getting below error in Pre-launch report:
Unsafe Cipher Mode
Your app contains a less secure encryption mode.
com.securepreferences.SecurePreferencesOld.encrypt

Reference link related to issue:
https://support.google.com/faqs/answer/10046138

We checked and found that the native code in library is using 'AES/ECB/PKCS5Padding' encryption which seems to cause the issue.

@talut
Copy link
Owner

talut commented Jun 22, 2021

Thanks for the issue. I'm aware about these problems. Currently I couldn't spare time to maintain my packages. But I'll handle all of these problems as soon as possible.

@mlecoq
Copy link

mlecoq commented Nov 14, 2021

@talut any news about this issue ?

@rakiop
Copy link

rakiop commented Nov 24, 2021

I had the same issue and end up fixing the code locally. I tried to create a pull request but I can't push my branch. If anyone is interested here are patches to fix the issue:

  1. migrate to more secure AES
From: rakiop <rakiop@o2.pl>
Date: Wed, 24 Nov 2021 12:06:29 +0100
Subject: [PATCH] Change android AES to AES/GCM/NoPadding

---
 .../rnsecurestorage/Constants.java            |  3 ++-
 .../rnsecurestorage/RNKeyStore.java           | 27 ++++++++++++++-----
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/android/src/main/java/com/taluttasgiran/rnsecurestorage/Constants.java b/android/src/main/java/com/taluttasgiran/rnsecurestorage/Constants.java
index ef46392..319f2ad 100644
--- a/android/src/main/java/com/taluttasgiran/rnsecurestorage/Constants.java
+++ b/android/src/main/java/com/taluttasgiran/rnsecurestorage/Constants.java
@@ -6,11 +6,12 @@ public class Constants {
     public static final String KEYSTORE_PROVIDER_3 = "AndroidOpenSSL";
 
     public static final String RSA_ALGORITHM = "RSA/ECB/PKCS1Padding";
-    public static final String AES_ALGORITHM = "AES/ECB/PKCS5Padding";
+    public static final String AES_ALGORITHM = "AES/GCM/NoPadding";
 
     public static final String TAG = "RNSecureStorage";
 
     // Internal storage file
     public static final String SKS_KEY_FILENAME = "SKS_KEY_FILE";
     public static final String SKS_DATA_FILENAME = "SKS_DATA_FILE";
+    public static final String AES_IV_FILENAME = "__iv";
 }
\ No newline at end of file
diff --git a/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNKeyStore.java b/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNKeyStore.java
index 7c49d15..325796b 100644
--- a/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNKeyStore.java
+++ b/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNKeyStore.java
@@ -15,6 +15,7 @@ import java.security.KeyStore;
 import java.security.KeyStoreException;
 import java.security.PrivateKey;
 import java.security.PublicKey;
+import java.security.SecureRandom;
 import java.util.Calendar;
 
 import javax.crypto.Cipher;
@@ -23,10 +24,24 @@ import javax.crypto.CipherOutputStream;
 import javax.crypto.KeyGenerator;
 import javax.crypto.SecretKey;
 import javax.crypto.spec.SecretKeySpec;
+import javax.crypto.spec.IvParameterSpec;
 import javax.security.auth.x500.X500Principal;
 
 public class RNKeyStore {
 
+    private IvParameterSpec getIv(Context context) throws IOException {
+        byte[] iv;
+        if(Storage.exists(context, Constants.AES_IV_FILENAME)){
+            iv = Storage.readValues(context, Constants.AES_IV_FILENAME );
+        }
+        else {
+            iv = new byte[16];
+            new SecureRandom().nextBytes(iv);
+            Storage.writeValues(context, Constants.AES_IV_FILENAME, iv);
+        }
+        return new IvParameterSpec(iv);
+    }
+
     private PublicKey getOrCreatePublicKey(Context context, String alias) throws GeneralSecurityException, IOException {
         KeyStore keyStore = getKeyStore();
         keyStore.load(null);
@@ -61,9 +76,9 @@ public class RNKeyStore {
         return encryptCipherText(cipher, plainTextBytes);
     }
 
-    private byte[] encryptAesPlainText(SecretKey secretKey, String plainText) throws GeneralSecurityException, IOException {
+    private byte[] encryptAesPlainText(SecretKey secretKey, IvParameterSpec iv, String plainText) throws GeneralSecurityException, IOException {
         Cipher cipher = Cipher.getInstance(Constants.AES_ALGORITHM);
-        cipher.init(Cipher.ENCRYPT_MODE, secretKey);
+        cipher.init(Cipher.ENCRYPT_MODE, secretKey, iv);
         return encryptCipherText(cipher, plainText);
     }
 
@@ -100,7 +115,7 @@ public class RNKeyStore {
 
     public void setCipherText(Context context, String alias, String input) throws GeneralSecurityException, IOException {
         Storage.writeValues(context, Constants.SKS_DATA_FILENAME + alias,
-                encryptAesPlainText(getOrCreateSecretKey(context, alias), input));
+                encryptAesPlainText(getOrCreateSecretKey(context, alias), getIv(context), input));
     }
 
     private PrivateKey getPrivateKey(String alias) throws GeneralSecurityException, IOException {
@@ -115,9 +130,9 @@ public class RNKeyStore {
         return decryptCipherText(cipher, cipherTextBytes);
     }
 
-    private byte[] decryptAesCipherText(SecretKey secretKey, byte[] cipherTextBytes) throws GeneralSecurityException, IOException {
+    private byte[] decryptAesCipherText(SecretKey secretKey, IvParameterSpec iv, byte[] cipherTextBytes) throws GeneralSecurityException, IOException {
         Cipher cipher = Cipher.getInstance(Constants.AES_ALGORITHM);
-        cipher.init(Cipher.DECRYPT_MODE, secretKey);
+        cipher.init(Cipher.DECRYPT_MODE, secretKey, iv);
         return decryptCipherText(cipher, cipherTextBytes);
     }
 
@@ -142,7 +157,7 @@ public class RNKeyStore {
     public String getPlainText(Context context, String alias) throws GeneralSecurityException, IOException {
         SecretKey secretKey = getSymmetricKey(context, alias);
         byte[] cipherTextBytes = Storage.readValues(context, Constants.SKS_DATA_FILENAME + alias);
-        return new String(decryptAesCipherText(secretKey, cipherTextBytes), "UTF-8");
+        return new String(decryptAesCipherText(secretKey, getIv(context), cipherTextBytes), "UTF-8");
     }
 
     public boolean exists(Context context, String alias) throws IOException {
-- 
2.30.0.windows.1

  1. Remove secure-preferences library (that also uses less secure AES and fails GoogleConsole check)(technically this requires minSdkVersion to be at least 23 which ends up with always using the KeyStore, but this patch also updates to EncryptedSharedPreferences):
From 7301b43125566b93ca31de386b1cca382c3842a9 Mon Sep 17 00:00:00 2001
From: rakiop <rakiop@o2.pl>
Date: Thu, 25 Nov 2021 11:38:56 +0100
Subject: [PATCH 2/2] Remove connection to secure-preferences

---
 android/build.gradle                              |  4 ++--
 .../rnsecurestorage/RNSecureStorageModule.java    | 15 +++++++++++++--
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/android/build.gradle b/android/build.gradle
index 3b8f297..a9ca261 100644
--- a/android/build.gradle
+++ b/android/build.gradle
@@ -20,7 +20,7 @@ apply plugin: 'com.android.library'
 android {
     compileSdkVersion 28
     defaultConfig {
-        minSdkVersion 16
+        minSdkVersion 23
         targetSdkVersion 28
         versionCode 4
         versionName "2.0.7"
@@ -37,8 +37,8 @@ repositories {
 }
 
 dependencies {
-    implementation "com.scottyab:secure-preferences-lib:0.1.4"
     implementation "com.facebook.react:react-native:+"
+    implementation "androidx.security:security-crypto:1.0.0"
 }
 task copyDownloadableDepsToLibs(type: Copy) {
     from configurations.compile
diff --git a/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNSecureStorageModule.java b/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNSecureStorageModule.java
index d8d88d1..2f035a2 100644
--- a/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNSecureStorageModule.java
+++ b/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNSecureStorageModule.java
@@ -3,15 +3,18 @@ package com.taluttasgiran.rnsecurestorage;
 import android.content.SharedPreferences;
 import android.os.Build;
 import androidx.annotation.Nullable;
+import androidx.security.crypto.EncryptedSharedPreferences;
+import androidx.security.crypto.MasterKeys;
 import com.facebook.react.bridge.Promise;
 import com.facebook.react.bridge.ReactApplicationContext;
 import com.facebook.react.bridge.ReactContextBaseJavaModule;
 import com.facebook.react.bridge.ReactMethod;
 import com.facebook.react.bridge.ReadableMap;
 import com.facebook.react.uimanager.IllegalViewOperationException;
-import com.securepreferences.SecurePreferences;
 
 import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.security.GeneralSecurityException;
 import java.util.ArrayList;
 import java.util.Locale;
 
@@ -31,7 +34,15 @@ public class RNSecureStorageModule extends ReactContextBaseJavaModule {
         if (useKeystore()) {
             rnKeyStore = new RNKeyStore();
         } else {
-            prefs = new SecurePreferences(getReactApplicationContext(), (String) null, "e4b001df9a082298dd090bb7455c45d92fbd5ddd.xml");
+            try {
+                prefs = EncryptedSharedPreferences.create("e4b001df9a082298dd090bb7455c45d92fbd5dda.xml", 
+                    MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC),
+                    getReactApplicationContext(),
+                    EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
+                    EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM);
+            }catch(GeneralSecurityException|IOException e){
+                ;
+            }
         }
     }
 
-- 
2.30.0.windows.1

@cdraeger
Copy link

@talut Do you have any update on this? Could the scheme be updated to GCM also keeping the migration path in mind?

I believe this issue is quite severe considering that it is a secure storage library, and the chosen scheme is flagged as unsecure (which indeed is an error in every pre-publish report). Thanks!

@paveltar
Copy link

paveltar commented Dec 25, 2022

I had the same issue and end up fixing the code locally. I tried to create a pull request but I can't push my branch. If anyone is interested here are patches to fix the issue:

  1. migrate to more secure AES
From: rakiop <rakiop@o2.pl>
Date: Wed, 24 Nov 2021 12:06:29 +0100
Subject: [PATCH] Change android AES to AES/GCM/NoPadding

---
 .../rnsecurestorage/Constants.java            |  3 ++-
 .../rnsecurestorage/RNKeyStore.java           | 27 ++++++++++++++-----
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/android/src/main/java/com/taluttasgiran/rnsecurestorage/Constants.java b/android/src/main/java/com/taluttasgiran/rnsecurestorage/Constants.java
index ef46392..319f2ad 100644
--- a/android/src/main/java/com/taluttasgiran/rnsecurestorage/Constants.java
+++ b/android/src/main/java/com/taluttasgiran/rnsecurestorage/Constants.java
@@ -6,11 +6,12 @@ public class Constants {
     public static final String KEYSTORE_PROVIDER_3 = "AndroidOpenSSL";
 
     public static final String RSA_ALGORITHM = "RSA/ECB/PKCS1Padding";
-    public static final String AES_ALGORITHM = "AES/ECB/PKCS5Padding";
+    public static final String AES_ALGORITHM = "AES/GCM/NoPadding";
 
     public static final String TAG = "RNSecureStorage";
 
     // Internal storage file
     public static final String SKS_KEY_FILENAME = "SKS_KEY_FILE";
     public static final String SKS_DATA_FILENAME = "SKS_DATA_FILE";
+    public static final String AES_IV_FILENAME = "__iv";
 }
\ No newline at end of file
diff --git a/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNKeyStore.java b/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNKeyStore.java
index 7c49d15..325796b 100644
--- a/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNKeyStore.java
+++ b/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNKeyStore.java
@@ -15,6 +15,7 @@ import java.security.KeyStore;
 import java.security.KeyStoreException;
 import java.security.PrivateKey;
 import java.security.PublicKey;
+import java.security.SecureRandom;
 import java.util.Calendar;
 
 import javax.crypto.Cipher;
@@ -23,10 +24,24 @@ import javax.crypto.CipherOutputStream;
 import javax.crypto.KeyGenerator;
 import javax.crypto.SecretKey;
 import javax.crypto.spec.SecretKeySpec;
+import javax.crypto.spec.IvParameterSpec;
 import javax.security.auth.x500.X500Principal;
 
 public class RNKeyStore {
 
+    private IvParameterSpec getIv(Context context) throws IOException {
+        byte[] iv;
+        if(Storage.exists(context, Constants.AES_IV_FILENAME)){
+            iv = Storage.readValues(context, Constants.AES_IV_FILENAME );
+        }
+        else {
+            iv = new byte[16];
+            new SecureRandom().nextBytes(iv);
+            Storage.writeValues(context, Constants.AES_IV_FILENAME, iv);
+        }
+        return new IvParameterSpec(iv);
+    }
+
     private PublicKey getOrCreatePublicKey(Context context, String alias) throws GeneralSecurityException, IOException {
         KeyStore keyStore = getKeyStore();
         keyStore.load(null);
@@ -61,9 +76,9 @@ public class RNKeyStore {
         return encryptCipherText(cipher, plainTextBytes);
     }
 
-    private byte[] encryptAesPlainText(SecretKey secretKey, String plainText) throws GeneralSecurityException, IOException {
+    private byte[] encryptAesPlainText(SecretKey secretKey, IvParameterSpec iv, String plainText) throws GeneralSecurityException, IOException {
         Cipher cipher = Cipher.getInstance(Constants.AES_ALGORITHM);
-        cipher.init(Cipher.ENCRYPT_MODE, secretKey);
+        cipher.init(Cipher.ENCRYPT_MODE, secretKey, iv);
         return encryptCipherText(cipher, plainText);
     }
 
@@ -100,7 +115,7 @@ public class RNKeyStore {
 
     public void setCipherText(Context context, String alias, String input) throws GeneralSecurityException, IOException {
         Storage.writeValues(context, Constants.SKS_DATA_FILENAME + alias,
-                encryptAesPlainText(getOrCreateSecretKey(context, alias), input));
+                encryptAesPlainText(getOrCreateSecretKey(context, alias), getIv(context), input));
     }
 
     private PrivateKey getPrivateKey(String alias) throws GeneralSecurityException, IOException {
@@ -115,9 +130,9 @@ public class RNKeyStore {
         return decryptCipherText(cipher, cipherTextBytes);
     }
 
-    private byte[] decryptAesCipherText(SecretKey secretKey, byte[] cipherTextBytes) throws GeneralSecurityException, IOException {
+    private byte[] decryptAesCipherText(SecretKey secretKey, IvParameterSpec iv, byte[] cipherTextBytes) throws GeneralSecurityException, IOException {
         Cipher cipher = Cipher.getInstance(Constants.AES_ALGORITHM);
-        cipher.init(Cipher.DECRYPT_MODE, secretKey);
+        cipher.init(Cipher.DECRYPT_MODE, secretKey, iv);
         return decryptCipherText(cipher, cipherTextBytes);
     }
 
@@ -142,7 +157,7 @@ public class RNKeyStore {
     public String getPlainText(Context context, String alias) throws GeneralSecurityException, IOException {
         SecretKey secretKey = getSymmetricKey(context, alias);
         byte[] cipherTextBytes = Storage.readValues(context, Constants.SKS_DATA_FILENAME + alias);
-        return new String(decryptAesCipherText(secretKey, cipherTextBytes), "UTF-8");
+        return new String(decryptAesCipherText(secretKey, getIv(context), cipherTextBytes), "UTF-8");
     }
 
     public boolean exists(Context context, String alias) throws IOException {
-- 
2.30.0.windows.1
  1. Remove secure-preferences library (that also uses less secure AES and fails GoogleConsole check)(technically this requires minSdkVersion to be at least 23 which ends up with always using the KeyStore, but this patch also updates to EncryptedSharedPreferences):
From 7301b43125566b93ca31de386b1cca382c3842a9 Mon Sep 17 00:00:00 2001
From: rakiop <rakiop@o2.pl>
Date: Thu, 25 Nov 2021 11:38:56 +0100
Subject: [PATCH 2/2] Remove connection to secure-preferences

---
 android/build.gradle                              |  4 ++--
 .../rnsecurestorage/RNSecureStorageModule.java    | 15 +++++++++++++--
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/android/build.gradle b/android/build.gradle
index 3b8f297..a9ca261 100644
--- a/android/build.gradle
+++ b/android/build.gradle
@@ -20,7 +20,7 @@ apply plugin: 'com.android.library'
 android {
     compileSdkVersion 28
     defaultConfig {
-        minSdkVersion 16
+        minSdkVersion 23
         targetSdkVersion 28
         versionCode 4
         versionName "2.0.7"
@@ -37,8 +37,8 @@ repositories {
 }
 
 dependencies {
-    implementation "com.scottyab:secure-preferences-lib:0.1.4"
     implementation "com.facebook.react:react-native:+"
+    implementation "androidx.security:security-crypto:1.0.0"
 }
 task copyDownloadableDepsToLibs(type: Copy) {
     from configurations.compile
diff --git a/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNSecureStorageModule.java b/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNSecureStorageModule.java
index d8d88d1..2f035a2 100644
--- a/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNSecureStorageModule.java
+++ b/android/src/main/java/com/taluttasgiran/rnsecurestorage/RNSecureStorageModule.java
@@ -3,15 +3,18 @@ package com.taluttasgiran.rnsecurestorage;
 import android.content.SharedPreferences;
 import android.os.Build;
 import androidx.annotation.Nullable;
+import androidx.security.crypto.EncryptedSharedPreferences;
+import androidx.security.crypto.MasterKeys;
 import com.facebook.react.bridge.Promise;
 import com.facebook.react.bridge.ReactApplicationContext;
 import com.facebook.react.bridge.ReactContextBaseJavaModule;
 import com.facebook.react.bridge.ReactMethod;
 import com.facebook.react.bridge.ReadableMap;
 import com.facebook.react.uimanager.IllegalViewOperationException;
-import com.securepreferences.SecurePreferences;
 
 import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.security.GeneralSecurityException;
 import java.util.ArrayList;
 import java.util.Locale;
 
@@ -31,7 +34,15 @@ public class RNSecureStorageModule extends ReactContextBaseJavaModule {
         if (useKeystore()) {
             rnKeyStore = new RNKeyStore();
         } else {
-            prefs = new SecurePreferences(getReactApplicationContext(), (String) null, "e4b001df9a082298dd090bb7455c45d92fbd5ddd.xml");
+            try {
+                prefs = EncryptedSharedPreferences.create("e4b001df9a082298dd090bb7455c45d92fbd5dda.xml", 
+                    MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC),
+                    getReactApplicationContext(),
+                    EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
+                    EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM);
+            }catch(GeneralSecurityException|IOException e){
+                ;
+            }
         }
     }
 
-- 
2.30.0.windows.1

Hey, can you confirm if this method doesn't break the access to already stored data and migration is safe?

I'm getting: "mac check in GCM failed" error after the migration to your code.

@abstractk
Copy link

Hi! Is there an update on this?

@McBlaise
Copy link

This is still an issue, any updates?

@talut
Copy link
Owner

talut commented Dec 23, 2023

Update

I just released v3.0.0 I hope it'll solve your problems.

Please check the new version and Readme file. It also includes a few new features as well.

It's tested with the newest RN versions.

Disclaimer

https://github.com/talut/rn-secure-storage?tab=readme-ov-file#disclaimer

@talut talut closed this as completed Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants