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

[P J P]

  Hello all,

+-- On Thu, 2 Jan 2020, Philippe Mathieu-Daudé wrote --+
| 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...

  Differs from the client..?

also

  memcpy(spt->filename, slirp->tftp_prefix, prefix_len);                      
  spt->filename[prefix_len] = '/';
  req_fname = spt->filename + prefix_len + 1;

Separator added after tftp_prefix is '/', not '\\'.  And tftp_read_data() 
opens the file as

  spt->fd = open(spt->filename, O_RDONLY | O_BINARY);

  "tftp_prefix/my_directory\\my_library\\my_file.txt"

Likely open(2) would return an error for path like above?


+-- On Thu, 2 Jan 2020, Eric Blake wrote --+
| Maybe:
| if (
| #if G_OS_WIN32
|     strstr(req_fname, "..\\") ||
|     req_fname[strlen(req_fname) - 1] == '\\' ||
| #endif
|     strstr(req_fname, "../") ||
|     req_fname[strlen(req_fname) - 1] == '/')
| 
| although I'm not a fan of #if mid-expression.

  True, it'll be difficult to read/follow. I checked other (qga/commands.c) 
usage of G_OS_WIN32, it's following format.

    #ifdef G_OS_WIN32 ... #else ... #endif


Considering 'tftp_prefix' and 'req_fname' are separated by forward slash 
('/'), [how] does it support WIN32 tftp server?

@Samuel ...?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D