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

[Philippe Mathieu-Daudé]

On 1/2/20 10:28 AM, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Dec 30, 2019 at 5:00 PM P J P <ppandit at redhat.com> wrote:
>>
>> From: Prasad J Pandit <pjp at fedoraproject.org>
>>
>> tftp restricts relative or directory path access on Linux systems.
>> Apply same restrictions on Windows systems too. It helps to avoid
>> directory traversal issue.
>>
>> Reported-by: Peter Maydell <peter.maydell at linaro.org>
>> Signed-off-by: Prasad J Pandit <pjp at fedoraproject.org>
>> ---
>>   src/tftp.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/tftp.c b/src/tftp.c
>> index 093c2e0..55d9353 100644
>> --- a/src/tftp.c
>> +++ b/src/tftp.c
>> @@ -344,8 +344,9 @@ static void tftp_handle_rrq(Slirp *slirp, struct sockaddr_storage *srcsas,
>>       k += 6; /* skipping octet */
>>
>>       /* 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 ..

I'm confused here, req_fname[] is client-controlled, right?
I'm seeing it filled in tftp_input():
struct tftp_t *tp = (struct tftp_t *)m->m_data;
So the path separator used differs from the client, not the server... 
Does it makes sense to handle this differently regarding the server OS?

> You changed the existing rule, and it seems to break some valid cases
> ex: /foo/bar/../blah
> 
> This traversal problem is so common, I wonder if there are known good
> practices...
> 
>> --
>> 2.24.1
>>
>> _______________________________________________
>> Slirp mailing list
>> Slirp at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/slirp
> 
> 
>