fix(security): bind signing authorities; disclose EOS permissions; fail early

Bind the authority to the key that signs:

- thorchain, mayachain: MsgDeposit's `signer` must be the account this session
  derives, not merely a well-formed address on the right network. It is
  serialized verbatim as the message authority, and the confirmation labels it
  as though it were a destination, so a valid-but-foreign signer produced a
  signed document the device cannot authorize and the screen did not give it
  away.

- binance: a transfer's input is its authority and no screen displays it, so
  network-correctness was not enough. It is now required to equal the session
  address. The output stays a plain address check -- it is the recipient, and
  it is shown.

Disclose what is signed:

- eos: show every actor@permission on an unknown action. The authorization
  list is compared across chunks and hashed into the preimage, but the
  approval named only the contract, action, byte count and data fingerprint --
  omitting the one part of an opaque action that says how much it may do.

Fail before approval:

- thortx: resolve the asset, render the amount and validate the expiry before
  the first screen. They ran after the router, vault and asset screens, and
  ethereum.c reports a late false return as ActionCancelled -- so a large but
  valid amount told the owner they had cancelled what they had just approved.

- saproxy: format both values before either screen, for the same reason.

Don't leave a cancelled screen up:

- Cancel and ClearSession now call layoutHome() after aborting. The Binance,
  Tendermint, Osmosis, THORChain, MAYAChain, EOS and Nano abort routines only
  clear state, so the cancelled transaction's approval screen stayed on the
  OLED. Placed at these two call sites rather than inside
  fsm_abort_workflows(), which toggle_screensaver() also calls immediately
  before drawing the screensaver.

Author: pastaghost

include/keepkey/firmware/binance.h

@@ -27,6 +27,10 @@ bool binance_signTxUpdateTransfer(const BinanceTransferMsg* _msg);
 bool binance_signTxUpdateMsgSend(const uint64_t amount, const char* to_address);
 bool binance_signTxFinalize(uint8_t* public_key, uint8_t* signature);
 bool binance_signingIsInited(void);
+
+/// True iff `address` is the account this session's key signs as. Use for a
+/// transfer's input, which is its authority.
+bool binance_addressIsSigner(const char* address);
 bool binance_signingIsFinished(void);
 void binance_signAbort(void);
 const BinanceSignTx* binance_getBinanceSignTx(void);

include/keepkey/firmware/mayachain.h

@@ -27,6 +27,10 @@ bool mayachain_signTxUpdateMsgSend(const uint64_t amount,
 bool mayachain_signTxUpdateMsgDeposit(const MayachainMsgDeposit* depmsg);
 bool mayachain_signTxFinalize(uint8_t* public_key, uint8_t* signature);
 bool mayachain_signingIsInited(void);
+
+/// True iff `address` is the account this session's key signs as. Use for
+/// MsgDeposit's `signer`, which is serialized verbatim as the authority.
+bool mayachain_addressIsSigner(const char* address);
 bool mayachain_signingIsFinished(void);
 void mayachain_signAbort(void);
 const MayachainSignTx* mayachain_getMayachainSignTx(void);

include/keepkey/firmware/thorchain.h

@@ -24,6 +24,10 @@ bool thorchain_signTxUpdateMsgSend(const uint64_t amount,
 bool thorchain_signTxUpdateMsgDeposit(const ThorchainMsgDeposit* depmsg);
 bool thorchain_signTxFinalize(uint8_t* public_key, uint8_t* signature);
 bool thorchain_signingIsInited(void);
+
+/// True iff `address` is the account this session's key signs as. Use for
+/// MsgDeposit's `signer`, which is serialized verbatim as the authority.
+bool thorchain_addressIsSigner(const char* address);
 bool thorchain_signingIsFinished(void);
 void thorchain_signAbort(void);
 const ThorchainSignTx* thorchain_getThorchainSignTx(void);

lib/firmware/binance.c

@@ -236,6 +236,20 @@ bool binance_signTxFinalize(uint8_t* public_key, uint8_t* signature) {
                            NULL) == 0;
 }
 
+/* The account this session's key signs as.
+ *
+ * A transfer's input is its authority. Checking only that it is a well-formed
+ * address on the session's network let a host obtain a signature over an input
+ * the device cannot represent, and no screen shows the input address, so
+ * nothing would have revealed it. */
+bool binance_addressIsSigner(const char* address) {
+  if (!initialized || !address || !address_prefix) return false;
+
+  char expected[46] = {0};
+  if (!tendermint_getAddress(&node, address_prefix, expected)) return false;
+  return strcmp(address, expected) == 0;
+}
+
 bool binance_signingIsInited(void) { return initialized; }
 
 bool binance_signingIsFinished(void) {

lib/firmware/eos.c

@@ -530,6 +530,37 @@ bool eos_compileActionUnknown(const EosActionCommon* common,
     char title[MEDIUM_STR_BUF];
     snprintf(title, sizeof(title), "%s:%s", account, name);
 
+    /* Show the AUTHORITIES this action runs under.
+     *
+     * unknown_common.authorization[] is part of the signed transaction and is
+     * compared across chunks and hashed into the preimage, but the screen
+     * below names only the contract, the action, a byte count and a data
+     * fingerprint. So an AdvancedMode owner approving an opaque action could
+     * not see which of their permissions it was being executed with -- the one
+     * part of an unknown action that says how much it is allowed to do.
+     * EosActionCommon carries at most 16 of them. */
+    for (pb_size_t i = 0; i < unknown_common.authorization_count; i++) {
+      const EosPermissionLevel* auth = &unknown_common.authorization[i];
+      char actor[EOS_NAME_STR_SIZE];
+      char permission[EOS_NAME_STR_SIZE];
+      CHECK_PARAM_RET(auth->has_actor && eos_formatName(auth->actor, actor),
+                      "Invalid authorization actor", false);
+      CHECK_PARAM_RET(
+          auth->has_permission && eos_formatName(auth->permission, permission),
+          "Invalid authorization permission", false);
+
+      if (!confirm(ButtonRequestType_ButtonRequest_ConfirmEosAction, title,
+                   "Authorized by %s@%s (%u of %u)", actor, permission,
+                   (unsigned)(i + 1),
+                   (unsigned)unknown_common.authorization_count)) {
+        fsm_sendFailure(FailureType_Failure_ActionCancelled,
+                        "Action Cancelled");
+        eos_signingAbort();
+        layoutHome();
+        return false;
+      }
+    }
+
     static uint8_t hash[32];
     hasher_Final(&hasher_unknown, hash);
 

lib/firmware/ethereum_contracts/saproxy.c

@@ -79,25 +79,31 @@ bool sa_confirmWithdrawFromSalary(uint32_t data_total,
    * be able to drift apart. See sa_withdrawFromSalaryExtentOk(). */
   if (!sa_withdrawFromSalaryExtentOk(msg)) return false;
 
-  char confStr[41];
-  // confirm raw unformatted numbers
-  /* bn_format() BLANKS its output buffer and returns 0 when the value does
-   * not fit -- ignoring the return renders an EMPTY amount on the
-   * confirmation screen, the one rendering a user cannot read as wrong. */
-  if (!sa_formatUint256(msg->data_initial_chunk.bytes + 4, "", confStr,
-                        sizeof(confStr)))
+  /* Format BOTH values before either screen.
+   *
+   * bn_format() blanks its output and returns 0 when the value does not fit,
+   * so an unrenderable amount is a refusal. Doing the second format after the
+   * first confirmation meant a large but perfectly valid uint256 amount failed
+   * only once the salary ID had been approved -- and the Ethereum dispatcher
+   * reports that late failure as ActionCancelled, so the owner is told they
+   * cancelled something they had in fact approved. Non-interactive work
+   * belongs before the first screen. */
+  char idStr[41];
+  char amountStr[41];
+  if (!sa_formatUint256(msg->data_initial_chunk.bytes + 4, "", idStr,
+                        sizeof(idStr)))
     return false;
+  if (!sa_formatUint256(msg->data_initial_chunk.bytes + 4 + 32, " Token Units",
+                        amountStr, sizeof(amountStr)))
+    return false;
+
   if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, "Sablier",
-               "Salary ID %s", confStr)) {
+               "Salary ID %s", idStr)) {
     return false;
   }
 
-  // confirm raw unformatted numbers
-  if (!sa_formatUint256(msg->data_initial_chunk.bytes + 4 + 32, " Token Units",
-                        confStr, sizeof(confStr)))
-    return false;
   if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, "Sablier",
-               "Withdraw Amount %s", confStr)) {
+               "Withdraw Amount %s", amountStr)) {
     return false;
   }
   return true;

lib/firmware/ethereum_contracts/thortx.c

@@ -142,33 +142,17 @@ bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg) {
   thorchainData =
       (uint8_t*)(msg->data_initial_chunk.bytes + 4 + (is_expiry ? 6 : 5) * 32);
 
-  // Start confirmations
-  for (ctr = 0; ctr < 20; ctr++) {
-    snprintf(&confStr[ctr * 2], 3, "%02x", msg->to.bytes[ctr]);
-  }
-  /* THOR_ROUTER is an Ethereum-mainnet identity. The same 20 bytes on another
-   * EVM chain are an unrelated contract, so the trusted label has to be bound
-   * to the chain; otherwise a host-chosen chain_id borrows it. */
-  if (msg->has_chain_id && msg->chain_id == 1 &&
-      strncmp(confStr, THOR_ROUTER, sizeof(THOR_ROUTER)) == 0) {
-    conf = "Thorchain router";
-  } else {
-    conf = confStr;
-  }
-  if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, "Thorchain data",
-               "Routing through %s", conf)) {
-    return false;
-  }
-
-  // just display token address and amount as string
-  for (ctr = 0; ctr < 20; ctr++) {
-    snprintf(&confStr[ctr * 2], 3, "%02x", vaultAddress[ctr]);
-  }
-  if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, "Thorchain data",
-               "Using Asgard vault %s", confStr)) {
-    return false;
-  }
-
+  /* Everything non-interactive FIRST, so an unrenderable call fails before any
+   * approval is taken.
+   *
+   * The amount used to be formatted after the router, vault and asset screens
+   * had been approved, and the expiry word validated after that. bn_format()
+   * refuses a value it cannot render, and ethereum.c turns a false return from
+   * this decoder into ActionCancelled -- so a large but valid amount, or a
+   * non-canonical expiry, told the owner they had cancelled a transaction they
+   * had already approved three screens of. Resolve the asset, render the
+   * amount, and check the expiry up here; the confirmations below then only
+   * display what is already known to be displayable. */
   assetAddress = contractAssetAddress;
   /* The THORChain ABI uses the zero address to mean this signing chain's
    * native asset.  Resolve that router-specific meaning directly instead of
@@ -181,40 +165,18 @@ bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg) {
     assetToken = tokenByChainAddress(msg->chain_id, assetAddress);
   }
 
+  char amountStr[41];
   if (assetToken == UnknownToken) {
-    // just display token address and amount as string
-    for (ctr = 0; ctr < 20; ctr++) {
-      snprintf(&confStr[ctr * 2], 3, "%02x", assetAddress[ctr]);
-    }
-    if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput,
-                 "Thorchain data", "from asset %s", confStr)) {
-      return false;
-    }
-    // We don't know what the exponent should be so just confirm raw unformatted
-    // number
-    /* bn_format() BLANKS its output buffer and returns 0 when the value
-     * does not fit -- ignoring the return renders an EMPTY amount on the
-     * confirmation screen, the one rendering a user cannot read as wrong.
-     * Never leave the caller a blank amount. */
+    /* We don't know what the exponent should be, so confirm the raw
+     * unformatted number. */
     if (!thor_formatUnknownAssetAmount(
-            msg->data_initial_chunk.bytes + 4 + 2 * 32, confStr,
-            sizeof(confStr)))
-      return false;
-
-    if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput,
-                 "Thorchain data", "amount %s", confStr)) {
+            msg->data_initial_chunk.bytes + 4 + 2 * 32, amountStr,
+            sizeof(amountStr)))
       return false;
-    }
-
   } else {
-    if (!ethereumFormatAmount(&Amount, assetToken, msg->chain_id, confStr,
-                              sizeof(confStr)))
+    if (!ethereumFormatAmount(&Amount, assetToken, msg->chain_id, amountStr,
+                              sizeof(amountStr)))
       return false;
-
-    if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput,
-                 "Thorchain data", "Confirm sending %s", confStr)) {
-      return false;
-    }
   }
 
   /* depositWithExpiry() carries a fifth head word the deposit() variant does
@@ -230,6 +192,7 @@ bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg) {
    * because a far-future expiry displayed as a small epoch is worse than no
    * screen at all -- it reads as "already expired" when it means the
    * opposite. */
+  char expiry_str[21] = {0};
   if (is_expiry) {
     const uint8_t* expiry_word = msg->data_initial_chunk.bytes + 4 + 4 * 32;
     for (size_t i = 0; i < 24; i++) {
@@ -240,7 +203,6 @@ bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg) {
       expiry = (expiry << 8) | expiry_word[i];
     }
 
-    char expiry_str[21] = {0};
     char tmp[21];
     int len = 0;
     if (expiry == 0) {
@@ -254,10 +216,60 @@ bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg) {
     for (int i = 0; i < len; i++) {
       expiry_str[i] = tmp[len - 1 - i];
     }
+  }
+
+  // Start confirmations
+  for (ctr = 0; ctr < 20; ctr++) {
+    snprintf(&confStr[ctr * 2], 3, "%02x", msg->to.bytes[ctr]);
+  }
+  /* THOR_ROUTER is an Ethereum-mainnet identity. The same 20 bytes on another
+   * EVM chain are an unrelated contract, so the trusted label has to be bound
+   * to the chain; otherwise a host-chosen chain_id borrows it. */
+  if (msg->has_chain_id && msg->chain_id == 1 &&
+      strncmp(confStr, THOR_ROUTER, sizeof(THOR_ROUTER)) == 0) {
+    conf = "Thorchain router";
+  } else {
+    conf = confStr;
+  }
+  if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, "Thorchain data",
+               "Routing through %s", conf)) {
+    return false;
+  }
+
+  // just display token address and amount as string
+  for (ctr = 0; ctr < 20; ctr++) {
+    snprintf(&confStr[ctr * 2], 3, "%02x", vaultAddress[ctr]);
+  }
+  if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, "Thorchain data",
+               "Using Asgard vault %s", confStr)) {
+    return false;
+  }
+
+  if (assetToken == UnknownToken) {
+    // just display token address and amount as string
+    for (ctr = 0; ctr < 20; ctr++) {
+      snprintf(&confStr[ctr * 2], 3, "%02x", assetAddress[ctr]);
+    }
     if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput,
-                 "Thorchain data", "Expiry epoch %s", expiry_str)) {
+                 "Thorchain data", "from asset %s", confStr)) {
       return false;
     }
+
+    if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput,
+                 "Thorchain data", "amount %s", amountStr)) {
+      return false;
+    }
+
+  } else {
+    if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput,
+                 "Thorchain data", "Confirm sending %s", amountStr)) {
+      return false;
+    }
+  }
+
+  if (is_expiry && !confirm(ButtonRequestType_ButtonRequest_ConfirmOutput,
+                            "Thorchain data", "Expiry epoch %s", expiry_str)) {
+    return false;
   }
 
   /* Pass the memo's true ABI length, not a fixed 64. There is no raw-memo

lib/firmware/fsm.c

@@ -321,6 +321,15 @@ void fsm_msgClearSession(ClearSession* msg) {
   (void)msg;
   fsm_abort_workflows();
   session_clear(/*clear_pin=*/true);
+  /* Several abort routines -- Binance, Tendermint, Osmosis, THORChain,
+     MAYAChain, EOS, Nano -- only clear state and touch no layout, so without
+     this the approval screen of the transaction just cancelled stays on the
+     OLED, describing an operation that no longer exists.
+
+     Done here and in fsm_msgCancel() rather than inside fsm_abort_workflows(),
+     because that is also called from toggle_screensaver(), which draws the
+     screensaver immediately afterwards. */
+  layoutHome();
   fsm_sendSuccess("Session cleared");
 }
 

lib/firmware/fsm_msg_binance.h

@@ -141,8 +141,14 @@ void fsm_msgBinanceTransferMsg(const BinanceTransferMsg* msg) {
          rejected. */
       {
         const char* const pfix = binance_sessionAddressPrefix();
+        /* The input is the transfer's AUTHORITY, and no screen displays it,
+           so being well formed on the right network is not enough -- it has to
+           be the account this session's key signs as. The output is the
+           recipient and is shown, so it only needs to be a valid address on
+           the same network. */
         if (!pfix ||
             !tendermint_validateBech32Address(msg->inputs[0].address, pfix) ||
+            !binance_addressIsSigner(msg->inputs[0].address) ||
             !tendermint_validateBech32Address(msg->outputs[0].address, pfix)) {
           binance_signAbort();
           fsm_sendFailure(FailureType_Failure_SyntaxError,

lib/firmware/fsm_msg_common.h

@@ -563,6 +563,10 @@ void fsm_msgEntropyAck(EntropyAck* msg) {
 void fsm_msgCancel(Cancel* msg) {
   (void)msg;
   fsm_abort_workflows();
+  /* See fsm_msgClearSession(): the abort routines for Binance, Tendermint,
+     Osmosis, THORChain, MAYAChain, EOS and Nano have no layout side effect, so
+     the cancelled transaction's approval screen would otherwise stay up. */
+  layoutHome();
   fsm_sendFailure(FailureType_Failure_ActionCancelled, "Aborted");
 }