On Tue, Nov 13, 2018 at 09:57:23PM +0100, Simon Goldschmidt wrote:
On 06.11.2018 15:51, Andrea Barisani wrote:
[..] The issue can be exploited by several means:
An excessively large crafted boot image file is parsed by the `tftp_handler` function which lacks any size checks, allowing the memory overwrite.
A malicious server can manipulate TFTP packet sequence numbers to store downloaded file chunks at arbitrary memory locations, given that the sequence number is directly used by the `tftp_handler` function to calculate the destination address for downloaded file chunks.
Additionally the `store_block` function, used to store downloaded file chunks in memory, when invoked by `tftp_handler` with a `tftp_cur_block` value of 0, triggers an unchecked integer underflow.
This allows to potentially erase memory located before the `loadAddr` when a packet is sent with a null, following at least one valid packet.
Do you happen to have more details on this suggested integer underflow? I have tried to reproduce it, but I failed to get a memory write address before 'load_addr'. This is because the 'store_block' function does not directly use the underflowed integer as a block counter, but adds 'tcp_block_wrap_offset' to this offset.
To me it seems like alternating between '0' and 'not 0' for the block counter could increase memory overwrites, but I fail to see how you can use this to store chunks at arbitrary memory locations. All you can do is subtract one block size from 'tftp_block_wrap_offset'...
Simon
Hello Simon,
the integer underflow can happen if a malicious TFTP server, able to control the TFTP packets sequence number, sends a crafted packet with sequence number set to 0 during a flow.
This happens because, within the store_block() function, the 'block' argument is declared as 'int' and when it is invoked inside tftp_handler() (case TFTP_DATA) this value is passed by doing 'tftp_cur_block - 1' (where tftp_cur_block is the sequence number extracted from the tftp packet without any previous check):
static inline void store_block(int block, uchar *src, unsigned len) ^^^^^^^^^ can have negative values (e.g. -1) { ulong offset = block * tftp_block_size + tftp_block_wrap_offset; ^^^^^ here if block is -1 the result stored onto offset would be a very large unsigned number, due to type conversions }
static void tftp_handler(...){
case TFTP_DATA: ... if (tftp_cur_block == tftp_prev_block) { /* Same block again; ignore it. */ break; }
tftp_prev_block = tftp_cur_block; timeout_count_max = tftp_timeout_count_max; net_set_timeout_handler(timeout_ms, tftp_timeout_handler);
store_block(tftp_cur_block - 1, pkt + 2, len); ^^^^^^^^^^^^^^^^^^ }
For these reasons the issue does not appear to be merely a "one block size" substraction, but rather offset can reach very large values. Unless I am missing something that I don't see of course...
You should probably prevent the underflow by placing a check against tftp_cur_block before the store_block() invocation, but I defer to you for a better implementation of the fix as you certainly know the overall logic much better.