From 0b181743a3d6b1423e112b17a400b5ac4ac09bcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Sun, 20 Sep 2015 22:42:50 +0200 Subject: Keyservers: Dont follow redirects, pin pgp.mit.edu, check for pinned cert on add (OKC-01-018) --- .../keychain/KeychainApplication.java | 5 +- .../keychain/keyimport/HkpKeyserver.java | 6 +- .../keychain/ui/SettingsKeyserverFragment.java | 22 +----- .../ui/dialog/AddEditKeyserverDialogFragment.java | 80 +++++++++++++++++---- .../keychain/util/TlsHelper.java | 82 +++++++--------------- 5 files changed, 101 insertions(+), 94 deletions(-) (limited to 'OpenKeychain/src/main/java/org') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/KeychainApplication.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/KeychainApplication.java index 311ef2d3b..45d81749a 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/KeychainApplication.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/KeychainApplication.java @@ -91,14 +91,15 @@ public class KeychainApplication extends Application { } brandGlowEffect(getApplicationContext(), - FormattingUtils.getColorFromAttr(getApplicationContext(), R.attr.colorPrimary)); + FormattingUtils.getColorFromAttr(getApplicationContext(), R.attr.colorPrimary)); setupAccountAsNeeded(this); // Update keyserver list as needed Preferences.getPreferences(this).upgradePreferences(this); - TlsHelper.addStaticCA("pool.sks-keyservers.net", getAssets(), "sks-keyservers.netCA.cer"); + TlsHelper.addPinnedCertificate("hkps.pool.sks-keyservers.net", getAssets(), "hkps.pool.sks-keyservers.net.CA.cer"); + TlsHelper.addPinnedCertificate("pgp.mit.edu", getAssets(), "pgp.mit.edu.cer"); TemporaryStorageProvider.cleanUp(this); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/HkpKeyserver.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/HkpKeyserver.java index 558b8ce7d..5683decdf 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/HkpKeyserver.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/HkpKeyserver.java @@ -204,11 +204,15 @@ public class HkpKeyserver extends Keyserver { OkHttpClient client = new OkHttpClient(); try { - TlsHelper.pinCertificateIfNecessary(client, url); + TlsHelper.usePinnedCertificateIfAvailable(client, url); } catch (TlsHelper.TlsHelperException e) { Log.w(Constants.TAG, e); } + // don't follow any redirects + client.setFollowRedirects(false); + client.setFollowSslRedirects(false); + if (proxy != null) { client.setProxy(proxy); client.setConnectTimeout(30000, TimeUnit.MILLISECONDS); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SettingsKeyserverFragment.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SettingsKeyserverFragment.java index d8edbe4f8..5a8ab36bc 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SettingsKeyserverFragment.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SettingsKeyserverFragment.java @@ -155,7 +155,7 @@ public class SettingsKeyserverFragment extends Fragment implements RecyclerItemC data.getBoolean(AddEditKeyserverDialogFragment.MESSAGE_VERIFIED); if (verified) { Notify.create(getActivity(), - R.string.add_keyserver_verified, Notify.Style.OK).show(); + R.string.add_keyserver_connection_verified, Notify.Style.OK).show(); } else { Notify.create(getActivity(), R.string.add_keyserver_without_verification, @@ -177,26 +177,6 @@ public class SettingsKeyserverFragment extends Fragment implements RecyclerItemC } break; } - case AddEditKeyserverDialogFragment.MESSAGE_VERIFICATION_FAILED: { - AddEditKeyserverDialogFragment.FailureReason failureReason = - (AddEditKeyserverDialogFragment.FailureReason) data.getSerializable( - AddEditKeyserverDialogFragment.MESSAGE_FAILURE_REASON); - switch (failureReason) { - case CONNECTION_FAILED: { - Notify.create(getActivity(), - R.string.add_keyserver_connection_failed, - Notify.Style.ERROR).show(); - break; - } - case INVALID_URL: { - Notify.create(getActivity(), - R.string.add_keyserver_invalid_url, - Notify.Style.ERROR).show(); - break; - } - } - break; - } } } }; diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/AddEditKeyserverDialogFragment.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/AddEditKeyserverDialogFragment.java index 47bc7dfda..3d96f3c6d 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/AddEditKeyserverDialogFragment.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/AddEditKeyserverDialogFragment.java @@ -24,6 +24,7 @@ import java.net.URI; import java.net.URISyntaxException; import android.app.Activity; +import android.support.design.widget.TextInputLayout; import android.support.v7.app.AlertDialog; import android.app.Dialog; import android.app.ProgressDialog; @@ -44,6 +45,7 @@ import android.view.inputmethod.EditorInfo; import android.view.inputmethod.InputMethodManager; import android.widget.Button; import android.widget.CheckBox; +import android.widget.CompoundButton; import android.widget.EditText; import android.widget.TextView; import android.widget.TextView.OnEditorActionListener; @@ -54,6 +56,7 @@ import com.squareup.okhttp.Request; import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.R; import org.sufficientlysecure.keychain.keyimport.HkpKeyserver; +import org.sufficientlysecure.keychain.ui.util.Notify; import org.sufficientlysecure.keychain.util.Log; import org.sufficientlysecure.keychain.util.Preferences; import org.sufficientlysecure.keychain.util.TlsHelper; @@ -68,11 +71,9 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On private static final String ARG_KEYSERVER = "arg_keyserver"; public static final int MESSAGE_OKAY = 1; - public static final int MESSAGE_VERIFICATION_FAILED = 2; public static final String MESSAGE_KEYSERVER = "new_keyserver"; public static final String MESSAGE_VERIFIED = "verified"; - public static final String MESSAGE_FAILURE_REASON = "failure_reason"; public static final String MESSAGE_KEYSERVER_DELETED = "keyserver_deleted"; public static final String MESSAGE_DIALOG_ACTION = "message_dialog_action"; public static final String MESSAGE_EDIT_POSITION = "keyserver_edited_position"; @@ -82,7 +83,9 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On private int mPosition; private EditText mKeyserverEditText; + private TextInputLayout mKeyserverEditTextLayout; private CheckBox mVerifyKeyserverCheckBox; + private CheckBox mOnlyTrustedKeyserverCheckBox; public enum DialogAction { ADD, @@ -91,7 +94,8 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On public enum FailureReason { INVALID_URL, - CONNECTION_FAILED + CONNECTION_FAILED, + NO_PINNED_CERTIFICATE } public static AddEditKeyserverDialogFragment newInstance(Messenger messenger, @@ -126,7 +130,15 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On alert.setView(view); mKeyserverEditText = (EditText) view.findViewById(R.id.keyserver_url_edit_text); - mVerifyKeyserverCheckBox = (CheckBox) view.findViewById(R.id.verify_keyserver_checkbox); + mKeyserverEditTextLayout = (TextInputLayout) view.findViewById(R.id.keyserver_url_edit_text_layout); + mVerifyKeyserverCheckBox = (CheckBox) view.findViewById(R.id.verify_connection_checkbox); + mOnlyTrustedKeyserverCheckBox = (CheckBox) view.findViewById(R.id.only_trusted_keyserver_checkbox); + mVerifyKeyserverCheckBox.setOnCheckedChangeListener(new CompoundButton.OnCheckedChangeListener() { + @Override + public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) { + mOnlyTrustedKeyserverCheckBox.setEnabled(isChecked); + } + }); switch (mDialogAction) { case ADD: { @@ -212,6 +224,8 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On positiveButton.setOnClickListener(new View.OnClickListener() { @Override public void onClick(View v) { + mKeyserverEditTextLayout.setErrorEnabled(false); + // behaviour same for edit and add final String keyserverUrl = mKeyserverEditText.getText().toString(); if (mVerifyKeyserverCheckBox.isChecked()) { @@ -220,13 +234,20 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On OrbotHelper.DialogActions dialogActions = new OrbotHelper.DialogActions() { @Override public void onOrbotStarted() { - verifyConnection(keyserverUrl, - proxyPrefs.parcelableProxy.getProxy()); + verifyConnection( + keyserverUrl, + proxyPrefs.parcelableProxy.getProxy(), + mOnlyTrustedKeyserverCheckBox.isChecked() + ); } @Override public void onNeutralButton() { - verifyConnection(keyserverUrl, null); + verifyConnection( + keyserverUrl, + null, + mOnlyTrustedKeyserverCheckBox.isChecked() + ); } @Override @@ -236,7 +257,11 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On }; if (OrbotHelper.putOrbotInRequiredState(dialogActions, getActivity())) { - verifyConnection(keyserverUrl, proxyPrefs.parcelableProxy.getProxy()); + verifyConnection( + keyserverUrl, + proxyPrefs.parcelableProxy.getProxy(), + mOnlyTrustedKeyserverCheckBox.isChecked() + ); } } else { dismiss(); @@ -272,14 +297,28 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On sendMessageToHandler(MESSAGE_OKAY, data); } - public void verificationFailed(FailureReason reason) { - Bundle data = new Bundle(); - data.putSerializable(MESSAGE_FAILURE_REASON, reason); + public void verificationFailed(FailureReason failureReason) { + switch (failureReason) { + case CONNECTION_FAILED: { + mKeyserverEditTextLayout.setError( + getString(R.string.add_keyserver_connection_failed)); + break; + } + case INVALID_URL: { + mKeyserverEditTextLayout.setError( + getString(R.string.add_keyserver_invalid_url)); + break; + } + case NO_PINNED_CERTIFICATE: { + mKeyserverEditTextLayout.setError( + getString(R.string.add_keyserver_keyserver_not_trusted)); + break; + } + } - sendMessageToHandler(MESSAGE_VERIFICATION_FAILED, data); } - public void verifyConnection(String keyserver, final Proxy proxy) { + public void verifyConnection(String keyserver, final Proxy proxy, final boolean onlyTrustedKeyserver) { new AsyncTask() { ProgressDialog mProgressDialog; @@ -288,7 +327,7 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On @Override protected void onPreExecute() { mProgressDialog = new ProgressDialog(getActivity()); - mProgressDialog.setMessage(getString(R.string.progress_verifying_keyserver_url)); + mProgressDialog.setMessage(getString(R.string.progress_verifying_keyserver_connection)); mProgressDialog.setCancelable(false); mProgressDialog.show(); } @@ -316,7 +355,18 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On Log.d("Converted URL", newKeyserver.toString()); OkHttpClient client = HkpKeyserver.getClient(newKeyserver.toURL(), proxy); - TlsHelper.pinCertificateIfNecessary(client, newKeyserver.toURL()); + + // don't follow any redirects + client.setFollowRedirects(false); + client.setFollowSslRedirects(false); + + if (onlyTrustedKeyserver + && !TlsHelper.usePinnedCertificateIfAvailable(client, newKeyserver.toURL())) { + Log.w(Constants.TAG, "No pinned certificate for this host in OpenKeychain's assets."); + reason = FailureReason.NO_PINNED_CERTIFICATE; + return reason; + } + client.newCall(new Request.Builder().url(newKeyserver.toURL()).build()).execute(); } catch (TlsHelper.TlsHelperException e) { reason = FailureReason.CONNECTION_FAILED; diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/TlsHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/TlsHelper.java index d1d1ada2a..1492abdeb 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/TlsHelper.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/TlsHelper.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2013-2014 Dominik Schürmann + * Copyright (C) 2013-2015 Dominik Schürmann * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -20,6 +20,7 @@ package org.sufficientlysecure.keychain.util; import android.content.res.AssetManager; import com.squareup.okhttp.OkHttpClient; + import org.sufficientlysecure.keychain.Constants; import java.io.ByteArrayInputStream; @@ -37,7 +38,6 @@ import java.security.cert.CertificateFactory; import java.util.HashMap; import java.util.Map; -import javax.net.ssl.HttpsURLConnection; import javax.net.ssl.SSLContext; import javax.net.ssl.TrustManagerFactory; @@ -49,15 +49,14 @@ public class TlsHelper { } } - private static Map sStaticCA = new HashMap<>(); - - public static void addStaticCA(String domain, byte[] certificate) { - sStaticCA.put(domain, certificate); - } + private static Map sPinnedCertificates = new HashMap<>(); - public static void addStaticCA(String domain, AssetManager assetManager, String name) { + /** + * Add certificate from assets to pinned certificate map. + */ + public static void addPinnedCertificate(String host, AssetManager assetManager, String cerFilename) { try { - InputStream is = assetManager.open(name); + InputStream is = assetManager.open(cerFilename); ByteArrayOutputStream baos = new ByteArrayOutputStream(); int reads = is.read(); @@ -68,27 +67,36 @@ public class TlsHelper { is.close(); - addStaticCA(domain, baos.toByteArray()); + sPinnedCertificates.put(host, baos.toByteArray()); } catch (IOException e) { Log.w(Constants.TAG, e); } } - public static void pinCertificateIfNecessary(OkHttpClient client, URL url) throws TlsHelperException, IOException { + /** + * Use pinned certificate for OkHttpClient if we have one. + * + * @return true, if certificate is available, false if not + * @throws TlsHelperException + * @throws IOException + */ + public static boolean usePinnedCertificateIfAvailable(OkHttpClient client, URL url) throws TlsHelperException, IOException { if (url.getProtocol().equals("https")) { - for (String domain : sStaticCA.keySet()) { - if (url.getHost().endsWith(domain)) { - pinCertificate(sStaticCA.get(domain), client); + // use certificate PIN from assets if we have one + for (String host : sPinnedCertificates.keySet()) { + if (url.getHost().endsWith(host)) { + pinCertificate(sPinnedCertificates.get(host), client); + return true; } } } + return false; } /** * Modifies the client to accept only requests with a given certificate. Applies to all URLs requested by the * client. * Therefore a client that is pinned this way should be used to only make requests to URLs with passed certificate. - * TODO: Refactor - More like SSH StrictHostKeyChecking than pinning? * * @param certificate certificate to pin * @param client OkHttpClient to enforce pinning on @@ -97,8 +105,10 @@ public class TlsHelper { */ private static void pinCertificate(byte[] certificate, OkHttpClient client) throws TlsHelperException, IOException { - // We don't use OkHttp's CertificatePinner since it depends on a TrustManager to verify it too. Refer to - // note at end of description: http://square.github.io/okhttp/javadoc/com/squareup/okhttp/CertificatePinner.html + // We don't use OkHttp's CertificatePinner since it can not be used to pin self-signed + // certificate if such certificate is not accepted by TrustManager. + // (Refer to note at end of description: + // http://square.github.io/okhttp/javadoc/com/squareup/okhttp/CertificatePinner.html ) // Creating our own TrustManager that trusts only our certificate eliminates the need for certificate pinning try { // Load CA @@ -126,42 +136,4 @@ public class TlsHelper { } } - /** - * Opens a Connection that will only accept certificates signed with a specific CA and skips common name check. - * This is required for some distributed Keyserver networks like sks-keyservers.net - * - * @param certificate The X.509 certificate used to sign the servers certificate - * @param url Connection target - */ - public static HttpsURLConnection openCAConnection(byte[] certificate, URL url) - throws TlsHelperException, IOException { - try { - // Load CA - CertificateFactory cf = CertificateFactory.getInstance("X.509"); - Certificate ca = cf.generateCertificate(new ByteArrayInputStream(certificate)); - - // Create a KeyStore containing our trusted CAs - String keyStoreType = KeyStore.getDefaultType(); - KeyStore keyStore = KeyStore.getInstance(keyStoreType); - keyStore.load(null, null); - keyStore.setCertificateEntry("ca", ca); - - // Create a TrustManager that trusts the CAs in our KeyStore - String tmfAlgorithm = TrustManagerFactory.getDefaultAlgorithm(); - TrustManagerFactory tmf = TrustManagerFactory.getInstance(tmfAlgorithm); - tmf.init(keyStore); - - // Create an SSLContext that uses our TrustManager - SSLContext context = SSLContext.getInstance("TLS"); - context.init(null, tmf.getTrustManagers(), null); - - // Tell the URLConnection to use a SocketFactory from our SSLContext - HttpsURLConnection urlConnection = (HttpsURLConnection) url.openConnection(); - urlConnection.setSSLSocketFactory(context.getSocketFactory()); - - return urlConnection; - } catch (CertificateException | KeyManagementException | KeyStoreException | NoSuchAlgorithmException e) { - throw new TlsHelperException(e); - } - } } -- cgit v1.2.3