[Slirp] [PATCH] slirp: tftp: restrict relative path access

[Eric Blake]

On 1/2/20 3:28 AM, Marc-André Lureau wrote:

>>       /* do sanity checks on the filename */
>> -    if (!strncmp(req_fname, "../", 3) ||
>> -        req_fname[strlen(req_fname) - 1] == '/' || strstr(req_fname, "/../")) {
>> +    if (strstr(req_fname, "../") || strstr(req_fname, "..\\")
>> +        || req_fname[strlen(req_fname) - 1] == '/'
>> +        || req_fname[strlen(req_fname) - 1] == '\\') {
>>           tftp_send_error(spt, 2, "Access violation", tp);
>>           return;
>>       }
> 
> Please divide the rule for #ifdef G_OS_WIN32 .. #else ..
> 
> You changed the existing rule, and it seems to break some valid cases
> ex: /foo/bar/../blah

Hmm. The existing code rejects "/foo/../" but accepts "/foo/..".  I 
don't know why it is special-casing on whether the last character of the 
string is "/".

What's more, it is not obvious whether "/foo/bar/../blah" is an escape 
attempt if /foo/bar can be a symlink under user control.  Windows lacks 
symlinks, so it is easier to tell: as long as there are fewer ../ 
components than there are earlier non-.. components, there is no escape. 
  But on Linux, where earlier components can be symlinks, you can't 
blindly shorten "/foo/bar/../blah" into "/foo/blah" because bar might be 
a symlink.

Another problem with the proposed code of using just strstr("../" 
instead of "/../" is that it has false positives on the non-escape 
"/foo.../bar" (not that a suffix of 3 dots is common, but it shouldn't 
be a reason for us to incorrectly reject it).

> This traversal problem is so common, I wonder if there are known good
> practices...

Sadly, path sanitization is tough, and I don't know of any good turnkey 
solutions out there :(

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org