release: fold the Maya memo fix into develop

lib/firmware/fsm_msg_mayachain.h

@@ -194,11 +194,19 @@ void fsm_msgMayachainMsgAck(const MayachainMsgAck* msg) {
 
     if (msg->deposit.has_memo) {
       // See if we can parse the memo
-      if (!mayachain_parseConfirmMemo(msg->deposit.memo,
-                                      sizeof(msg->deposit.memo))) {
+      /* strnlen, not sizeof: the capacity of a fixed array is not the length
+         of the memo in it. Mirrors the THORChain path. */
+      if (!mayachain_parseConfirmMemo(
+              msg->deposit.memo,
+              strnlen(msg->deposit.memo, sizeof(msg->deposit.memo)))) {
         // Memo not recognizable, ask to confirm it
-        if (!confirm(ButtonRequestType_ButtonRequest_ConfirmMemo, _("Memo"),
-                     "%s", msg->deposit.memo)) {
+        /* confirm_bytes, not confirm("%s"): "%s" stops at the first NUL, so
+           an unparsed memo with an embedded zero would be signed with its tail
+           hidden. Takes an explicit length and escapes non-printables. */
+        if (!confirm_bytes(
+                ButtonRequestType_ButtonRequest_ConfirmMemo, _("Memo"),
+                (const uint8_t*)msg->deposit.memo,
+                strnlen(msg->deposit.memo, sizeof(msg->deposit.memo)))) {
           mayachain_signAbort();
           fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL);
           layoutHome();
@@ -225,10 +233,12 @@ void fsm_msgMayachainMsgAck(const MayachainMsgAck* msg) {
   if (sign_tx->has_memo && !msg->deposit.has_memo) {
     // See if we can parse the tx memo. This memo ignored if deposit msg has
     // memo
-    if (!mayachain_parseConfirmMemo(sign_tx->memo, sizeof(sign_tx->memo))) {
+    if (!mayachain_parseConfirmMemo(
+            sign_tx->memo, strnlen(sign_tx->memo, sizeof(sign_tx->memo)))) {
       // Memo not recognizable, ask to confirm it
-      if (!confirm(ButtonRequestType_ButtonRequest_ConfirmMemo, _("Memo"), "%s",
-                   sign_tx->memo)) {
+      if (!confirm_bytes(ButtonRequestType_ButtonRequest_ConfirmMemo, _("Memo"),
+                         (const uint8_t*)sign_tx->memo,
+                         strnlen(sign_tx->memo, sizeof(sign_tx->memo)))) {
         mayachain_signAbort();
         fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL);
         layoutHome();

lib/firmware/mayachain.c

@@ -222,10 +222,30 @@ bool mayachain_parseConfirmMemo(const char* swapStr, size_t size) {
 
   // check if memo data is recognized
 
-  if (size > sizeof(memoBuf)) return false;
+  /* One byte short of the buffer, so a full-length memo is still terminated by
+     the memzero below. */
+  if (size >= sizeof(memoBuf)) return false;
   memzero(memoBuf, sizeof(memoBuf));
-  strlcpy(memoBuf, swapStr, size);
-  memoBuf[255] = '\0';  // ensure null termination
+
+  /* `size` is a byte count and swapStr is NOT guaranteed to be NUL terminated.
+     strlcpy copied only size-1 of them, silently dropping the memo's last
+     character -- an affiliate fee of "75" bps rendered as "7" -- and then
+     walked past the end of the source looking for a terminator. Copy exactly
+     `size` bytes; the memzero'd tail terminates them.
+     Same defect as the THORChain path; Maya is a fork of it and kept the
+     original code. */
+  memcpy(memoBuf, swapStr, size);
+
+  /* strtok treats memoBuf as a C string and stops at the first NUL, but all
+     `size` bytes are covered by the signature. A memo carrying an embedded
+     zero would parse and confirm as if it ended there while the suffix stayed
+     signed. A length word that does not describe its own content is a
+     non-canonical encoding, so refuse it and let the caller disclose the raw
+     bytes. Mirrors thorchain.c. */
+  for (uint16_t i = 0; i < size; i++) {
+    if (memoBuf[i] == '\0') return false;
+  }
+
   tok = strtok(memoBuf, ":");
 
   // get transaction and asset

lib/firmware/thorchain.c

@@ -247,25 +247,23 @@ ThorchainMemoResult thorchain_parseConfirmMemo(const char* swapStr,
      as if it ended at the zero byte while the suffix stayed in the signed
      calldata. The EVM caller passes the true ABI length, so those bytes are
      real.
-     The distinction that matters is whether anything FOLLOWS the NUL:
-       - content after it  -> the memo lies about where it ends and there is no
-                              honest way to parse it. Fail closed; the caller's
-                              UNPARSED path discloses the raw bytes with a
-                              length-aware writer that shows the NUL too.
-       - only NULs after it -> the length merely overstates by a byte or two,
-                              which is what ABI padding looks like when the
-                              length word is generous. Nothing is hidden, so
-                              treat the memo as ending there and parse it.
-     Rejecting both would break legitimate ADD/deposit memos whose ABI length
-     word runs one past the string. */
+
+     Reject ANY NUL inside the declared length, including a trailing one.
+
+     An earlier version of this check exempted trailing NULs on the grounds
+     that nothing is hidden behind them. That was wrong twice over. It was
+     adopted to make two test fixtures pass -- fixtures that declare 59 bytes
+     for a 58-byte memo -- which is the one thing the release invariant forbids:
+     tests adapt to disclosure, disclosure never weakens for a test. And it
+     accepts a length word that does not describe its own content, which is the
+     same non-canonical ABI encoding that the offset-word validation already
+     refuses. A declaration the device cannot trust is not made trustworthy by
+     the bytes it misdescribes happening to be zero.
+
+     The caller's UNPARSED path discloses the raw bytes with a length-aware
+     writer, so nothing is lost by refusing to parse. */
   for (uint16_t i = 0; i < size; i++) {
-    if (memoBuf[i] != '\0') continue;
-    for (uint16_t j = i + 1; j < size; j++) {
-      if (memoBuf[j] != '\0') return THORCHAIN_MEMO_UNPARSED;
-    }
-    /* Trailing padding only. memoBuf is already NUL-terminated at i, so
-       strtok below stops there on its own -- nothing further to do. */
-    break;
+    if (memoBuf[i] == '\0') return THORCHAIN_MEMO_UNPARSED;
   }
 
   tok = strtok(memoBuf, ":");

unittests/firmware/thorchain.cpp

@@ -13,22 +13,34 @@ TEST(Thorchain, MemoWithEmbeddedNulIsNotParsed) {
      the buffer to strtok, which stops at the first NUL. A memo such as
      "=:ETH.ETH:<dest>:0\0:affiliate:75" is signed in FULL -- the EVM caller
      passes the true ABI length -- but parsing and confirmation stopped at the
-     zero byte, so the affiliate suffix was never shown.
-     There is no honest way to parse a field that lies about where it ends, so
-     the parser must fail closed and let the caller disclose the raw bytes with
-     a length-aware writer. */
-  static const char kMemo[] =
+     zero byte, so the affiliate suffix was never shown. */
+  static const char kHiddenSuffix[] =
       "=:ETH.ETH:0x41e5560054824ea6b0732e656e3ad64e20e94e45:0\0:affiliate:75";
-  /* sizeof - 1 keeps the embedded NUL and drops only the literal's terminator.
-   */
-  EXPECT_EQ(THORCHAIN_MEMO_UNPARSED,
-            thorchain_parseConfirmMemo(kMemo, sizeof(kMemo) - 1));
+  EXPECT_EQ(
+      THORCHAIN_MEMO_UNPARSED,
+      thorchain_parseConfirmMemo(kHiddenSuffix, sizeof(kHiddenSuffix) - 1));
+
+  /* A TRAILING NUL inside the declared length is refused too.
 
-  /* A trailing NUL at the very end is the same rule: the length says those
-     bytes are content. */
-  static const char kTrailing[] = "=:ETH.ETH:0xabc:0\0";
+     An earlier fix exempted this case, reasoning that nothing is hidden behind
+     bytes that are all zero. It was adopted to make two fixtures pass -- and
+     those fixtures were wrong, declaring 59 bytes for a 58-byte memo. Relaxing
+     firmware disclosure to satisfy a test is the one move this release's
+     invariant forbids.
+
+     It is also inconsistent: a length word that does not describe its own
+     content is a non-canonical ABI encoding, which the offset-word validation
+     already refuses. A declaration the device cannot trust does not become
+     trustworthy because the bytes it misdescribes happen to be zero. */
+  static const char kTrailingNul[] =
+      "ADD:ETH.ETH:0xc5b2608927ea95ed43f842f553e3a27b09c050e8:420\0";
   EXPECT_EQ(THORCHAIN_MEMO_UNPARSED,
-            thorchain_parseConfirmMemo(kTrailing, sizeof(kTrailing) - 1));
+            thorchain_parseConfirmMemo(kTrailingNul, sizeof(kTrailingNul) - 1));
+
+  /* The SAME memo with a truthful length parses normally. This is the control:
+     it shows the rule rejects the misdeclaration, not the memo. */
+  EXPECT_NE(THORCHAIN_MEMO_UNPARSED,
+            thorchain_parseConfirmMemo(kTrailingNul, sizeof(kTrailingNul) - 2));
 }
 
 TEST(Thorchain, ThorchainGetAddress) {