XRPL-Labs / xrpld-hooks

ISC License
94 stars 28 forks source link

PREPARE_PAYMENT_SIMPLE calls etxn_details with wrong buffer size #64

Closed vbar closed 1 year ago

vbar commented 1 year ago

Issue Description

The definition of PREPARE_PAYMENT_SIMPLE in macro.h ends with

        _08_03_ENCODE_ACCOUNT_DST          (buf_out, to_address                     );      /* account | size  22 */ \
        int64_t edlen = etxn_details((uint32_t)buf_out, PREPARE_PAYMENT_SIMPLE_SIZE);       /* emitdet | size 1?? */ \
        int64_t fee = etxn_fee_base(buf_out_master, PREPARE_PAYMENT_SIMPLE_SIZE);                                    \ 
        _06_08_ENCODE_DROPS_FEE            (fee_ptr, fee                            );                               \
    }

Steps to Reproduce

Leaving aside the lack of error checking, only buf_out_master points to a buffer of size PREPARE_PAYMENT_SIMPLE_SIZE; buf_out, initialized to buf_out_master and incremented by previous calls to _nn_nn_ENCODE*, is somewhere in the middle of that buffer.

Expected Result

The actual size is PREPARE_PAYMENT_SIMPLE_SIZE - ((uint32_t)buf_out - (uint32_t)buf_out_master) .

Actual Result

I doubt this is exploitable (users can call etxn_details with any arguments anyway), but it's confusing...

Environment

Supporting Files

vbar commented 1 year ago

Analogically in PREPARE_PAYMENT_SIMPLE_TRUSTLINE, which moreover uses incorrect PREPARE_PAYMENT_SIMPLE_TRUSTLINE_SIZE - w/o HAS_CALLBACK, it's defined as 287, which is 1 byte too low - so passing PREPARE_PAYMENT_SIMPLE_TRUSTLINE_SIZE - ((uint32_t)buf_out - (uint32_t)buf_out_master) will fail, but not because the computation is incorrect, but because the buffer actually is too small and the code using it (most prominently the peggy example) writes beyond it and works only by chance...

RichardAH commented 1 year ago

Looks like it was fixed :)