On 14.11.2018 16:35, Daniele Bianco wrote:
On Wed, Nov 14, 2018 at 04:26:17PM +0100, Andrea Barisani wrote:
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.
Hi Simon,
the 'if (tftp_cur_block != 1)' is not triggered if 'tftp_mcast_active'
is set (and the CONFIG_MCAST_TFTP is defined).
Please note the code indentation does not help in this case as it is
misleading, but this is because of the #ifdef.
Ah, now I do see it, thanks for the hint! Indeed, the indentation of
that else totally hid it from my eyes that the next block wasn't
executed always!
Luckily, searching through the whole mainline codebase shows no users of
this option (CONFIG_MCAST_TFTP), so I guess this is not a real world
problem, currently :-)
Thanks for your explanation and your fast response!
Cheers,
Simon
Cheers,
Daniele
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"
--
Daniele Bianco
Hardware Security | F-Secure
<daniele.bia...@f-secure.com> | https://www.f-secure.com
GPG Key fingerprint = 88A7 43F4 F28F 1B9D 6F2D 4AC5 AE75 822E 9544 A497
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
