8827 - discussion

* validate urls before changing anything in Provider.define()
* save private key and vpn cert after login/signup

7 files changed, 77 insertions, 17 deletions

diff --git a/app/src/insecure/java/se/leap/bitmaskclient/ProviderApiManager.java b/app/src/insecure/java/se/leap/bitmaskclient/ProviderApiManager.java
index d57dfe6d..1c5247c0 100644
--- a/app/src/insecure/java/se/leap/bitmaskclient/ProviderApiManager.java
+++ b/app/src/insecure/java/se/leap/bitmaskclient/ProviderApiManager.java
@@ -52,10 +52,12 @@ import static se.leap.bitmaskclient.Constants.BROADCAST_RESULT_KEY;
 import static se.leap.bitmaskclient.Constants.PROVIDER_KEY;
 import static se.leap.bitmaskclient.Constants.PROVIDER_VPN_CERTIFICATE;
 import static se.leap.bitmaskclient.DownloadFailedDialog.DOWNLOAD_ERRORS.ERROR_CERTIFICATE_PINNING;
+import static se.leap.bitmaskclient.DownloadFailedDialog.DOWNLOAD_ERRORS.ERROR_CORRUPTED_PROVIDER_JSON;
 import static se.leap.bitmaskclient.ProviderAPI.ERRORS;
 import static se.leap.bitmaskclient.R.string.certificate_error;
 import static se.leap.bitmaskclient.R.string.malformed_url;
 import static se.leap.bitmaskclient.R.string.warning_corrupted_provider_cert;
+import static se.leap.bitmaskclient.R.string.warning_corrupted_provider_details;
 
 /**
  * Created by cyberta on 04.01.18.
@@ -145,7 +147,11 @@ public class ProviderApiManager extends ProviderApiManagerBase {
         try {
             JSONObject providerJson = new JSONObject(providerDotJsonString);
 
-            provider.define(providerJson);
+            if (provider.define(providerJson)) {
+                result.putBoolean(BROADCAST_RESULT_KEY, true);
+            } else {
+                return setErrorResult(result, warning_corrupted_provider_details, ERROR_CORRUPTED_PROVIDER_JSON.toString());
+            }
 
             result.putBoolean(BROADCAST_RESULT_KEY, true);
         } catch (JSONException e) {
diff --git a/app/src/main/java/se/leap/bitmaskclient/ConfigHelper.java b/app/src/main/java/se/leap/bitmaskclient/ConfigHelper.java
index 329fd543..f8204b20 100644
--- a/app/src/main/java/se/leap/bitmaskclient/ConfigHelper.java
+++ b/app/src/main/java/se/leap/bitmaskclient/ConfigHelper.java
@@ -281,6 +281,7 @@ public class ConfigHelper {
             provider.setMainUrl(new URL(preferences.getString(Provider.MAIN_URL, "")));
             provider.define(new JSONObject(preferences.getString(Provider.KEY, "")));
             provider.setCaCert(preferences.getString(Provider.CA_CERT, ""));
+            provider.setCaCertFingerprint(preferences.getString(Provider.CA_CERT_FINGERPRINT, ""));
             provider.setVpnCertificate(preferences.getString(PROVIDER_VPN_CERTIFICATE, ""));
             provider.setPrivateKey(preferences.getString(PROVIDER_PRIVATE_KEY, ""));
         } catch (MalformedURLException | JSONException e) {
@@ -290,6 +291,10 @@ public class ConfigHelper {
         return provider;
     }
 
+    public String getFromPersistedProvider(String toFetch, String providerDomain, SharedPreferences preferences) {
+        return preferences.getString(toFetch + "." + providerDomain, "");
+    }
+
     public static String getProviderName(String provider) {
         return getProviderName(null, provider);
     }
diff --git a/app/src/main/java/se/leap/bitmaskclient/EipFragment.java b/app/src/main/java/se/leap/bitmaskclient/EipFragment.java
index b4c7a7de..41d9ff04 100644
--- a/app/src/main/java/se/leap/bitmaskclient/EipFragment.java
+++ b/app/src/main/java/se/leap/bitmaskclient/EipFragment.java
@@ -63,6 +63,7 @@ import static android.view.View.VISIBLE;
 import static de.blinkt.openvpn.core.ConnectionStatus.LEVEL_NONETWORK;
 import static se.leap.bitmaskclient.Constants.BROADCAST_EIP_EVENT;
 import static se.leap.bitmaskclient.Constants.BROADCAST_PROVIDER_API_EVENT;
+import static se.leap.bitmaskclient.Constants.BROADCAST_RESULT_CODE;
 import static se.leap.bitmaskclient.Constants.BROADCAST_RESULT_KEY;
 import static se.leap.bitmaskclient.Constants.EIP_ACTION_CHECK_CERT_VALIDITY;
 import static se.leap.bitmaskclient.Constants.EIP_ACTION_START;
@@ -266,7 +267,7 @@ public class EipFragment extends Fragment implements Observer {
     }
 
     private boolean canStartEIP() {
-        boolean certificateExists = !provider.hasVpnCertificate();
+        boolean certificateExists = provider.hasVpnCertificate();
         boolean isAllowedAnon = provider.allowsAnonymous();
         return (isAllowedAnon || certificateExists) && !eipStatus.isConnected() && !eipStatus.isConnecting();
     }
@@ -463,7 +464,7 @@ public class EipFragment extends Fragment implements Observer {
                 return;
             }
 
-            int resultCode = intent.getIntExtra(BROADCAST_RESULT_KEY, -1);
+            int resultCode = intent.getIntExtra(BROADCAST_RESULT_CODE, -1);
             Bundle resultData = intent.getParcelableExtra(BROADCAST_RESULT_KEY);
             switch (action) {
                 case BROADCAST_EIP_EVENT:
diff --git a/app/src/main/java/se/leap/bitmaskclient/Provider.java b/app/src/main/java/se/leap/bitmaskclient/Provider.java
index 4d608222..e53dd4fb 100644
--- a/app/src/main/java/se/leap/bitmaskclient/Provider.java
+++ b/app/src/main/java/se/leap/bitmaskclient/Provider.java
@@ -141,9 +141,27 @@ public final class Provider implements Parcelable {
         }
     }
 
-    public void define(JSONObject providerJson) {
-        definition = providerJson;
-        parseDefinition(definition);
+    public boolean define(JSONObject providerJson) {
+       /*
+        * fix against "api_uri": "https://calyx.net.malicious.url.net:4430",
+        * This method aims to prevent attacks where the provider.json file got manipulated by a third party.
+        * The main url should not change.
+        */
+
+        try {
+            String providerApiUrl = providerJson.getString(Provider.API_URL);
+            String providerDomain = providerJson.getString(Provider.DOMAIN);
+            if (getMainUrlString().contains(providerDomain) && providerApiUrl.contains(providerDomain  + ":")) {
+                definition = providerJson;
+                parseDefinition(definition);
+                return true;
+            } else {
+                return false;
+            }
+        } catch (JSONException e) {
+            e.printStackTrace();
+            return false;
+        }
     }
 
     protected JSONObject getDefinition() {
diff --git a/app/src/main/java/se/leap/bitmaskclient/ProviderApiManagerBase.java b/app/src/main/java/se/leap/bitmaskclient/ProviderApiManagerBase.java
index a9321a9c..5fe6ed05 100644
--- a/app/src/main/java/se/leap/bitmaskclient/ProviderApiManagerBase.java
+++ b/app/src/main/java/se/leap/bitmaskclient/ProviderApiManagerBase.java
@@ -87,7 +87,6 @@ import static se.leap.bitmaskclient.ProviderAPI.SIGN_UP;
 import static se.leap.bitmaskclient.ProviderAPI.SUCCESSFUL_LOGIN;
 import static se.leap.bitmaskclient.ProviderAPI.SUCCESSFUL_LOGOUT;
 import static se.leap.bitmaskclient.ProviderAPI.SUCCESSFUL_SIGNUP;
-import static se.leap.bitmaskclient.ProviderAPI.UPDATE_PROGRESSBAR;
 import static se.leap.bitmaskclient.ProviderAPI.UPDATE_PROVIDER_DETAILS;
 import static se.leap.bitmaskclient.R.string.certificate_error;
 import static se.leap.bitmaskclient.R.string.error_io_exception_user_message;
@@ -665,6 +664,8 @@ public abstract class ProviderApiManagerBase {
             provider.setCaCert(getPersistedProviderCA(providerDomain));
             provider.define(getPersistedProviderDefinition(providerDomain));
             provider.setCaCertFingerprint(getPersistedCaCertFingerprint(providerDomain));
+            provider.setPrivateKey(getPersistedPrivateKey(providerDomain));
+            provider.setVpnCertificate(getPersistedVPNCertificate(providerDomain));
         }
     }
 
@@ -707,10 +708,6 @@ public abstract class ProviderApiManagerBase {
                 return setErrorResult(result, warning_corrupted_provider_cert, ERROR_CERTIFICATE_PINNING.toString());
             }
 
-            if (!hasApiUrlExpectedDomain(providerDefinition, mainUrl)){
-                return setErrorResult(result, warning_corrupted_provider_details, ERROR_CORRUPTED_PROVIDER_JSON.toString());
-            }
-
             if (!canConnect(caCert, providerDefinition, result)) {
                 return result;
             }
@@ -797,6 +794,24 @@ public abstract class ProviderApiManagerBase {
         return "";
     }
 
+    protected String getPersistedPrivateKey(String providerDomain) {
+        try {
+            return getPersistedProviderDefinition(providerDomain).getString(PROVIDER_PRIVATE_KEY);
+        } catch (JSONException e) {
+            e.printStackTrace();
+        }
+        return "";
+    }
+
+    protected String getPersistedVPNCertificate(String providerDomain) {
+        try {
+            return getPersistedProviderDefinition(providerDomain).getString(PROVIDER_VPN_CERTIFICATE);
+        } catch (JSONException e) {
+            e.printStackTrace();
+        }
+        return "";
+    }
+
     protected JSONObject getPersistedProviderDefinition(String providerDomain) {
         try {
             return new JSONObject(preferences.getString(Provider.KEY + "." + providerDomain, ""));
@@ -806,6 +821,10 @@ public abstract class ProviderApiManagerBase {
         }
     }
 
+    protected String getFromPersistedProvider(String toFetch, String providerDomain) {
+        return preferences.getString(toFetch + "." + providerDomain, "");
+    }
+
     protected String getPersistedProviderCA(String providerDomain) {
         return preferences.getString(Provider.CA_CERT + "." + providerDomain, "");
     }
diff --git a/app/src/main/java/se/leap/bitmaskclient/ProviderCredentialsBaseActivity.java b/app/src/main/java/se/leap/bitmaskclient/ProviderCredentialsBaseActivity.java
index 47a45a74..7714e979 100644
--- a/app/src/main/java/se/leap/bitmaskclient/ProviderCredentialsBaseActivity.java
+++ b/app/src/main/java/se/leap/bitmaskclient/ProviderCredentialsBaseActivity.java
@@ -40,6 +40,7 @@ import static se.leap.bitmaskclient.Constants.BROADCAST_RESULT_CODE;
 import static se.leap.bitmaskclient.Constants.BROADCAST_RESULT_KEY;
 import static se.leap.bitmaskclient.Constants.CREDENTIALS_PASSWORD;
 import static se.leap.bitmaskclient.Constants.CREDENTIALS_USERNAME;
+import static se.leap.bitmaskclient.Constants.PROVIDER_KEY;
 import static se.leap.bitmaskclient.ProviderAPI.DOWNLOAD_CERTIFICATE;
 import static se.leap.bitmaskclient.ProviderAPI.LOG_IN;
 import static se.leap.bitmaskclient.ProviderAPI.SIGN_UP;
@@ -201,7 +202,8 @@ public abstract class ProviderCredentialsBaseActivity extends ConfigWizardBaseAc
         ProviderAPICommand.execute(this, SIGN_UP, parameters, provider);
     }
 
-    void downloadVpnCertificate() {
+    void downloadVpnCertificate(Provider handledProvider) {
+        provider = handledProvider;
         ProviderAPICommand.execute(this, DOWNLOAD_CERTIFICATE, provider);
     }
 
@@ -361,7 +363,8 @@ public abstract class ProviderCredentialsBaseActivity extends ConfigWizardBaseAc
         hideProgressBar();
     }
 
-    private void successfullyFinished() {
+    private void successfullyFinished(Provider handledProvider) {
+        provider = handledProvider;
         Intent resultData = new Intent();
         resultData.putExtra(Provider.KEY, provider);
         setResult(RESULT_OK, resultData);
@@ -379,10 +382,13 @@ public abstract class ProviderCredentialsBaseActivity extends ConfigWizardBaseAc
             }
 
             int resultCode = intent.getIntExtra(BROADCAST_RESULT_CODE, -1);
+            Bundle resultData = intent.getParcelableExtra(BROADCAST_RESULT_KEY);
+            Provider handledProvider = resultData.getParcelable(PROVIDER_KEY);
+
             switch (resultCode) {
                 case ProviderAPI.SUCCESSFUL_SIGNUP:
                 case ProviderAPI.SUCCESSFUL_LOGIN:
-                    downloadVpnCertificate();
+                    downloadVpnCertificate(handledProvider);
                     break;
                 case ProviderAPI.FAILED_LOGIN:
                 case ProviderAPI.FAILED_SIGNUP:
@@ -390,7 +396,7 @@ public abstract class ProviderCredentialsBaseActivity extends ConfigWizardBaseAc
                     break;
 
                 case ProviderAPI.CORRECTLY_DOWNLOADED_CERTIFICATE:
-                    successfullyFinished();
+                    successfullyFinished(handledProvider);
                     //activity.eip_fragment.updateEipService();
                     break;
                 case ProviderAPI.INCORRECTLY_DOWNLOADED_CERTIFICATE:
diff --git a/app/src/production/java/se/leap/bitmaskclient/ProviderApiManager.java b/app/src/production/java/se/leap/bitmaskclient/ProviderApiManager.java
index 839c5a5d..5317118b 100644
--- a/app/src/production/java/se/leap/bitmaskclient/ProviderApiManager.java
+++ b/app/src/production/java/se/leap/bitmaskclient/ProviderApiManager.java
@@ -36,9 +36,11 @@ import static android.text.TextUtils.isEmpty;
 import static se.leap.bitmaskclient.Constants.BROADCAST_RESULT_KEY;
 import static se.leap.bitmaskclient.Constants.PROVIDER_VPN_CERTIFICATE;
 import static se.leap.bitmaskclient.DownloadFailedDialog.DOWNLOAD_ERRORS.ERROR_CERTIFICATE_PINNING;
+import static se.leap.bitmaskclient.DownloadFailedDialog.DOWNLOAD_ERRORS.ERROR_CORRUPTED_PROVIDER_JSON;
 import static se.leap.bitmaskclient.ProviderAPI.ERRORS;
 import static se.leap.bitmaskclient.R.string.malformed_url;
 import static se.leap.bitmaskclient.R.string.warning_corrupted_provider_cert;
+import static se.leap.bitmaskclient.R.string.warning_corrupted_provider_details;
 
 /**
  * Implements the logic of the provider api http requests. The methods of this class need to be called from
@@ -124,9 +126,12 @@ public class ProviderApiManager extends ProviderApiManagerBase {
 
         try {
             JSONObject providerJson = new JSONObject(providerDotJsonString);
-            provider.define(providerJson);
+            if (provider.define(providerJson)) {
+                result.putBoolean(BROADCAST_RESULT_KEY, true);
+            } else {
+                return setErrorResult(result, warning_corrupted_provider_details, ERROR_CORRUPTED_PROVIDER_JSON.toString());
+            }
 
-            result.putBoolean(BROADCAST_RESULT_KEY, true);
         } catch (JSONException e) {
             String reason_to_fail = pickErrorMessage(providerDotJsonString);
             result.putString(ERRORS, reason_to_fail);