release: fold the Maya memo fix into develop - #494
Merged
Conversation
… chains
THORChain and Maya both copy a byte-counted memo into a buffer and then parse it
with strtok, which stops at the first NUL. Every byte inside the declared length
is signed, so a memo like "=:ETH.ETH:<dest>:0\0:affiliate:75" confirmed as if it
ended at the zero byte while the affiliate suffix stayed in the calldata.
Both now refuse any NUL inside the declared length. A length word that does not
describe its own content is a non-canonical encoding -- the same thing the ABI
offset-word validation already rejects -- and there is no honest way to parse it.
The caller's unparsed path discloses the raw bytes with a length-aware writer.
Maya had more than that. It is a fork of the THORChain code and never received
the earlier fixes, so it still had:
strlcpy(memoBuf, swapStr, size) dropped the memo's last character; an
affiliate fee of "75" bps rendered as "7"
sizeof(memo) at both callers handed the parser a fixed array's trailing
zeros as if they were content
confirm(..., "%s", memo) the unparsed fallback truncated at the same
NUL, which would have moved the defect one
screen later rather than removing it
Found by auditing every confirm body fed a protobuf bytes field through "%s",
which is what #465 asks for. Reading the THORChain patch would not have found
it; only looking at every caller did.
Two add_liquidity fixtures declared 59 bytes for a 58-byte memo and now fail
against the stricter rule. They were wrong: the 59th byte is ABI padding. Fixed
in python-keepkey with signatures repinned, and repinned here.
Unit tests cover an interior NUL, a trailing NUL, and the same memo with a
truthful length -- the last is the control proving the rule rejects the
declaration rather than the memo.
fix(memo): refuse a length that misdescribes its own content, in both chains
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the 7.14.2 line on develop. #418 merged before the Maya fixes landed on the release branch, so develop is missing them.
Contents: the strict NUL rule for THORChain plus the same memo fixes Maya never received (strlcpy truncation, sizeof->strnlen, %s->confirm_bytes), corrected fixture length words, and the pyk repin.
7.15 stages on top of this.