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

fix: _abi_decode() validation #3626

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Sep 26, 2023

What I did

tracking for GHSA-cx2q-hfxr-rj97

How I did it

How to verify it

Commit message

    `_abi_decode()` does not validate input when it is nested in certain
    expressions. the following example gets correctly validated (bounds
    checked):

    ```vyper
        x: uint8 = _abi_decode(slice(msg.data, 4, 32), uint8)
    ```

    however, the following example is not bounds checked:

    ```vyper
    @external
    def abi_decode(x: uint256) -> uint256:
        a: uint256 = convert(
            _abi_decode(
                slice(msg.data, 4, 32),
                (uint8)
            ),
            uint256
        )
        return a  # abi_decode(256) returns: 256
    ```

    the issue is caused because the `ABIDecode()` builtin tags its output
    with `encoding=Encoding.ABI`, but this does not result in validation
    until that itself is passed to `make_setter` (which is called for
    instance when generating an assignment or return statement).

    the issue can be triggered by constructing an example where the output
    of `ABIDecode()` is not internally passed to `make_setter` or other
    input validating routine.

    this commit fixes the issue by calling `make_setter` in `ABIDecode()`
    before returning the output buffer, which causes validation to be
    performed. note that this causes a performance regression in the common
    (and majority of) cases where `make_setter` is immediately called on the
    result of `ABIDecode()` because a redundant memory copy ends up being
    generated (like in the aforementioned examples: in a plain assignment or
    return statement). however, fixing this performance regression is left
    to future work in the optimizer.

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@charles-cooper charles-cooper marked this pull request as ready for review September 26, 2023 17:27
@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2023

Codecov Report

Merging #3626 (0a1dfe1) into master (f224d83) will decrease coverage by 0.07%.
Report is 3 commits behind head on master.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #3626      +/-   ##
==========================================
- Coverage   89.12%   89.05%   -0.07%     
==========================================
  Files          86       86              
  Lines       11407    11460      +53     
  Branches     2595     2606      +11     
==========================================
+ Hits        10166    10206      +40     
- Misses        819      832      +13     
  Partials      422      422              
Files Coverage Δ
vyper/builtins/functions.py 90.58% <100.00%> (+0.06%) ⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@trocher trocher left a comment

Choose a reason for hiding this comment

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

Lgtm

@charles-cooper charles-cooper merged commit d438d92 into vyperlang:master Sep 26, 2023
82 checks passed
@charles-cooper charles-cooper deleted the fix/abi_decode_clamp branch September 26, 2023 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants