Merge pull request #473 from keepkey/fix/7142-consolidated-audit-remediation

fix(security): consolidate 7.14.2 audit remediation

Author: BitHighlander

deps/python-keepkey

@@ -28,6 +28,7 @@
 
 #define CONFIRM_SIGN_IDENTITY_TITLE 32
 #define CONFIRM_SIGN_IDENTITY_BODY 416
+#define CONFIRM_SIGN_IDENTITY_KEY 96
 
 bool confirm_cipher(bool encrypt, const char* key);
 bool confirm_encrypt_msg(const char* msg, bool signing);
@@ -46,7 +47,11 @@ bool confirm_transaction(const char* total_amount, const char* fee);
 bool confirm_load_device(bool is_node);
 bool confirm_address(const char* desc, const char* address);
 bool confirm_xpub(const char* node_str, const char* xpub);
-bool confirm_sign_identity(const IdentityType* identity, const char* challenge);
+bool format_sign_identity_key_selection(const IdentityType* identity,
+                                        const char* curve, char* out,
+                                        size_t out_len);
+bool confirm_sign_identity(const IdentityType* identity, const char* challenge,
+                           const char* curve);
 
 /**
  * Render the largest screen-sized prefix of a byte string.

include/keepkey/firmware/app_confirm.h

@@ -28,6 +28,7 @@
 void signing_init(const SignTx* msg, const CoinType* _coin,
                   const HDNode* _root);
 void signing_abort(void);
+bool signing_is_active(void);
 void signing_txack(TransactionType* tx);
 void send_fsm_co_error_message(int co_error);
 

include/keepkey/firmware/signing.h

@@ -52,19 +52,18 @@ extern bool reset_msg_stack;
 
 static CONFIDENTIAL char strbuf[BODY_CHAR_MAX];
 
-/* Set by format_body() when the formatted body did not fit strbuf, i.e. when
- * characters were lost before any screen existed to show them. Read and
- * cleared by confirm_helper(). Truncation here is invisible to every later
- * check: what reaches the renderer is a complete, well-formed, shorter string,
- * so the screen looks correct and is not. */
-static bool body_truncated = false;
-
-/* The single place a host-supplied body is formatted. vsnprintf() returns the
- * length it WOULD have written, which is the only chance to notice that
- * strbuf was too small -- after this, the evidence is gone. */
-static void format_body(const char* request_body, va_list vl) {
-  const int needed = vsnprintf(strbuf, sizeof(strbuf), request_body, vl);
-  body_truncated = (needed < 0) || ((size_t)needed >= sizeof(strbuf));
+/* vsnprintf() returns the length it WOULD have written. Treat anything that
+ * did not fit as a refusal: once characters are lost, no renderer or pager can
+ * recover them and there is no complete body the user can approve. */
+static bool format_body_into(char* out, size_t out_len,
+                             const char* request_body, va_list vl) {
+  if (!out || out_len == 0 || !request_body) return false;
+  const int needed = vsnprintf(out, out_len, request_body, vl);
+  return needed >= 0 && (size_t)needed < out_len;
+}
+
+static bool format_body(const char* request_body, va_list vl) {
+  return format_body_into(strbuf, sizeof(strbuf), request_body, vl);
 }
 
 /// Handler for push button being pressed.
@@ -511,19 +510,14 @@ static bool page_body_confirm(const char* request_title, const char* body,
   return ok;
 }
 
-/// Show a confirmation, warning first when its body will not fit the screen.
+/// Show a confirmation, paging when its complete body will not fit the screen.
 ///
 /// draw_string() draws until a glyph no longer fits the canvas and then simply
 /// stops: a body taller than BODY_ROWS is drawn in part, with no ellipsis and
 /// nothing to tell the user that the tail of an address, an amount or a
-/// warning was dropped. The vsnprintf() into strbuf[BODY_CHAR_MAX] below cuts
-/// long host strings a second time, just as quietly.
-///
-/// So when the body will not fit, put an explicit screen in front of it. That
-/// screen costs its own hold, and the hold is a real consent signal: a host
-/// Cancel breaks it and the caller reports ActionCancelled, exactly as it
-/// would for the body screen. A body that is only partly shown is now never
-/// shown without saying so.
+/// warning was dropped. Complete formatted bodies are therefore paged here.
+/// Source formatting overflow is refused by every public entry point before a
+/// ButtonRequest is emitted, because lost source cannot be paged.
 ///
 /// Bodies that fit take exactly the path they took before: one screen, one
 /// ButtonRequest, one hold.
@@ -534,70 +528,15 @@ static bool confirm_helper(const char* request_title, const char* request_body,
   const uint16_t body_width =
       (uint16_t)((iconNum == NO_ICON) ? BODY_WIDTH : BODY_WIDTH_WITH_ICON);
 
-  /* Consume the source-completeness latch exactly once, whatever happens
-   * below: leaving it set would make the NEXT confirmation warn for this
-   * one's reason. */
-  const bool truncated = body_truncated;
-  body_truncated = false;
-
-  /* Two independent ways the user can be shown less than what is being
-   * approved, and they need separate measurements because they happen at
-   * different times:
-   *
-   *   SOURCE       the formatted body did not fit strbuf. Characters were lost
-   *                before the renderer ever saw them, so no amount of looking
-   *                at the screen can detect it -- only vsnprintf()'s return
-   *                value could, and format_body() kept it.
-   *   RENDER       the body reached the renderer intact but did not fit the
-   *                canvas. draw_string_fits() replays the real placement and
-   *                reports whether the last character landed.
-   *
-   * Only layout_standard_notification is known to wrap the body at BODY_WIDTH
+  /* Only layout_standard_notification is known to wrap the body at BODY_WIDTH
    * over BODY_ROWS rows. Custom layouts place and size their own body, and
    * layout_constant_power_notification draws from x = 128 + LEFT_MARGIN where
    * the canvas edge, not BODY_WIDTH, is the limit. Measuring either of those
-   * against BODY_WIDTH would be wrong, so leave them exactly as they were --
-   * but a SOURCE truncation is layout-independent and must warn regardless.  */
+   * against BODY_WIDTH would be wrong, so leave them exactly as they were. */
   const bool render_incomplete =
       (layout_notification_func == &layout_standard_notification) &&
       !confirm_body_fits(request_body, body_width);
 
-  if (truncated) {
-    /* SOURCE truncation: characters were lost in vsnprintf() before the
-     * renderer ever saw them. They cannot be paged, because they do not
-     * exist any more. Say exactly that -- the old copy promised to show the
-     * rest on the next hold and then redrew the same clipped body, which is
-     * worse than not warning at all: a user who read it carefully was
-     * misled about what they had seen. */
-    if (!confirm_screen("Cut Off",
-                        "This text is too long to show in full. The rest "
-                        "cannot be displayed. Hold to continue anyway.",
-                        &layout_standard_notification, constant_power, NO_ICON,
-                        immediate)) {
-      return false;
-    }
-    /* The warning CONSUMED the caller's ButtonRequest, so the body screen that
-     * follows needs one of its own -- otherwise it is a required press the
-     * host was never told about, and the invariant above is broken on exactly
-     * the path that added a screen. An interactive user would get through it
-     * (button_request_acked is still set from the warning's ack), but a
-     * ButtonRequest-driven client waits forever for a message that never
-     * comes. Reachable today: confirm_sign_identity() formats into a 416-byte
-     * body and strbuf is BODY_CHAR_MAX == 352, so a long identity truncates
-     * and lands here. See #482. */
-    if (notify_host) {
-      ButtonRequest body_req;
-      memset(&body_req, 0, sizeof(body_req));
-      body_req.has_code = true;
-      body_req.code = ButtonRequestType_ButtonRequest_Other;
-      button_request_acked = false;
-      msg_write(MessageType_MessageType_ButtonRequest, &body_req);
-    }
-    return page_body_confirm(request_title, request_body,
-                             layout_notification_func, constant_power, iconNum,
-                             immediate, body_width, notify_host);
-  }
-
   if (render_incomplete) {
     /* RENDER overflow: the body reached the renderer intact, so every
      * character is still in hand and can be shown -- on more than one screen.
@@ -617,8 +556,12 @@ bool confirm(ButtonRequestType type, const char* request_title,
 
   va_list vl;
   va_start(vl, request_body);
-  format_body(request_body, vl);
+  const bool formatted = format_body(request_body, vl);
   va_end(vl);
+  if (!formatted) {
+    memzero(strbuf, sizeof(strbuf));
+    return false;
+  }
 
   /* Send button request */
   ButtonRequest resp;
@@ -716,8 +659,12 @@ bool confirm_constant_power(ButtonRequestType type, const char* request_title,
 
   va_list vl;
   va_start(vl, request_body);
-  format_body(request_body, vl);
+  const bool formatted = format_body(request_body, vl);
   va_end(vl);
+  if (!formatted) {
+    memzero(strbuf, sizeof(strbuf));
+    return false;
+  }
 
   /* Send button request */
   ButtonRequest resp;
@@ -740,8 +687,12 @@ bool confirm_with_custom_button_request(const ButtonRequest* button_request,
 
   va_list vl;
   va_start(vl, request_body);
-  format_body(request_body, vl);
+  const bool formatted = format_body(request_body, vl);
   va_end(vl);
+  if (!formatted) {
+    memzero(strbuf, sizeof(strbuf));
+    return false;
+  }
 
   /* Send button request */
   msg_write(MessageType_MessageType_ButtonRequest, button_request);
@@ -766,8 +717,12 @@ bool confirm_with_custom_layout(layout_notification_t layout_notification_func,
 
   va_list vl;
   va_start(vl, request_body);
-  format_body(request_body, vl);
+  const bool formatted = format_body(request_body, vl);
   va_end(vl);
+  if (!formatted) {
+    memzero(strbuf, sizeof(strbuf));
+    return false;
+  }
 
   /* Send button request */
   ButtonRequest resp;
@@ -789,8 +744,12 @@ bool confirm_without_button_request(const char* request_title,
 
   va_list vl;
   va_start(vl, request_body);
-  format_body(request_body, vl);
+  const bool formatted = format_body(request_body, vl);
   va_end(vl);
+  if (!formatted) {
+    memzero(strbuf, sizeof(strbuf));
+    return false;
+  }
 
   bool ret =
       confirm_helper(request_title, strbuf, &layout_standard_notification,
@@ -806,8 +765,12 @@ bool confirm_with_icon(ButtonRequestType type, IconType iconNum,
 
   va_list vl;
   va_start(vl, request_body);
-  format_body(request_body, vl);
+  const bool formatted = format_body(request_body, vl);
   va_end(vl);
+  if (!formatted) {
+    memzero(strbuf, sizeof(strbuf));
+    return false;
+  }
 
   /* Send button request */
   ButtonRequest resp;
@@ -829,8 +792,12 @@ bool review(ButtonRequestType type, const char* request_title,
 
   va_list vl;
   va_start(vl, request_body);
-  format_body(request_body, vl);
+  const bool formatted = format_body(request_body, vl);
   va_end(vl);
+  if (!formatted) {
+    memzero(strbuf, sizeof(strbuf));
+    return false;
+  }
 
   /* Send button request */
   ButtonRequest resp;
@@ -852,8 +819,12 @@ bool review_without_button_request(const char* request_title,
 
   va_list vl;
   va_start(vl, request_body);
-  format_body(request_body, vl);
+  const bool formatted = format_body(request_body, vl);
   va_end(vl);
+  if (!formatted) {
+    memzero(strbuf, sizeof(strbuf));
+    return false;
+  }
 
   const bool shown =
       confirm_helper(request_title, strbuf, &layout_standard_notification,
@@ -869,8 +840,12 @@ bool review_with_icon(ButtonRequestType type, IconType iconNum,
 
   va_list vl;
   va_start(vl, request_body);
-  format_body(request_body, vl);
+  const bool formatted = format_body(request_body, vl);
   va_end(vl);
+  if (!formatted) {
+    memzero(strbuf, sizeof(strbuf));
+    return false;
+  }
 
   /* Send button request */
   ButtonRequest resp;
@@ -892,8 +867,12 @@ bool review_immediate(ButtonRequestType type, const char* request_title,
 
   va_list vl;
   va_start(vl, request_body);
-  format_body(request_body, vl);
+  const bool formatted = format_body(request_body, vl);
   va_end(vl);
+  if (!formatted) {
+    memzero(strbuf, sizeof(strbuf));
+    return false;
+  }
 
   /* Send button request */
   ButtonRequest resp;

lib/board/confirm_sm.c

@@ -349,10 +349,42 @@ bool confirm_address(const char* desc, const char* address) {
  *     true/false of confirmation
  *
  */
-bool confirm_sign_identity(const IdentityType* identity,
-                           const char* challenge) {
+bool format_sign_identity_key_selection(const IdentityType* identity,
+                                        const char* curve, char* out,
+                                        size_t out_len) {
+  if (!identity || !curve || !out || out_len == 0) return false;
+  const int needed = snprintf(
+      out, out_len, "Index: %" PRIu32 "\nCurve: %s\nPath: %s", identity->index,
+      curve, (identity->has_path && identity->path[0]) ? "shown next" : "none");
+  return needed >= 0 && (size_t)needed < out_len;
+}
+
+bool confirm_sign_identity(const IdentityType* identity, const char* challenge,
+                           const char* curve) {
   char title[CONFIRM_SIGN_IDENTITY_TITLE], body[CONFIRM_SIGN_IDENTITY_BODY];
 
+  if (!identity || !curve) return false;
+
+  /* These values select the key and signing algorithm. Keep them out of the
+   * free-form identity body so the maximum-size path can be reviewed by the
+   * exact-byte pager instead of being shortened by a printf buffer. */
+  char key_selection[CONFIRM_SIGN_IDENTITY_KEY];
+  if (!format_sign_identity_key_selection(identity, curve, key_selection,
+                                          sizeof(key_selection)) ||
+      !confirm(ButtonRequestType_ButtonRequest_SignIdentity, "Identity Key",
+               "%s", key_selection)) {
+    memzero(key_selection, sizeof(key_selection));
+    return false;
+  }
+  memzero(key_selection, sizeof(key_selection));
+
+  if (identity->has_path && identity->path[0] &&
+      !confirm_bytes(ButtonRequestType_ButtonRequest_SignIdentity,
+                     "Identity Path", (const uint8_t*)identity->path,
+                     strlen(identity->path))) {
+    return false;
+  }
+
   /* Format protocol */
   if (identity->has_proto && identity->proto[0]) {
     strlcpy(title, identity->proto, sizeof(title));
@@ -384,9 +416,26 @@ bool confirm_sign_identity(const IdentityType* identity,
     strlcat(body, "\n", sizeof(body));
   }
 
-  /* Format challenge */
-  if (challenge && strlen(challenge) != 0) {
-    strlcat(body, challenge, sizeof(body));
+  /* Preserve the established single identity/challenge screen when it can be
+   * formatted without loss. A maximum-size challenge does not fit the shared
+   * confirmation buffer after host and user metadata; in that case confirm the
+   * metadata first and page every challenge byte separately. */
+  if (challenge && challenge[0]) {
+    const size_t body_len = strlen(body);
+    const size_t challenge_len = strlen(challenge);
+    if (body_len + challenge_len < BODY_CHAR_MAX) {
+      strlcat(body, challenge, sizeof(body));
+      return confirm(ButtonRequestType_ButtonRequest_SignIdentity, title, "%s",
+                     body);
+    }
+
+    if (body_len != 0 && !confirm(ButtonRequestType_ButtonRequest_SignIdentity,
+                                  title, "%s", body)) {
+      return false;
+    }
+    return confirm_bytes(ButtonRequestType_ButtonRequest_SignIdentity,
+                         "Visual Challenge", (const uint8_t*)challenge,
+                         challenge_len);
   }
 
   return confirm(ButtonRequestType_ButtonRequest_SignIdentity, title, "%s",