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

Limit inflation in ABI decoded output #4537

Closed
ricmoo opened this issue Jan 11, 2024 · 2 comments
Closed

Limit inflation in ABI decoded output #4537

ricmoo opened this issue Jan 11, 2024 · 2 comments
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6

Comments

@ricmoo
Copy link
Member

ricmoo commented Jan 11, 2024

Describe the Feature

TL;DR: When decoding ABI-encoded data, it is possible to use a malicious ABI fragment and specially constructed data to cause excessive memory and processing power when decoding dynamic arrays, resulting in a denial-of-service vector.

More Background

When dynamic-length items are ABI-encoded, a fixed-length item is inserted, which is a pointer to a position in the data after all fixed-length items, which encodes the dynamic value.

This form of data may be intentional, as it can be used as a form of compression for data passed to a contract. It works by using the same pointer for multiple fields. For example, imagine the following example:

// The Interface
iface = new Interface([ "function foo(string a, string b) returns (uint)" ]);

// The standard encoding: (we will ignore the selector `0x124a83fa` in the output)
iface.encodeFunctionData("foo", [ "hello world", "hello world" ]);

0x00: 0000000000000000000000000000000000000000000000000000000000000040 // Pointer to 0x40
0x20: 0000000000000000000000000000000000000000000000000000000000000080 // Pointer to 0x80
0x40: 000000000000000000000000000000000000000000000000000000000000000b // First string length (11)
0x60: 68656c6c6f20776f726c64000000000000000000000000000000000000000000 // First string data
0x80: 000000000000000000000000000000000000000000000000000000000000000b // Second string length (11)
0xa0: 68656c6c6f20776f726c64000000000000000000000000000000000000000000 // Second string data

// However, another (valid) encoding could exploit the fact that both strings are identical
0x00: 0000000000000000000000000000000000000000000000000000000000000040 // Pointer to 0x40
0x20: 0000000000000000000000000000000000000000000000000000000000000040 // Share the pointer***
0x40: 000000000000000000000000000000000000000000000000000000000000000b // String length (11)
0x60: 68656c6c6f20776f726c64000000000000000000000000000000000000000000 // String data

*** Notice the second element in the ABI-encoding (offset 0x20) can just
    share the same reference as the first element, which saves 64 bytes.

However, careful construction of ABI-encoded data allows this technique to re-use pointers excessively, which will exhaust system memory and processing power, causing a DoS attack vector.

So, this feature is to introduce a sane default inflation ratio and a means to adjust that ratio if needed.

An article will be released soon which outlines the issue more, which this issue will link to.

Code Example

No response

@ricmoo ricmoo added the enhancement New feature or improvement. label Jan 11, 2024
@ricmoo
Copy link
Member Author

ricmoo commented Jan 11, 2024

I'm still experimenting with some details, but the current proposed fix will use a default ratio of 1024, which means up to 1024 times the amount of decoded data can be read from the encoded data.

However to adjust this, the following will be possible:

// Disable any pointer reuse; only allow each byte to be effectively be read once
ethers.AbiCoder._setDefaultMaxInflation(1);

// Disable limit checking; allows decoding arbitrary-sized output, but may exhaust memory and the CPU.
ethers.AbiCoder._setDefaultMaxInflation(0);

// Allow custom levels of compression; allow for 1 million times inflation
ethers.AbiCoder._setDefaultMaxInflation(1000000);

I don't imagine anyone needing to adjust any of this though, and it should "just work". :)

@ricmoo ricmoo added on-deck This Enhancement or Bug is currently being worked on. minor-bump Planned for the next minor version bump. v6 Issues regarding v6 next-patch Issues scheduled for the next arch release. labels Jan 11, 2024
@ricmoo
Copy link
Member Author

ricmoo commented Jan 13, 2024

This has been published in v6.10.0.

@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. minor-bump Planned for the next minor version bump. next-patch Issues scheduled for the next arch release. labels Jan 13, 2024
@ricmoo ricmoo closed this as completed Jan 19, 2024
rrw-zilliqa added a commit to Zilliqa/ethers.js that referenced this issue Apr 29, 2024
* docs: fixed typo in jsdocs for Wallet.createRandom (ethers-io#4461)

* admin: added diff scripts for build page

* admin: updated dist files

* Added safe and finalized provider events (ethers-io#3921).

* tests: bumped Node versions for testing (ethers-io#4451)

* admin: style fix (ethers-io#4356)

* More robust FallbackProvider broadcast (ethers-io#4186, ethers-io#4297, ethers-io#4442).

* Account for provider config weight when kicking off a request in FallbackProvider (ethers-io#4298).

* Fixed ParamType formatting causing bad tuple full and minimal ABI output (ethers-io#4329, ethers-io#4479).

* Added Base network to AlchemyProvider (ethers-io#4384).

* Add auto-detected static network support to providers and allow customizing socket provider options (ethers-io#4199, ethers-io#4418, ethers-io#4441).

* Use provider-specified suggested priority fee when available, otherwise fallback onto existing logic of 1 gwei (ethers-io#4463).

* admin: updated dist files

* admin: update changelog after build-clean

* docs: Fixed some grammar in getting-started (ethers-io#4486, ethers-io#4487, ethers-io#4488)

* Fix uncatchable issue when sending transactions over JSON-RPC and provide some retry-recovery for missing v (ethers-io#4513).

* admin: update dist files

* Fix Base58 padding for string representation of binary data (ethers-io#4527).

* admin: updated dist files

* Limit decoded result imflation ratio from ABI-encoded data (ethers-io#4537).

* admin: updated dist files

* Better debugging output on fetch errors.

* docs: added StaticJsonRpcProvider to migration docs

* Fixed typo in Error string (ethers-io#4539).

* Fix EIP-712 type aliases for uint and int (ethers-io#4541).

* Added additional sepolia testnets.

* Updated third-party provider network URLs (ethers-io#4542).

* admin: updated dist files

* Fixed normalization and abstracted EIP-712 Array parsing (ethers-io#4541).

* admin: updated dist files

* tests: added testing for correct thrid-party URLs

* Updated thrid-part provider URLs for QuickNode.

* tests: rename test suite to follow naming convention

* admin: updated dist files

* Normalize EIP-712 types before computing the payload (ethers-io#4541).

* tests: add tests for EIP-712 payload aliases

* admin: updated dist files

---------

Co-authored-by: Richard Moore <me@ricmoo.com>
rrw-zilliqa added a commit to Zilliqa/ethers.js that referenced this issue Apr 29, 2024
* Fix uncatchable issue when sending transactions over JSON-RPC and provide some retry-recovery for missing v (ethers-io#4513).

* admin: update dist files

* Fix Base58 padding for string representation of binary data (ethers-io#4527).

* admin: updated dist files

* Limit decoded result imflation ratio from ABI-encoded data (ethers-io#4537).

* admin: updated dist files

* Better debugging output on fetch errors.

* docs: added StaticJsonRpcProvider to migration docs

* Fixed typo in Error string (ethers-io#4539).

* Fix EIP-712 type aliases for uint and int (ethers-io#4541).

* Added additional sepolia testnets.

* Updated third-party provider network URLs (ethers-io#4542).

* admin: updated dist files

* Fixed normalization and abstracted EIP-712 Array parsing (ethers-io#4541).

* admin: updated dist files

* tests: added testing for correct thrid-party URLs

* Updated thrid-part provider URLs for QuickNode.

* tests: rename test suite to follow naming convention

* admin: updated dist files

* Normalize EIP-712 types before computing the payload (ethers-io#4541).

* tests: add tests for EIP-712 payload aliases

* admin: updated dist files

* (feat) ZIL-5458: Add emacs backups to .gitignore
(feat) ZIL-5458: Don't validate canonical signatures

---------

Co-authored-by: Richard Moore <me@ricmoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6
Projects
None yet
Development

No branches or pull requests

1 participant