From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Goldschmidt Date: Wed, 14 Nov 2018 13:03:12 +0100 Subject: [U-Boot] CVE-2018-18439, CVE-2018-18440 - U-Boot verified boot bypass vulnerabilities In-Reply-To: <20181114115211.GI3458@lambda.inversepath.com> References: <20181106145150.GC10037@lambda.inversepath.com> <46b613b6-37c7-2f94-e8bc-ddc4b07cce60@gmail.com> <20181114115211.GI3458@lambda.inversepath.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. > } > > 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... > 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