docs(textual): Require signing over raw bytes #12910

amaury1093 · 2022-08-11T15:47:49Z

Description

Security concerns around what's signed, pulled out from #12785 (comment).

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

peterbourgon · 2022-08-11T16:22:24Z

Relevant comments:

tl;dr: signatures verify the integrity of the signed bytes, not a semantic transformation of those bytes

arirubinstein · 2022-08-11T22:20:28Z

docs/architecture/adr-050-sign-mode-textual.md

+Recall that the transaction bytes merklelized on chain are the Protobuf binary serialization of [TxRaw](https://github.com/cosmos/cosmos-sdk/blob/v0.46.0/proto/cosmos/tx/v1beta1/tx.proto#L33), which contains the `body_bytes` and `auth_info_bytes`. Moreover, the transaction hash is defined as the SHA256 hash of the `TxRaw` bytes. We require that the user signs over these canonical bytes in SIGN_MODE_TEXTUAL, more specifically over the following string:
+
+```
+*Hash of raw bytes: <base64(sha256(<body_bytes> ++ " " ++ <auth_info_bytes>))>


Is there a reason base64 is used as the representation here instead of hex bytes? There could result in ambiguity depending on how the transaction can be tampered with. the digest should be represented in a simpler byte representation, e.g. hex-encoded with expected length validation on read

e.g. here are two base64-encoded payloads which differ in serialized representation but the same raw bytes:

payload1="QzNWwq==" payload2="QzNWwr==" % echo -n $payload1 | shasum -a256 b679c1a6f416eb2eb18af4dce3347c3aa74e559aa88a8ef0ef4d6eefb0939c32 - % echo -n $payload2 | shasum -a256 a7e3cd272e296994116cdf160221a26e71ca6b800abb5c62e90086c245fa115e - % echo -n $payload1 | base64 -d | xxd 00000000: 4333 56c2 C3V. % echo -n $payload2 | base64 -d | xxd 00000000: 4333 56c2 C3V.

Additionally, while it doesn't appear to immediately be exploitable in the current form, would it be possible to utilize a scheme involving prepending the length of each payload before each, and/or adopt ASN.1/DER representation of the bytes here in the event " " becomes significant?
e.g. H(len(param1)||param1||separator||len(param2)||param2)

Agreed, but more specifically...

We require that the user signs over these canonical bytes in SIGN_MODE_TEXTUAL, more specifically over the following string:

The string is a rendering of the canonical bytes, and not the canonical bytes themselves. Whatever the on-chain representation is, that's what should be signed, directly. Signatures assert integrity only for the literal msg bytes provided.

Not sure we need a separator at all, but length prefixing each part makes sense

I added length prefix in b15409d.

@arirubinstein Thanks for flagging the non-uniqueness of base64 encoding, I actually wasn't aware. We also use base64 in value renderers. This means that 2 different SignDocTextuals (i.e. 2 different string arrays) can represent the same proto Tx, which breaks the spec on bijectivity. Maybe it's worth indeed considering using hex everywhere.

I would prefer to use HEX for everything personally. Base64 is such a mess.

Switched to capital hex

Note1: there is also z-base-32 which is more compact, readable and doesn't have the padding bytes issue.

Note2: we don't need to length prefix the last part. So <len(body_bytes) ++ <body_bytes> ++ <auth_info_bytes> is good enough.

For z-base-32 support, here is the spec (good read!): https://philzimmermann.com/docs/human-oriented-base-32-encoding.txt

BTW, Algorand is using base32.

Linking: #13141

docs/architecture/adr-050-sign-mode-textual.md

Co-authored-by: Aaron Craelius <aaron@regen.network>

aaronc · 2022-08-16T21:04:35Z

One additional thing we should do is make sure that TxRaw bytes are encoded according to ADR 027 in the tx decoder. This is actually an existing malleability issue for SIGN_MODE_DIRECT that we should address.

amaury1093 · 2022-08-17T08:35:50Z

@aaronc That has already been addressed in #9743.

aaronc · 2022-08-17T08:55:58Z

Sorry I forgot!

amaury1093 · 2022-08-18T08:47:08Z

This is R4R again. We sign over a hex string now.

Edit: about using Hex instead of base64 in other parts of the spec, I did another PR: #12954

robert-zaremba

LGTM. Added a suggestion to consider z-base-32

## Description ref: #12910 (comment) --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

webmaster128 · 2022-08-22T16:25:30Z

docs/architecture/adr-050-sign-mode-textual.md

+Recall that the transaction bytes merklelized on chain are the Protobuf binary serialization of [TxRaw](https://github.com/cosmos/cosmos-sdk/blob/v0.46.0/proto/cosmos/tx/v1beta1/tx.proto#L33), which contains the `body_bytes` and `auth_info_bytes`. Moreover, the transaction hash is defined as the SHA256 hash of the `TxRaw` bytes. We require that the user signs over these bytes in SIGN_MODE_TEXTUAL, more specifically over the following string:
+
+```
+*Hash of raw bytes: <HEX(sha256(<len(body_bytes) ++ <body_bytes> ++ len(auth_info_bytes) ++ <auth_info_bytes>))>


If we use <foo> to mark the insertion of a variable in the document, can we just use a single set of brackets to include the calculation like this?

Suggested change

*Hash of raw bytes: <HEX(sha256(<len(body_bytes) ++ <body_bytes> ++ len(auth_info_bytes) ++ <auth_info_bytes>))>

*Hash of raw bytes: <HEX(sha256(len(body_bytes) ++ <body_bytes> ++ len(auth_info_bytes) ++ auth_info_bytes))>

Also we need to specify how len is encoded to bytes. I suggest 4 byte big endian uint32.

Good point. I had varint in mind, just to accomodate for arbitrarily long txs.

Hmm, varint is complex and not unique. Also we don't need to optimize for output length here. What about 8 byte big endian uint64 instead?

OK. uint64 should be plenty.

Just wanted to point out that ASN.1/DER encoding does this natively and has a go lib as well.

Yeah, but pulling in a DER dependency for each client comes at a cost, especially when not used consistently thoughout the stack. For Ethereum signatures I had to write DER encoding/decoding by hand in TypeScript, which is not fun. But then again 50 lines of code also don't justify pulling in the whole fully featured ASN.1/DER dependency (if it exists in reasonable quality).

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

…cosmos-sdk into am/textual-sign-over-bytes

docs/architecture/adr-050-sign-mode-textual.md

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

amaury1093 · 2022-09-05T15:17:23Z

Putting automerge on.

docs(textual): Require signing over raw bytes

6decc11

amaury1093 requested a review from a team as a code owner August 11, 2022 15:47

github-actions bot added the Type: ADR label Aug 11, 2022

amaury1093 mentioned this pull request Aug 11, 2022

docs(textual): custom Msg renderers #12785

Merged

19 tasks

amaury1093 requested a review from robert-zaremba August 11, 2022 15:53

arirubinstein reviewed Aug 11, 2022

View reviewed changes

peterbourgon reviewed Aug 14, 2022

View reviewed changes

docs/architecture/adr-050-sign-mode-textual.md Show resolved Hide resolved

webmaster128 reviewed Aug 15, 2022

View reviewed changes

docs/architecture/adr-050-sign-mode-textual.md Outdated Show resolved Hide resolved

sign lenght prefix

b15409d

amaury1093 force-pushed the am/textual-sign-over-bytes branch from 42f2671 to b15409d Compare August 16, 2022 11:28

Update docs/architecture/adr-050-sign-mode-textual.md

8f90776

Co-authored-by: Aaron Craelius <aaron@regen.network>

amaury1093 added 2 commits August 18, 2022 10:43

Use capital hex

58a5eda

Merge branch 'main' into am/textual-sign-over-bytes

55545bc

amaury1093 mentioned this pull request Aug 18, 2022

fix(textual): Use HEX instead of base64 #12954

Merged

19 tasks

aaronc approved these changes Aug 18, 2022

View reviewed changes

robert-zaremba approved these changes Aug 19, 2022

View reviewed changes

webmaster128 reviewed Aug 22, 2022

View reviewed changes

Update docs/architecture/adr-050-sign-mode-textual.md

a87614c

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

amaury1093 mentioned this pull request Aug 22, 2022

JSON: Display bytes as hex instead of base64 #12994

Closed

amaury1093 added 2 commits August 23, 2022 16:14

Add details about len()

42e4cdb

Merge branch 'am/textual-sign-over-bytes' of ssh://github.com/cosmos/…

0b551f3

…cosmos-sdk into am/textual-sign-over-bytes

webmaster128 approved these changes Aug 24, 2022

View reviewed changes

docs/architecture/adr-050-sign-mode-textual.md Outdated Show resolved Hide resolved

robert-zaremba mentioned this pull request Sep 2, 2022

Use z-base32 for bytes encoding (outside of protobuf) #13141

Open

Update docs/architecture/adr-050-sign-mode-textual.md

4b0f94f

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Sep 5, 2022

Merge branch 'main' into am/textual-sign-over-bytes

56c557a

mergify bot merged commit fe90139 into main Sep 5, 2022

mergify bot deleted the am/textual-sign-over-bytes branch September 5, 2022 15:24

JimLarson mentioned this pull request Oct 12, 2022

feat: string and message value renderers for textual #13510

Merged

19 tasks

MHHukiewitz mentioned this pull request Apr 18, 2023

Cosmos signature verification and fix aleph-im/aleph-sdk-python#20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(textual): Require signing over raw bytes #12910

docs(textual): Require signing over raw bytes #12910

amaury1093 commented Aug 11, 2022 •

edited

Loading

peterbourgon commented Aug 11, 2022

arirubinstein Aug 11, 2022 •

edited

Loading

peterbourgon Aug 14, 2022 •

edited

Loading

aaronc Aug 15, 2022

amaury1093 Aug 16, 2022 •

edited

Loading

alexanderbez Aug 17, 2022

plafer Aug 17, 2022

amaury1093 Aug 18, 2022 •

edited

Loading

robert-zaremba Aug 19, 2022

robert-zaremba Aug 19, 2022

robert-zaremba Sep 2, 2022

aaronc commented Aug 16, 2022 •

edited

Loading

amaury1093 commented Aug 17, 2022

aaronc commented Aug 17, 2022

amaury1093 commented Aug 18, 2022 •

edited

Loading

robert-zaremba left a comment

webmaster128 Aug 22, 2022

amaury1093 Aug 22, 2022

webmaster128 Aug 23, 2022

amaury1093 Aug 23, 2022

arirubinstein Aug 24, 2022

webmaster128 Aug 24, 2022

amaury1093 commented Sep 5, 2022

	*Hash of raw bytes: <HEX(sha256(<len(body_bytes) ++ <body_bytes> ++ len(auth_info_bytes) ++ <auth_info_bytes>))>
	*Hash of raw bytes: <HEX(sha256(len(body_bytes) ++ <body_bytes> ++ len(auth_info_bytes) ++ auth_info_bytes))>

docs(textual): Require signing over raw bytes #12910

docs(textual): Require signing over raw bytes #12910

Conversation

amaury1093 commented Aug 11, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

peterbourgon commented Aug 11, 2022

arirubinstein Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

peterbourgon Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Aug 16, 2022 • edited Loading

amaury1093 commented Aug 17, 2022

aaronc commented Aug 17, 2022

amaury1093 commented Aug 18, 2022 • edited Loading

robert-zaremba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 commented Sep 5, 2022

amaury1093 commented Aug 11, 2022 •

edited

Loading

arirubinstein Aug 11, 2022 •

edited

Loading

peterbourgon Aug 14, 2022 •

edited

Loading

amaury1093 Aug 16, 2022 •

edited

Loading

amaury1093 Aug 18, 2022 •

edited

Loading

aaronc commented Aug 16, 2022 •

edited

Loading

amaury1093 commented Aug 18, 2022 •

edited

Loading