From ef0fa75ad439242777b8992600656d66c4633206 Mon Sep 17 00:00:00 2001 From: Andrea Maria Piana Date: Mon, 18 Jun 2018 14:59:15 +0200 Subject: [PATCH] Handle empty/invalid keys Signed-off-by: Andrea Maria Piana --- src/status_im/data_store/realm/core.cljs | 44 +++++-------- src/status_im/translations/en.cljs | 6 ++ .../ui/screens/accounts/login/events.cljs | 29 ++++---- .../desktop/main/tabs/profile/views.cljs | 5 +- src/status_im/ui/screens/events.cljs | 60 ++++++++--------- .../ui/screens/profile/user/views.cljs | 5 +- src/status_im/utils/keychain.cljs | 48 -------------- src/status_im/utils/keychain/core.cljs | 66 +++++++++++++++++++ src/status_im/utils/keychain/events.cljs | 17 +++++ .../react_native/js_dependencies.cljs | 3 + .../status_im/test/data_store/realm/core.cljs | 28 +------- test/cljs/status_im/test/runner.cljs | 2 + .../status_im/test/utils/keychain/core.cljs | 52 +++++++++++++++ 13 files changed, 209 insertions(+), 156 deletions(-) delete mode 100644 src/status_im/utils/keychain.cljs create mode 100644 src/status_im/utils/keychain/core.cljs create mode 100644 src/status_im/utils/keychain/events.cljs create mode 100644 test/cljs/status_im/test/utils/keychain/core.cljs diff --git a/src/status_im/data_store/realm/core.cljs b/src/status_im/data_store/realm/core.cljs index 804c0e3071..9d8cc7754b 100644 --- a/src/status_im/data_store/realm/core.cljs +++ b/src/status_im/data_store/realm/core.cljs @@ -12,6 +12,8 @@ [status-im.utils.utils :as utils]) (:refer-clojure :exclude [exists?])) +(def new-account-filename "new-account") + (defn to-buffer [key] (when key (let [length (.-length key) @@ -20,24 +22,11 @@ (aset arr i (aget key i))) (.-buffer arr)))) -(defn unencrypted-realm? - "Detect whether there is a unencrypted version of realm by checking whether - opening realm is successful" - [file-name] - (boolean - (.schemaVersion rn-dependencies/realm file-name))) - (defn encrypted-realm-version "Returns -1 if the file does not exists, the schema version if it successfully - decrypts it, nil otherwise." - ;; We don't throw here as we want to know whether the - ;; user is upgrading from an older version of the app (<= 0.9.18), in which case - ;; we need to reset the database, as it was unencrypted / wallet compatibility." + decrypts it, error otherwise." [file-name encryption-key] - (try - (.schemaVersion rn-dependencies/realm file-name (to-buffer encryption-key)) - (catch js/Object e - nil))) + (.schemaVersion rn-dependencies/realm file-name (to-buffer encryption-key))) (defn open-realm [options file-name encryption-key] @@ -53,6 +42,15 @@ [file-name] (.deleteFile rn-dependencies/realm (clj->js {:path file-name}))) +(defn- delete-realms [] + (log/warn "realm: deleting all realms") + (try + (do + (delete-realm (.-defaultPath rn-dependencies/realm)) + (delete-realm new-account-filename)) + (catch :default ex + (log/warn "failed to delete realm" ex)))) + (defn- close [realm] (when realm (.close realm))) @@ -75,17 +73,9 @@ (defn migrate-realm "Migrate realm if is a compatible version or reset the database" [file-name schemas encryption-key] - (let [encrypted-version (encrypted-realm-version file-name encryption-key) - ;; If it's unencrypted reset schema - unencrypted? (and (not encrypted-version) - (unencrypted-realm? file-name))] - (cond - ;; -1 if it's a new installation, n if encrypted and existing - encrypted-version (migrate-schemas file-name schemas encryption-key encrypted-version) - unencrypted? (do - (utils/show-popup "Important: Wallet Upgrade" "The Status Wallet will be upgraded in this release. The 12 mnemonic words will generate different addresses and whisper identities (public key). Given that we changed the algorithm used to generate keys and addresses, it will be impossible to re-import accounts created with the old algorithm in Status. Please create a new account.") - (reset-realm file-name schemas encryption-key) - (migrate-realm file-name schemas encryption-key))))) + (migrate-schemas file-name schemas encryption-key (encrypted-realm-version + file-name + encryption-key))) (defn open-migrated-realm [file-name schemas encryption-key] @@ -94,8 +84,6 @@ (defn- index-entity-schemas [all-schemas] (into {} (map (juxt :name identity)) (-> all-schemas last :schema))) -(def new-account-filename "new-account") - (def base-realm (atom nil)) (def account-realm (atom nil)) diff --git a/src/status_im/translations/en.cljs b/src/status_im/translations/en.cljs index e1a8a0dbdc..8c20ce3663 100644 --- a/src/status_im/translations/en.cljs +++ b/src/status_im/translations/en.cljs @@ -681,6 +681,12 @@ :ropsten-network "Ropsten test network" :rinkeby-network "Rinkeby test network" + ;; invalid-key + + :invalid-key-title "Invalid key detected" + :invalid-key-content "The key used to encrypt your data is invalid. Clicking on 'Reset database' will delete any existing realm and create a new one with a stronger key. If you would like to backup you data please click on cancel and the app will quit. Ensure the data directory of the app is clean and make sure no data is backed up by your cloud provider." + :invalid-key-confirm "Reset database" + ;; browser :browser "Browser" :enter-dapp-url "Enter a ÐApp URL" diff --git a/src/status_im/ui/screens/accounts/login/events.cljs b/src/status_im/ui/screens/accounts/login/events.cljs index 6382d86226..ff4435efdb 100644 --- a/src/status_im/ui/screens/accounts/login/events.cljs +++ b/src/status_im/ui/screens/accounts/login/events.cljs @@ -7,7 +7,7 @@ [status-im.data-store.core :as data-store] [status-im.native-module.core :as status] [status-im.utils.config :as config] - [status-im.utils.keychain :as keychain] + [status-im.utils.keychain.core :as keychain] [status-im.utils.utils :as utils])) ;;;; FX @@ -24,21 +24,28 @@ (fn [] (status/clear-web-data))) +(defn change-account [address encryption-key] + (let [change-account-fn (fn [] (data-store/change-account address + false + encryption-key + #(dispatch [:change-account-handler % address])))] + (if config/stub-status-go? + (utils/set-timeout change-account-fn + 300) + (change-account-fn)))) + (reg-fx ::change-account (fn [[address]] ;; if we don't add delay when running app without status-go ;; "null is not an object (evaluating 'realm.schema')" error appears - (keychain/get-encryption-key-then - (fn [encryption-key] - (let [change-account-fn (fn [] (data-store/change-account address - false - encryption-key - #(dispatch [:change-account-handler % address])))] - (if config/stub-status-go? - (utils/set-timeout change-account-fn - 300) - (change-account-fn))))))) + (.. (keychain/get-encryption-key) + (then (partial change-account address)) + (catch (fn [{:keys [error key]}] + ;; no need of further error handling as already taken care + ;; when starting the app + (when (= error :weak-key) + (change-account address key))))))) ;;;; Handlers diff --git a/src/status_im/ui/screens/desktop/main/tabs/profile/views.cljs b/src/status_im/ui/screens/desktop/main/tabs/profile/views.cljs index 4bfb63c769..2a748f9764 100644 --- a/src/status_im/ui/screens/desktop/main/tabs/profile/views.cljs +++ b/src/status_im/ui/screens/desktop/main/tabs/profile/views.cljs @@ -1,7 +1,6 @@ (ns status-im.ui.screens.desktop.main.tabs.profile.views (:require-macros [status-im.utils.views :as views]) (:require [re-frame.core :as re-frame] - [status-im.utils.keychain :as keychain] [status-im.ui.components.react :as react] [status-im.ui.screens.profile.user.views :as profile])) @@ -36,9 +35,7 @@ [react/view [my-profile-info current-account]] [react/view {:style {:height 1 :background-color "#e8ebec" :margin-horizontal 16}}] - [react/touchable-highlight {:on-press #(keychain/get-encryption-key-then - (fn [encryption-key] - (re-frame/dispatch [:logout encryption-key]))) + [react/touchable-highlight {:on-press #(re-frame/dispatch [:logout]) :style {:margin-top 60}} [react/view [react/text {:style {:color :red}} "Log out"]]]])) diff --git a/src/status_im/ui/screens/events.cljs b/src/status_im/ui/screens/events.cljs index 5904ade26c..3a6337b23f 100644 --- a/src/status_im/ui/screens/events.cljs +++ b/src/status_im/ui/screens/events.cljs @@ -28,13 +28,17 @@ status-im.ui.screens.bootnodes-settings.events status-im.ui.screens.currency-settings.events status-im.ui.screens.usage-data.events + status-im.utils.keychain.events [re-frame.core :as re-frame] [status-im.native-module.core :as status] [status-im.ui.components.permissions :as permissions] [status-im.constants :as constants] [status-im.data-store.core :as data-store] + [status-im.data-store.realm.core :as realm] + [status-im.utils.keychain.core :as keychain] [status-im.i18n :as i18n] [status-im.js-dependencies :as dependencies] + [status-im.ui.components.react :as react] [status-im.transport.core :as transport] [status-im.transport.inbox :as inbox] [status-im.ui.screens.db :refer [app-db]] @@ -47,7 +51,6 @@ [status-im.utils.handlers-macro :as handlers-macro] [status-im.utils.http :as http] [status-im.utils.instabug :as instabug] - [status-im.utils.keychain :as keychain] [status-im.utils.mixpanel :as mixpanel] [status-im.utils.platform :as platform] [status-im.utils.types :as types] @@ -139,25 +142,6 @@ (status/move-to-internal-storage #(status/start-node config))) -(re-frame/reg-fx - ::initialize-keychain-fx - (fn [] - (keychain/get-encryption-key-then - (fn [encryption-key] - (re-frame/dispatch [:initialize-app encryption-key]))))) - -(re-frame/reg-fx - ::get-encryption-key-fx - (fn [event] - (keychain/get-encryption-key-then - (fn [encryption-key] - (re-frame/dispatch [event encryption-key]))))) - -(re-frame/reg-fx - ::got-encryption-key-fx - (fn [{:keys [encryption-key callback]}] - (callback encryption-key))) - (re-frame/reg-fx :initialize-geth-fx (fn [config] @@ -237,23 +221,31 @@ (handlers/register-handler-fx :initialize-keychain (fn [_ _] - {::initialize-keychain-fx nil})) + {:get-encryption-key [:initialize-app]})) -(handlers/register-handler-fx - :got-encryption-key - (fn [_ [_ opts]] - {::got-encryption-key-fx opts})) +(def handle-invalid-key-parameters + {:title (i18n/label :invalid-key-title) + :content (i18n/label :invalid-key-content) + :confirm-button-text (i18n/label :invalid-key-confirm) + :on-cancel #(.exitApp react/back-handler) + :on-accept (fn [] + (realm/delete-realms) + (.. (keychain/reset) + (then + #(re-frame/dispatch [:initialize-keychain]))))}) (handlers/register-handler-fx :initialize-app - (fn [_ [_ encryption-key]] - {::init-device-UUID nil - ::testfairy-alert nil - :dispatch-n [[:initialize-db encryption-key] - [:load-accounts] - [:initialize-views] - [:listen-to-network-status] - [:initialize-geth]]})) + (fn [_ [_ encryption-key error]] + (if (= error :invalid-key) + {:show-confirmation handle-invalid-key-parameters} + {::init-device-UUID nil + ::testfairy-alert nil + :dispatch-n [[:initialize-db encryption-key] + [:load-accounts] + [:initialize-views] + [:listen-to-network-status] + [:initialize-geth]]}))) (handlers/register-handler-fx :logout @@ -267,7 +259,7 @@ [:navigate-to :accounts]]} (navigation/navigate-to-clean nil) (transport/stop-whisper))) - {::get-encryption-key-fx this-event}))) + {:get-encryption-key [this-event]}))) (handlers/register-handler-fx :initialize-db diff --git a/src/status_im/ui/screens/profile/user/views.cljs b/src/status_im/ui/screens/profile/user/views.cljs index 95b09d1da0..2d65e88ac4 100644 --- a/src/status_im/ui/screens/profile/user/views.cljs +++ b/src/status_im/ui/screens/profile/user/views.cljs @@ -16,7 +16,6 @@ [status-im.ui.screens.profile.user.styles :as styles] [status-im.utils.build :as build] [status-im.utils.config :as config] - [status-im.utils.keychain :as keychain] [status-im.utils.platform :as platform] [status-im.utils.utils :as utils] [status-im.ui.components.icons.vector-icons :as icons] @@ -91,9 +90,7 @@ (defn- handle-logout [] (utils/show-confirmation (i18n/label :t/logout-title) (i18n/label :t/logout-are-you-sure) - (i18n/label :t/logout) #(keychain/get-encryption-key-then - (fn [encryption-key] - (re-frame/dispatch [:logout encryption-key]))))) + (i18n/label :t/logout) #(re-frame/dispatch [:logout]))) (defn- my-profile-settings [{:keys [seed-backed-up? mnemonic]} currency] (let [show-backup-seed? (and (not seed-backed-up?) (not (string/blank? mnemonic)))] diff --git a/src/status_im/utils/keychain.cljs b/src/status_im/utils/keychain.cljs deleted file mode 100644 index e00891dcf9..0000000000 --- a/src/status_im/utils/keychain.cljs +++ /dev/null @@ -1,48 +0,0 @@ -(ns status-im.utils.keychain - (:require [re-frame.core :as re-frame] - [taoensso.timbre :as log] - [status-im.react-native.js-dependencies :as rn])) - -(def key-bytes 64) -(def username "status-im.encryptionkey") - -(defn- encryption-key-fetch [{:keys [resolve reject]}] - (-> (.getGenericPassword rn/keychain) - (.then - (fn [res] - (if (not res) - (when reject - (reject)) - (let [encryption-key (.parse js/JSON (.-password res))] - (log/debug "Found existing encryption key!") - (re-frame/dispatch [:got-encryption-key {:encryption-key encryption-key - :callback resolve}]))))) - (.catch - (fn [err] - (log/debug err))))) - -(defn encryption-key-reset [] - (log/debug "Resetting key...") - (-> (.resetGenericPassword rn/keychain))) - -(defn get-encryption-key-then [callback] - (log/debug "Initializing realm encryption key...") - (encryption-key-fetch {:resolve callback - :reject (fn [] - (log/debug "No key exists, creating...") - (-> (rn/secure-random key-bytes) - (.then - (fn [encryption-key] - (-> (.setGenericPassword - rn/keychain - username - (.stringify js/JSON (.from js/Array encryption-key))) - (.then - (fn [res] - (encryption-key-fetch {:resolve callback}))) - (.catch - (fn [err] - (log/debug err)))))) - (.catch - (fn [err] - (log/debug err)))))})) \ No newline at end of file diff --git a/src/status_im/utils/keychain/core.cljs b/src/status_im/utils/keychain/core.cljs new file mode 100644 index 0000000000..fe6d76a050 --- /dev/null +++ b/src/status_im/utils/keychain/core.cljs @@ -0,0 +1,66 @@ +(ns status-im.utils.keychain.core + (:require [re-frame.core :as re-frame] + [taoensso.timbre :as log] + [status-im.react-native.js-dependencies :as rn])) + +(def key-bytes 64) +(def username "status-im.encryptionkey") + +(defn- bytes->js-array [b] + (.from js/Array b)) + +(defn- string->js-array [s] + (.parse js/JSON (.-password s))) + +;; Smoke test key to make sure is ok, we noticed some non-random keys on +;; some IOS devices. We check naively that there are no more than key-bytes/2 +;; identical characters. +(defn validate + [encryption-key] + (cond + (or (not encryption-key) + (not= (.-length encryption-key) key-bytes)) + (.reject js/Promise {:error :invalid-key + :key encryption-key}) + + (>= (/ key-bytes 2) + (count (keys (group-by identity encryption-key)))) + (.reject js/Promise {:error :weak-key + :key encryption-key}) + + :else encryption-key)) + +(defn store [encryption-key] + (log/debug "storing encryption key") + (-> (.setGenericPassword + rn/keychain + username + (.stringify js/JSON encryption-key)) + (.then (constantly encryption-key)))) + +(defn create [] + (log/debug "no key exists, creating...") + (.. (rn/secure-random key-bytes) + (then bytes->js-array))) + +(defn handle-not-found [] + (.. (create) + (then validate) + (then store))) + +(def handle-found + (comp validate + string->js-array)) + +(defn get-encryption-key [] + (log/debug "initializing realm encryption key...") + (.. (.getGenericPassword rn/keychain) + (then + (fn [res] + (if res + (handle-found res) + (handle-not-found)))))) + +(defn reset [] + (log/debug "resetting key...") + (.resetGenericPassword rn/keychain)) diff --git a/src/status_im/utils/keychain/events.cljs b/src/status_im/utils/keychain/events.cljs new file mode 100644 index 0000000000..e290ecff4e --- /dev/null +++ b/src/status_im/utils/keychain/events.cljs @@ -0,0 +1,17 @@ +(ns status-im.utils.keychain.events + (:require [re-frame.core :as re-frame] + [taoensso.timbre :as log] + [status-im.utils.keychain.core :as keychain])) + +(defn handle-key-error [event {:keys [error key]}] + (if (= :weak-key error) + (log/warn "weak key used, database might not be encrypted properly") + (log/error "invalid key detected")) + (re-frame/dispatch (into [] (concat event [key error])))) + +(re-frame/reg-fx + :get-encryption-key + (fn [event] + (.. (keychain/get-encryption-key) + (then #(re-frame/dispatch (conj event %))) + (catch (partial handle-key-error event))))) diff --git a/test/cljs/status_im/react_native/js_dependencies.cljs b/test/cljs/status_im/react_native/js_dependencies.cljs index 29103e72bb..51f97cba0a 100644 --- a/test/cljs/status_im/react_native/js_dependencies.cljs +++ b/test/cljs/status_im/react_native/js_dependencies.cljs @@ -42,3 +42,6 @@ :setInterval js/setInterval :clearTimeout js/clearTimeout :clearInterval js/clearInterval}) + +(def keychain #js {:setGenericPassword (constantly (.resolve js/Promise true))}) +(def secure-random #(.resolve js/Promise (clj->js (range 0 %)))) diff --git a/test/cljs/status_im/test/data_store/realm/core.cljs b/test/cljs/status_im/test/data_store/realm/core.cljs index 67d4659f31..c8a973cd77 100644 --- a/test/cljs/status_im/test/data_store/realm/core.cljs +++ b/test/cljs/status_im/test/data_store/realm/core.cljs @@ -3,50 +3,24 @@ [status-im.utils.utils :as utils] [status-im.data-store.realm.core :as core])) -(def showed-popup? (atom nil)) -(def resetted-realm? (atom nil)) (def migrated-realm? (atom nil)) (defn fixtures [f] - (reset! showed-popup? nil) - (reset! resetted-realm? nil) (reset! migrated-realm? nil) (f)) (use-fixtures :each fixtures) (deftest migrate-realm - (with-redefs [core/reset-realm #(reset! resetted-realm? true) - utils/show-popup #(reset! showed-popup? true) - core/open-realm #(reset! migrated-realm? true)] + (with-redefs [core/open-realm #(reset! migrated-realm? true)] (testing "the database does not exists" (with-redefs [core/encrypted-realm-version (constantly -1)] (core/migrate-realm "test-filename" [] "encryption-key") - (testing "it does not reset realm" - (is (not @resetted-realm?))) - (testing "it does not show a popup" - (is (not @showed-popup?))) (testing "it migrates the db" (is @migrated-realm?)))) (testing "the database exists" (with-redefs [core/encrypted-realm-version (constantly 2)] (core/migrate-realm "test-filename" [] "encryption-key") - (testing "it does not reset realm" - (is (not @resetted-realm?))) - (testing "it does not show a popup" - (is (not @showed-popup?))) - (testing "it migrates the db" - (is @migrated-realm?)))) - (testing "the database exists, but is unencrypted" - (with-redefs [core/encrypted-realm-version #(if @resetted-realm? - -1 - nil) - core/unencrypted-realm? (constantly true)] - (core/migrate-realm "test-filename" [] "encryption-key") - (testing "it resets realm" - (is @resetted-realm?)) - (testing "it shows a popup" - (is @showed-popup?)) (testing "it migrates the db" (is @migrated-realm?)))))) diff --git a/test/cljs/status_im/test/runner.cljs b/test/cljs/status_im/test/runner.cljs index 1d03aab10d..7a7edc76f2 100644 --- a/test/cljs/status_im/test/runner.cljs +++ b/test/cljs/status_im/test/runner.cljs @@ -41,6 +41,7 @@ [status-im.test.utils.datetime] [status-im.test.utils.mixpanel] [status-im.test.utils.prices] + [status-im.test.utils.keychain.core] [status-im.test.ui.screens.accounts.login.events])) (enable-console-print!) @@ -93,4 +94,5 @@ 'status-im.test.utils.datetime 'status-im.test.utils.mixpanel 'status-im.test.utils.prices + 'status-im.test.utils.keychain.core 'status-im.test.ui.screens.accounts.login.events) diff --git a/test/cljs/status_im/test/utils/keychain/core.cljs b/test/cljs/status_im/test/utils/keychain/core.cljs new file mode 100644 index 0000000000..c71c50a143 --- /dev/null +++ b/test/cljs/status_im/test/utils/keychain/core.cljs @@ -0,0 +1,52 @@ +(ns status-im.test.utils.keychain.core + (:require [cljs.test :refer-macros [deftest async is testing]] + [status-im.react-native.js-dependencies :as rn] + [status-im.utils.keychain.core :as keychain])) + +(def strong-key (range 0 64)) +(def weak-key (concat (range 0 32) (take 32 (repeat 0)))) + +(defn- key->json [k] + (->> k + (keychain/bytes->js-array) + (.stringify js/JSON))) + +(deftest key-does-not-exists + (async + done + (with-redefs [rn/keychain #js {:getGenericPassword (constantly (.resolve js/Promise nil))}] + (testing "it returns a valid key" + (.. (keychain/get-encryption-key) + (then (fn [k] + (is (= strong-key (js->clj k))) + (done))) + (catch (fn [err] + (is (not err)) + (done)))))))) + +(deftest key-does-exists + (async + done + (with-redefs [rn/keychain #js {:getGenericPassword (constantly (.resolve js/Promise #js {:password (key->json (range 64 128))}))}] + (testing "it returns a valid key" + (.. (keychain/get-encryption-key) + (then (fn [k] + (is (= (range 64 128) (js->clj k))) + (done))) + (catch (fn [err] + (is (not err)) + (done)))))))) + +(deftest key-is-weak + (async + done + (with-redefs [rn/keychain #js {:getGenericPassword (constantly (.resolve js/Promise #js {:password (key->json weak-key)}))}] + (testing "it returns a valid key" + (.. (keychain/get-encryption-key) + (then (fn [_] + (is false) + (done))) + (catch (fn [{:keys [error key]}] + (is (= :weak-key error)) + (is (= weak-key (js->clj key))) + (done))))))))