Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arc60 Proposal update and Reference implementation #8

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

ehanoc
Copy link

@ehanoc ehanoc commented May 18, 2024

ARC60 Proposal Change + Reference Implementation

Proposal Changes

ARC60Domain

  • During implementation noticed that is unnecessary and just extra. Also doesn't protect against an informed attacker that is aware that the domain separator is used and the right hand side of the payload is what is expected to be signed and validated. The validation checks on the payload need to be sufficient on their own and a domain separator becomes superfluous.

SCOPE Changes

Auth => CHALLENGE32 : Most auth protocols do only require a 256 bit challenge to be signed and with that is good to be specific on the type. Requires no SCHEMA to validate, catering and helping to implement in hardware as well. The app just needs to check 32 byte size, since it cant ever be a valid tx anyway. This scope makes it very obvious and specific.

MX_RANDOM : For backwards compatibility. Some wallets have private APIs that cater to some apps and this is the standard. MX prefix + some random data. Also making easier to implement withou a hard requirement for JSON schema, wallet just needs to validate the MX prefix and make sure its not a transaction.

LSIG => LSIG_TEMPLATE: To avoid signing unknown data types and LSIG's, working with templates (ARC47) guarantees this IF we expand the validations to include a HASH OF THE TEMPLATE and description and NOT on the compiled LSIG after the values are set in. If we hash the templates and whitelist them in the implementation of the wallet, we CAN GUARANTEE that we only sign known LSIG's. Including the hash on the wallet implement instead of the actual templated programs, also makes the implementation a lot easier for hardware wallets (if they want to check this themselves) as doing integrity checks against hashes vs the entire LSIG's templates is a lot faster, cheap memory requirements and fast.

The LSIG request is of the following format:

export interface ARC47LsigTemplateRequest {
    LogicSignatureDescription: {  // sha512_256 hash of this object is the template hash
        name: string,
        description: string,
        program: string, // base64 encoded TEAL program
        variables: Array<{
            variable: string,
            name: string,
            type: string,
            description: string
        }>
    },
    hash: string, // hash of the program, AFTER values replaced and compiled
    values: { // to replace on the program
        [key: string]: any
    }
}
  • Wallets MUST keep a list of sha512_256 hashes of the ARC47LsigTemplateRequest.LogicSignatureDescription and whitelist only templates that matches those hashes

Reference Implementation and Test Suite

Instructions

Requires network as without a standalone compiler we need to reach a node to compile TEAL.

$ cd assets/arc-0060
$ yarn
$ yarn test:cov

Output

 PASS  ./arc60wallet.api.spec.ts (5.788 s)
  ARC60 TEST SUITE
    rawSign
      ✓ (OK) should sign data correctly (35 ms)
      ✓ (FAILS) should throw error for shorter incorrect length seed (1 ms)
      ✓ (FAILS) should throw error for longer incorrect length seed (1 ms)
    getPublicKey
      ✓ (OK) should return the correct public key (1 ms)
      ✓ (FAILS) should throw error for shorter incorrect length seed (1 ms)
      ✓ (FAILS) should throw error for longer incorrect length seed (1 ms)
    Reject unknown LSIGs
      ✓ (FAILS) Tries to sign with any scope if "Program" is present (38 ms)
    SCOPE == CHALLENGE32
      ✓ (OK) Signs random 32 byte challenge (3 ms)
      ✓ (FAILS) Tries to sign with bad size longer random data as CHALLENGE32 (2 ms)
      ✓ (FAILS) Tries to sign with bad size shorter random data as CHALLENGE32 (1 ms)
    SCOPE == MX_RANDOM
      ✓ (OK) Signs random 32 byte challenge with MX prefix (3 ms)
      ✓ (OK) Signs 512 bytes random data with MX prefix (1 ms)
      ✓ (FAILS) Tries to sign but no MX prefix is present (1 ms)
    SCOPE == INVALID
      ✓ (FAILS) Tries to sign with invalid scope (2 ms)
    SCOPE == LSIG_TEMPLATE
      ✓ (FAIL) Fails to sign LSIG_TEMPLATE program, templated program does not match known hashes (123 ms)
      ✓ (FAIL) Bad LSIG_TEMPLATE request, fails schema validation (65 ms)
      ✓ (OK) Signs LSIG_TEMPLATE program, templated program is known, values replaced and signature produced (1063 ms)
    ARC31 AUTH
      ✓ (OK) Sign ARC31 message (40 ms)
      ✓ (FAILS) Tries to sign ARC31 with bad schema (96 ms)
    Invalid or Unkown Signer
      ✓ (FAILS) Tries to sign with bad signer (1 ms)
    Unknown Encoding
      ✓ (FAILS) Tries to sign with unknown encoding (1 ms)

--------------------|---------|----------|---------|---------|-------------------
File                | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
--------------------|---------|----------|---------|---------|-------------------
All files           |     100 |      100 |     100 |     100 |                   
 arc60wallet.api.ts |     100 |      100 |     100 |     100 |                   
--------------------|---------|----------|---------|---------|-------------------
Test Suites: 1 passed, 1 total
Tests:       21 passed, 21 total
Snapshots:   0 total
Time:        5.999 s, estimated 7 s
Ran all test suites.
Done in 6.97s.

TODO

arc-0060.md - Not done. In imcomplete state. Needs to be updated to reflect these, if accepted.

@k13n
Copy link

k13n commented May 20, 2024

Thanks, I like the proposed scope changes, but the CHALLENGE32 scope is very dangerous as it is proposed right now. It is correct that it cannot be used to sign a valid transaction, because any valid transaction is longer than 32 bytes. But an attacker can craft a 32-byte long delegated logic signature that accepts all transactions. To illustrate this, I created the following gist: https://gist.github.com/k13n/698703fd83d5c49bcba353fbd3193a36

In this example, the delegated logic-sig looks like this:

byte 0xa1fd1d7b75783e970a0c875e8833f2
pop
int 1

The important bit is the last line that returns one, which means the lsig approves the transaction. The remaining lines are just fillers to bring the lsig to 25 bytes when compiled. The remaining 7 bytes to reach 32 bytes come from the Program prefix that's added to sign a compiled lsig.

The compiled lsig plus the Program prefix is 32 bytes long and looks like this in hex:
50726f6772616d0120010126010fa1fd1d7b75783e970a0c875e8833f2284822

If an attacker manages to convince a user to sign this 32-byte payload with her address, the attacker can authorize any transaction from the victim's account (as is shown in the attached gist). With the proposed CHALLENGE32 scope, the attacker could easily obtain such a signature.

To prevent this, I think we need to check if the CHALLENGE32 payload is prefixed with Program and if that's the case we reject it. The probability that a truly random 32-byte challenge has this prefix is 1 / 8**7 = 1 / 2097152. That is, one out of two million randomly generated 32-byte challenges will be rejected by a wallet.

EDIT: It's 1 / 256**7, not 1 / 8**7

… lsig request arc47

Signed-off-by: ehanoc <ehanoc@protonmail.com>
@ehanoc
Copy link
Author

ehanoc commented May 20, 2024

Thanks, I like the proposed scope changes, but the CHALLENGE32 scope is very dangerous as it is proposed right now. It is correct that it cannot be used to sign a valid transaction, because any valid transaction is longer than 32 bytes. But an attacker can craft a 32-byte long delegated logic signature that accepts all transactions. To illustrate this, I created the following gist: https://gist.github.com/k13n/698703fd83d5c49bcba353fbd3193a36

In this example, the delegated logic-sig looks like this:

byte 0xa1fd1d7b75783e970a0c875e8833f2
pop
int 1

The important bit is the last line that returns one, which means the lsig approves the transaction. The remaining lines are just fillers to bring the lsig to 25 bytes when compiled. The remaining 7 bytes to reach 32 bytes come from the Program prefix that's added to sign a compiled lsig.

The compiled lsig plus the Program prefix is 32 bytes long and looks like this in hex: 50726f6772616d0120010126010fa1fd1d7b75783e970a0c875e8833f2284822

If an attacker manages to convince a user to sign this 32-byte payload with her address, the attacker can authorize any transaction from the victim's account (as is shown in the attached gist). With the proposed CHALLENGE32 scope, the attacker could easily obtain such a signature.

To prevent this, I think we need to check if the CHALLENGE32 payload is prefixed with Program and if that's the case we reject it. The probability that a truly random 32-byte challenge has this prefix is 1 / 8**7 = 1 / 2097152. That is, one out of two million randomly generated 32-byte challenges will be rejected by a wallet.

It's added now.

… time for LSIG_TEMPLATE

Signed-off-by: ehanoc <ehanoc@protonmail.com>
assets/arc-0060/auth-schema.json Show resolved Hide resolved
export enum ScopeType {
UNKNOWN = -1,
CHALLENGE32 = 0,
MX_RANDOM = 1,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale of random bytes now? We have had very long discussions on "random is not good".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MX_RANDOM is for backwards compatibility purposes as there are some private API's out there that use it. The MX prefix makes it safe to sign as i can't be anything else than a arbitrary message. Still open to remove / tweak it though.

We can test the implementation is an adversarial scenario if someone finds one.


toSign = decodedData
break;
case ScopeType.LSIG_TEMPLATE:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LSIG_TEMPLATE thing is strange/no-user-friendly. IMO, we either allow the signing of LSIGS (users are responsible for passing the correct data with a schema) or remove this scope.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree on this. If we use templates and integrity checks for known templates; this API becomes safe as no unknown LSIGS can be signed. Just allowing LSIGS will open a can of worms and nightmare for apps / wallets to offer safe.

I see very little to no risk in doing so by using templates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants