[U-Boot] CVE-2018-18439, CVE-2018-18440 - U-Boot verified boot bypass vulnerabilities

Andrea Barisani andrea.barisani at f-secure.com
Wed Nov 14 15:26:17 UTC 2018


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"


More information about the U-Boot mailing list