On Wed, Nov 14, 2018 at 04:13:00PM +0100, Simon Goldschmidt wrote:
On 14.11.2018 15:45, Andrea Barisani wrote:
On Wed, Nov 14, 2018 at 01:03:12PM +0100, Simon Goldschmidt wrote:
On 14.11.2018 12:52, Andrea Barisani wrote:
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
And this is exatclty my point. This might be bad coding style, but for me it works: 'block' is an 'int' and is '-1', so 'block * tftp_block_size' is '-512'. Now from the code flow in tftp_handler(), it's clear that if we come here with tftp_cur_block == 0 (so 'block' is -1), 'tftp_block_wrap_offset' is not 0 but some positive value 'x * tftp_block_size' (see function 'update_block_number').
So the resulting 'offset' is '-512 + (x * 512)' where 'x > 0'. I still fail to see how this can be a very large positive number resulting in an effective negative offset or arbitrary write.
I understand your point, however what does happen when we enter the 'case TFTP_DATA' and we are in the first block received, so we trigger new_transfer() that sets the tftp_block_wrap_offset to 0 *and* tftp_mcast_active is set?
I don't see any protection for this case for the underflow, am I wrong?
static void new_transfer(void) { tftp_prev_block = 0; tftp_block_wrap = 0; tftp_block_wrap_offset = 0; #ifdef CONFIG_CMD_TFTPPUT tftp_put_final_block_sent = 0; #endif }
... case TFTP_DATA:
if (tftp_state == STATE_SEND_RRQ || tftp_state == STATE_OACK || tftp_state == STATE_RECV_WRQ) { /* first block received */ tftp_state = STATE_DATA; tftp_remote_port = src; new_transfer(); ^^^^^^^^^^^^^^^See some lines below...
#ifdef CONFIG_MCAST_TFTP if (tftp_mcast_active) { /* start!=1 common if mcast */ <<<< HERE tftp_prev_block = tftp_cur_block - 1; } else #endif if (tftp_cur_block != 1) { /* Assertion */
If tftp_cur_block is 0 for the first block, we stop right away. No chance to reach store_block() at that time.
CC'ing my colleague Daniele whom can better reply further on this.
puts("\nTFTP error: "); printf("First block is not block 1 (%ld)\n", tftp_cur_block); puts("Starting again\n\n"); net_start_again(); break; } } 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); ^^^^^^^^^^^^^^^^^^This should result in having -1 and thus -512 as result of the 'offset' math that converted to ulong would result in a very large value.
}
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...
So I take it this "bug" report is from reading the code only, not from actually testing it and seeing the arbitrary memory write? I wouldn't have expected this in a CVE report...
As you see from our report the core issues have been fully tested and reproduced.
Yes. Thanks for that. I'm working on fixing them :-)
And that's much appreciated :)
It is true however that the additional remark on the `store_block' function has only been evaluated by code analysis, in the context of the advisory it seemed something worth notice in relation to the code structure but again, as you say we didn't practically test that specific aspect, while everything else was tested and reproduced.
The vulnerability report highlights two (in our opinion) critical vulnerabilities, one of which described a secondary aspect only checked by means of source code analysis.
In my opinion as well these are critical, yes.
The secondary aspect that we are discussing does not change the overall impact of the TFTP bugs, which remains unchanged as arbitrary code execution can anyway be achieved.
Of course. I'm working on fixing the actual bug and while debugging it tried to fix the other thing you mentioned. I could not reproduce it in a test setup (where I can freely send tftp packets). That's why I asked. The other bugs are of course not affected by this one not being valid.
Understood.
Cheers
Thanks for confirming this.
Simon
Thanks!
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.
Don't get me wrong: I'm just yet another user of U-Boot and I don't know the code better than you do. In fact, I looked at the tftp code for the first time yesterday after reading you report on the tftp issue in detail.
Simon