From 789ee6121224abd5877709d7aa9e872136cd75be Mon Sep 17 00:00:00 2001 From: Igor Mandrigin Date: Tue, 22 May 2018 10:07:27 +0200 Subject: [PATCH] Use a custom type to avoid accidentially logging passwords. Signed-off-by: Igor Mandrigin --- doc/decisions/0007-masking-sensitive-data.md | 39 +++++++++++++++++++ .../ui/screens/wallet/send/events.cljs | 28 ++++++------- .../ui/screens/wallet/send/views.cljs | 3 +- src/status_im/utils/handlers.cljs | 9 ++--- src/status_im/utils/security.cljs | 19 +++++++++ 5 files changed, 78 insertions(+), 20 deletions(-) create mode 100644 doc/decisions/0007-masking-sensitive-data.md create mode 100644 src/status_im/utils/security.cljs diff --git a/doc/decisions/0007-masking-sensitive-data.md b/doc/decisions/0007-masking-sensitive-data.md new file mode 100644 index 0000000000..f7e55495bf --- /dev/null +++ b/doc/decisions/0007-masking-sensitive-data.md @@ -0,0 +1,39 @@ +# 0007. Masking Sensitive Data + +| Date | Tags | +|---|---| +| 2018-05-22 | e.g: architecture, security | + + +## Status + +Proposed + +## Context + +We have some data that we don't want to appear in the logs (user passwords are +a good example). Currently, they are passed around as strings, that could be +printed out by mistake in a log entry (see https://github.com/status-im/status-react/issues/4053) + +## Decision + +To minimize the risk of leaking passwords through logs, we should not pass +passwords as strings in our codebase. We introduced a new type `MaskedData` in +`status-im.utils.security`. +We use `(security/mask-data ` to wrap sensitive data into this +type and then use `(security/unmask )` to get the plaintext back. + +It is important to keep that sensitive data masked as much as possible, until +you need the plaintext to pass to the extenral APIs. + +Example: +```clojure +(println (security/mask-data "my-plaintext-password")) ;; Outputs "******" +(println (security/unmask (security/mask-data "my-plaintext-password"))) ;; Outputs "my-plaintext-password" +``` + +## Consequences + +Tradeoffs: +- developers need to be aware of this type and have a clear separation where do +we use plaintext and where do we use masked datak diff --git a/src/status_im/ui/screens/wallet/send/events.cljs b/src/status_im/ui/screens/wallet/send/events.cljs index 493a7d83c4..bed05fd449 100644 --- a/src/status_im/ui/screens/wallet/send/events.cljs +++ b/src/status_im/ui/screens/wallet/send/events.cljs @@ -11,6 +11,7 @@ [status-im.utils.handlers :as handlers] [status-im.utils.hex :as utils.hex] [status-im.utils.money :as money] + [status-im.utils.security :as security] [status-im.utils.types :as types] [status-im.utils.utils :as utils] [status-im.constants :as constants] @@ -20,8 +21,9 @@ (re-frame/reg-fx ::accept-transaction - (fn [{:keys [password id on-completed]}] - (status/approve-sign-requests (list id) password on-completed))) + (fn [{:keys [masked-password id on-completed]}] + ;; unmasking the password as late as possible to avoid being exposed from app-db + (status/approve-sign-requests (list id) (security/unmask masked-password) on-completed))) (defn- send-ethers [{:keys [web3 from to value gas gas-price]}] (.sendTransaction (.-eth web3) @@ -170,9 +172,9 @@ (let [{:keys [password]} (get-in db [:wallet :send-transaction]) new-db' (update-in new-db [:wallet :send-transaction] merge sending-db)] ; just update sending state as we are in wallet flow {:db new-db' - ::accept-transaction {:id id - :password password - :on-completed on-transactions-completed}}))) + ::accept-transaction {:id id + :masked-password password + :on-completed on-transactions-completed}}))) ;;SIGN MESSAGE (= method constants/web3-personal-sign) @@ -259,9 +261,9 @@ network (:network db) {:keys [amount id password to symbol method gas gas-price]} (get-in db [:wallet :send-transaction])] (if id - {::accept-transaction {:id id - :password password - :on-completed on-transactions-completed} + {::accept-transaction {:id id + :masked-password password + :on-completed on-transactions-completed} :db (assoc-in db' [:wallet :send-transaction :in-progress?] true)} {:db (update-in db' [:wallet :send-transaction] assoc :waiting-signal? true @@ -282,9 +284,9 @@ (fn [{db :db} _] (let [{:keys [id password]} (get-in db [:wallet :send-transaction])] {:db (assoc-in db [:wallet :send-transaction :in-progress?] true) - ::accept-transaction {:id id - :password password - :on-completed on-transactions-modal-completed}}))) + ::accept-transaction {:id id + :masked-password password + :on-completed on-transactions-modal-completed}}))) (defn discard-transaction [{:keys [db]}] @@ -315,8 +317,8 @@ (handlers/register-handler-fx :wallet.send/set-password - (fn [{:keys [db]} [_ password]] - {:db (assoc-in db [:wallet :send-transaction :password] password)})) + (fn [{:keys [db]} [_ masked-password]] + {:db (assoc-in db [:wallet :send-transaction :password] masked-password)})) (handlers/register-handler-fx :wallet.send/set-signing? diff --git a/src/status_im/ui/screens/wallet/send/views.cljs b/src/status_im/ui/screens/wallet/send/views.cljs index edb3daa5b6..cedad36f00 100644 --- a/src/status_im/ui/screens/wallet/send/views.cljs +++ b/src/status_im/ui/screens/wallet/send/views.cljs @@ -20,6 +20,7 @@ [status-im.ui.screens.wallet.send.styles :as styles] [status-im.ui.screens.wallet.styles :as wallet.styles] [status-im.utils.money :as money] + [status-im.utils.security :as security] [status-im.utils.utils :as utils] [status-im.transport.utils :as transport.utils] [status-im.utils.ethereum.tokens :as tokens] @@ -56,7 +57,7 @@ :secure-text-entry true :placeholder (i18n/label :t/enter-password) :placeholder-text-color components.styles/color-gray4 - :on-change-text #(re-frame/dispatch [:wallet.send/set-password %]) + :on-change-text #(re-frame/dispatch [:wallet.send/set-password (security/mask-data %)]) :style styles/password :accessibility-label :enter-password-input}] (when wrong-password? diff --git a/src/status_im/utils/handlers.cljs b/src/status_im/utils/handlers.cljs index cdc508093b..b653fd3158 100644 --- a/src/status_im/utils/handlers.cljs +++ b/src/status_im/utils/handlers.cljs @@ -22,12 +22,9 @@ (defn- pretty-print-event [ctx] (let [[first second] (get-coeffect ctx :event)] - ;; TODO wrap passwords in a custom type so it won't be possible to print them occasionally - (if (= first :wallet.send/set-password) - (str first " " "******") ;; special case not to expose password to the logs - (if (or (string? second) (keyword? second) (boolean? second)) - (str first " " second) - first)))) + (if (or (string? second) (keyword? second) (boolean? second)) + (str first " " second) + first))) (def debug-handlers-names "Interceptor which logs debug information to js/console for each event." diff --git a/src/status_im/utils/security.cljs b/src/status_im/utils/security.cljs new file mode 100644 index 0000000000..25c08019ad --- /dev/null +++ b/src/status_im/utils/security.cljs @@ -0,0 +1,19 @@ +(ns status-im.utils.security) + +(defprotocol Unmaskable + ;; Retrieve the stored value. + (unmask [this])) + +;; MaskedData ensures that the object passed to it won't be occasionally printed +;; via println or log functions. Useful for keeping sensitive data, such as passwords +;; to avoid accidentally exposing them. +(deftype MaskedData [data] + Object + (toString [_] "******") + Unmaskable + (unmask [this] + (.-data this))) + +;; Returns a MaskedData instance that stores the piece of data. +(defn mask-data [data] + (MaskedData. data))