[Spice-devel] [vdagent-win PATCH v2] Avoid to use names with reserved characters.

[Victor Toso]

Hi,

On Mon, Sep 05, 2016 at 09:52:59AM -0400, Frediano Ziglio wrote:
> >
> > Hi,
> >
> > On Mon, Sep 05, 2016 at 12:36:26PM +0100, Frediano Ziglio wrote:
> > > Some characters are reserved and should not be used in Windows
> > > independently by the file system used.
> > > This avoid to use paths in the filename which could lead to some
> > > nasty hacks (like names like "..\hack.txt").
> >
> > True :)
> >
> > Although, I wonder if we should not *change* the filename and proceed
> > with the file-transfer?
> >
>
> What do you mean here? Kind of change "..\hack.txt" to "hack.txt" ?

Well, yes?

>From security point of view (nasty hack!) the concern should be
*accessing* the VM. If malicious user has access to the VM, I'm not sure
that drag & drop should always be worried about that.

Now, I believe that we have different restriction between filesystems
and/or guests.. so, if we realize that in the vdagent we might want to
change the filename and proceed with the file-transfer.

I not sure what to do but I thought it would be good to mention it here
:)

> > >
> > > ":" is used to separate filenames from stream names and can be used
> > > to create hidden streams. Also is used for drive separator (A:)
> > > or device names (NUL:).
> > > "/" and "\" are reserved for components (directory, filename, drive,
> > > share, server) separators.
> > > "*" and "?" are wildcards (which on Windows are supported by
> > > different APIs too).
> > > "<", ">" and "|" are reserved for shell usage.
> > >
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  vdagent/file_xfer.cpp | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> > > index 0e90ebe..2072277 100644
> > > --- a/vdagent/file_xfer.cpp
> > > +++ b/vdagent/file_xfer.cpp
> > > @@ -65,6 +65,10 @@ void FileXfer::handle_start(VDAgentFileXferStartMessage*
> > > start,
> > >          return;
> > >      }
> > >      vd_printf("%u %s (%" PRIu64 ")", start->id, file_name, file_size);
> > > +    if (strcspn(file_name, "<>:\"/\\|?*") != strlen(file_name)) {
> > > +        vd_printf("filename contains invalid characters");
> > > +        return;
> > > +    }
> >
> > I would add in the commit log that this endup returning error to the
> > user with VD_AGENT_FILE_XFER_STATUS_ERROR as status (just to be clear as
> > we can't see that in the patch itself)
> >
> > Cheers,
> >   toso
> >
>
> Done, I added:
>
> "The return statement cause the file transfer to be aborted with
> VD_AGENT_FILE_XFER_STATUS_ERROR as status."

Thanks,
  toso

>
> > >      if (!as_user.begin()) {
> > >          vd_printf("as_user failed");
> > >          return;
>
> Frediano