Skip to content

concat built-in can corrupt memory in vyper

High severity GitHub Reviewed Published Jan 18, 2024 in vyperlang/vyper • Updated Oct 10, 2024

Package

pip vyper (pip)

Affected versions

>= 0.3.0, <= 0.3.10

Patched versions

0.4.0

Description

Summary

concat built-in can write over the bounds of the memory buffer that was allocated for it and thus overwrite existing valid data. The root cause is that the build_IR for concat doesn't properly adhere to the API of copy functions (for >=0.3.2 the copy_bytes function).

A contract search was performed and no vulnerable contracts were found in production.

Tracked in issue vyperlang/vyper#3737

Details

The build_IR allocates a new internal variable for the concatenation: https://github.com/vyperlang/vyper/blob/3b310d5292c4d1448e673d7b3adb223f9353260e/vyper/builtins/functions.py#L534-L550

Notice that the buffer is allocated for the maxlen + 1 word to actually hold the length of the array.

Later the copy_bytes function is used to copy the actual source arguments to the destination: https://github.com/vyperlang/vyper/blob/3b310d5292c4d1448e673d7b3adb223f9353260e/vyper/builtins/functions.py#L569-L572

The dst_data is defined via:

  • data ptr - to skip the 1 word that holds the length
  • offset - to skip the source arguments that were already written to the buffer
    • the offset is increased via: ["set", ofst, ["add", ofst, arglen]], ie it is increased by the length of the source argument

Now, the copy_bytes function has multiple control flow paths, the following ones are of interest:

  1. https://github.com/vyperlang/vyper/blob/3b310d5292c4d1448e673d7b3adb223f9353260e/vyper/codegen/core.py#L270-L273
  2. https://github.com/vyperlang/vyper/blob/3b310d5292c4d1448e673d7b3adb223f9353260e/vyper/codegen/core.py#L301-L320

Note that the function itself contains the following note:
https://github.com/vyperlang/vyper/blob/3b310d5292c4d1448e673d7b3adb223f9353260e/vyper/codegen/core.py#L245-L247

That is we can ask for a copy of 1B yet a whole word is copied.

Consider the first interesting path - if the dst_data's distance to the end of the concat data buffer is < 32B, the copy_op = STORE(dst, LOAD(src)) from copy_bytes will result in buffer overflow as it essentially will mstore to dst_data the mload of the source (mload will load whole word and the distance of the dst_data to the word boundary is <32B).

From the two mentioned paths in copy_bytes it can be seen that both sources from memory and storage can cause the corruption.

PoC

The main attack vector that was found was when the concat is inside an internal function. Suppose we have an external function that calls internal one. In such case the address space is divided such that the memory for the internal function is in lower portion of the adr space. As such the buffer overflow can overwrite valid data of the caller.

Here is a simple example:

#@version ^0.3.9

@internal
def bar() -> uint256:
    sss: String[2] = concat("a", "b") 
    return 1


@external
def foo() -> int256:
    a: int256 = -1
    b: uint256 = self.bar()
    return a 

foo should clearly return -1, but it returns 452312848583266388373324160190187140051835877600158453279131187530910662655

-1 was used intentionally due to its bit structure but the value here is fairly irelevant. In this example during the second iteration of the for loop in the build_IR mload to dst+1 will be executed (because len('a') == 1), thus the function will write 1B over the bounds of the buffer. The string 'b' is stored such that its right-most byte is a zero byte. So a zero byte will be written over the bounds. So when -1 is considered it's left-most B will be overwritten to all 0. Therefore it can be seen: 452312848583266388373324160190187140051835877600158453279131187530910662655 == (2**248-1) will output True.

IR

If we look at the contract's IR (vyper --no optimize -f ir), we see:

# Line 30
                          /* a: int256 = -1 */ [mstore, 320, -1 <-1>],

And for the second iteration of the loop in concat:

 len,
                        [mload, arg],
                        [seq,
                          [with,
                            src,
                            [add, arg, 32],
                            [with,
                              dst,
                              [add, [add, 256 <concat destination>, 32], concat_ofst],
                              [mstore, dst, [mload, src]]]],
                          [set, concat_ofst, [add, concat_ofst, len]]]]],
                    [mstore, 256 <concat destination>, concat_ofst],
                    256 <concat destination>]],

So the address of the int is 320.

The dst is defined as: [add, [add, 256 <concat destination>, 32], concat_ofst],.
In the second iteration the concat_ofst will be 1 because len('a)==1 so 256+32+1 = 289. Now this address will be mstored to - so the last mstored B will have the address 289+32=320 which clearly overlaps with the address of the int a.

PoC 2

Due to how immutables are handled, they can be corrupted too:

#@version ^0.3.9

i: immutable(int256)

@external
def __init__():
    i = -1
    s: String[2] = concat("a", "b")

@external
def foo() -> int256:
    return i

Output of calling foo() = 452312848583266388373324160190187140051835877600158453279131187530910662655.

Impact

The buffer overflow can result in the change of semantics of the contract. The overflow is length-dependent and thus it might go unnoticed during contract testing.

However, certainly not all usages of concat will result in overwritten valid data as we require it to be in an internal function and close to the return statement where other memory allocations don't occur.

Concluding remarks

The bug based on the fast path in copy_bytes was likely introduced in: 548d35d720fb6fd8efbdc0ce525bed259a73f0b9. git bisect was used between v0.3.1 and v0.3.2, forge test was run and the test asserted that the function indeed returns -1.

For the general case, 0.3.0 and 0.3.1 are also affected.

References

@charles-cooper charles-cooper published to vyperlang/vyper Jan 18, 2024
Published by the National Vulnerability Database Jan 18, 2024
Published to the GitHub Advisory Database Jan 19, 2024
Reviewed Jan 19, 2024
Last updated Oct 10, 2024

Severity

High

CVSS overall score

This score calculates overall vulnerability severity from 0 to 10 and is based on the Common Vulnerability Scoring System (CVSS).
/ 10

CVSS v3 base metrics

Attack vector
Network
Attack complexity
Low
Privileges required
None
User interaction
None
Scope
Unchanged
Confidentiality
Low
Integrity
Low
Availability
Low

CVSS v3 base metrics

Attack vector: More severe the more the remote (logically and physically) an attacker can be in order to exploit the vulnerability.
Attack complexity: More severe for the least complex attacks.
Privileges required: More severe if no privileges are required.
User interaction: More severe when no user interaction is required.
Scope: More severe when a scope change occurs, e.g. one vulnerable component impacts resources in components beyond its security scope.
Confidentiality: More severe when loss of data confidentiality is highest, measuring the level of data access available to an unauthorized user.
Integrity: More severe when loss of data integrity is the highest, measuring the consequence of data modification possible by an unauthorized user.
Availability: More severe when the loss of impacted component availability is highest.
CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:L/I:L/A:L

EPSS score

0.099%
(42nd percentile)

CVE ID

CVE-2024-22419

GHSA ID

GHSA-2q8v-3gqq-4f8p

Source code

Credits

Loading Checking history
See something to contribute? Suggest improvements for this vulnerability.