From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrea Barisani Date: Wed, 14 Nov 2018 16:26:17 +0100 Subject: [U-Boot] CVE-2018-18439, CVE-2018-18440 - U-Boot verified boot bypass vulnerabilities In-Reply-To: <26f1e5da-a71c-15b9-671f-2c6e4e5f0bc5@gmail.com> References: <20181106145150.GC10037@lambda.inversepath.com> <46b613b6-37c7-2f94-e8bc-ddc4b07cce60@gmail.com> <20181114115211.GI3458@lambda.inversepath.com> <20181114144538.GQ3458@lambda.inversepath.com> <26f1e5da-a71c-15b9-671f-2c6e4e5f0bc5@gmail.com> Message-ID: <20181114152617.GS3458@lambda.inversepath.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > -- Andrea Barisani Head of Hardware Security | F-Secure Founder | Inverse Path https://www.f-secure.com https://inversepath.com 0x864C9B9E 0A76 074A 02CD E989 CE7F AC3F DA47 578E 864C 9B9E "Pluralitas non est ponenda sine necessitate"