Re: BMC update via TFTP

[Joseph Reynolds <jrey@linux.ibm.com>]

On 12/3/19 1:55 PM, Adriana Kobylak wrote:
> On 2019-12-03 11:08, Gunnar Mills wrote:
>> On 12/3/2019 10:12 AM, Gunnar Mills wrote:
>>
>>>> In BMC WebUI under "Download image file from TFTP server" section,
>>>>
>>>> we have text fields "TFTP Server IP address" and "File name".
>>>> "File name" doesn't take folders in path. Is this a bug or
>>>> expected behavior?
>>>>
>>>> TFTP downloads work only if file is kept in root of tftp share.
>>> This is expected.
>>>
>> https://github.com/openbmc/phosphor-bmc-code-mgmt/blob/b0ce996ac60cf80487d71c3cdb7165d065079377/download_manager.cpp#L33 
>>
>>> [1]
>>
>> As long as we continue to sanitize the local filename, I don't see why
>> we need to limit the source file name.
>>  Patrick, Adriana, Any objection? Remember why we wrote it this way? I
>> assume for simplicity..?
>
> It was done for security to prevent users from specifying a file 
> outside the tftp directory, such as /mydir/../root/system-file.
> But seems the current file name handling breaks the ability to get 
> files from a subdir like Raj pointed out, we should be able to fix 
> that out, tftp supports having a file in subdirs such as 
> /tftpboot/subdirectory/file, and passing /subdirectory/file to it as 
> the path.

It seems to me that the burden must be on the TFTP server to protect 
itself.  See the "Security Considerations" section of RFC 1350 - 
https://tools.ietf.org/html/rfc1350
Specifically, OpenBMC only needs read access (it does not need to write 
files to the TFTP server) and only needs to be able to access files the 
TFTP server intends to provide (typically: all the files under a 
specific directory).

To be more clear:
- OpenBMC does not provide a TFTP server, so this is guidance for 
someone setting a TFTP server for use with an OpenBMC system
- The TFTP server only needs to offer READ access.
- The TFTP server ought to prevent clients from using 
cleverly-constructed pathnames (for example, any file which begins with 
"/" or contains an ".." element) to access files outside of its sandbox.
- That said, I am not a TFTP expert, and whichever TFTP server is used, 
its security guidance should be followed.

What all of the above, I think it would be okay for OpenBMC to validate 
the pathname it sends to the TFTP server, but don't think it is necessary.

Finally, I hope we can move away from TFTP and more toward a secure 
image distribution approach.
Note that TFTP is disabled in OpenBMC by default.  See 
https://github.com/openbmc/bmcweb/blob/master/CMakeLists.txt

- Joseph

>>
>>  Thanks,
>>  Gunnar
>>
>>
>> Links:
>> ------
>> [1]
>> https://github.com/openbmc/phosphor-bmc-code-mgmt/blob/b0ce996ac60cf80487d71c3cdb7165d065079377/download_manager.cpp#L33 
>>