obexd broken for absolute paths

obexd broken for absolute paths

[Bastien Nocera]

Heya,

I was trying to test gnome-user-share's Bluetooth support for BlueZ 5,
and was quite surprised it didn't work one bit, with transfers failing
as soon as they were created.

I made this simple change to test/simple-obex-agent so you could
replicate the failure. Obviously, change the download path to exist on
your system:
-               return properties['Name']
+               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))

This will see OBEX Push transfers fail as soon as accepted.

I must also mention the dreadful code in agent_reply() in obexd/src/manager.c:
                const char *slash = strrchr(name, '/');
                DBG("Agent replied with %s", name);
                if (!slash) {
                        agent->new_name = g_strdup(name);
                        agent->new_folder = NULL;
                } else {
                        agent->new_name = g_strdup(slash + 1); 
                        agent->new_folder = g_strndup(name, slash - name);
                }

Please use g_path_get_basename() and g_path_get_dirname(). This is even
a security issue because I could pass relative paths and expect some
system file to get overwritten.

Cheers

Re: obexd broken for absolute paths

[Bastien Nocera]

On Fri, 2013-11-08 at 20:21 +0100, Bastien Nocera wrote:
> Heya,
> 
> I was trying to test gnome-user-share's Bluetooth support for BlueZ 5,
> and was quite surprised it didn't work one bit, with transfers failing
> as soon as they were created.
> 
> I made this simple change to test/simple-obex-agent so you could
> replicate the failure. Obviously, change the download path to exist on
> your system:
> -               return properties['Name']
> +               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))
> 
> This will see OBEX Push transfers fail as soon as accepted.

Turns out this is a feature of filesystem plugin in obexd, and a bit of
a problem as well:
- There's no way to change the folder without changing the service file
- It doesn't default to use the XDG_RUNTIME_DIR 

> I must also mention the dreadful code in agent_reply() in obexd/src/manager.c:
>                 const char *slash = strrchr(name, '/');
>                 DBG("Agent replied with %s", name);
>                 if (!slash) {
>                         agent->new_name = g_strdup(name);
>                         agent->new_folder = NULL;
>                 } else {
>                         agent->new_name = g_strdup(slash + 1); 
>                         agent->new_folder = g_strndup(name, slash - name);
>                 }
> 
> Please use g_path_get_basename() and g_path_get_dirname(). This is even
> a security issue because I could pass relative paths and expect some
> system file to get overwritten.

I've sent a patch for this.

Cheers

Re: obexd broken for absolute paths

[Luiz Augusto von Dentz]

Hi Bastian,

On Sat, Nov 9, 2013 at 7:17 PM, Bastien Nocera <hadess@hadess.net> wrote:
> On Fri, 2013-11-08 at 20:21 +0100, Bastien Nocera wrote:
>> Heya,
>>
>> I was trying to test gnome-user-share's Bluetooth support for BlueZ 5,
>> and was quite surprised it didn't work one bit, with transfers failing
>> as soon as they were created.
>>
>> I made this simple change to test/simple-obex-agent so you could
>> replicate the failure. Obviously, change the download path to exist on
>> your system:
>> -               return properties['Name']
>> +               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))
>>
>> This will see OBEX Push transfers fail as soon as accepted.
>
> Turns out this is a feature of filesystem plugin in obexd, and a bit of
> a problem as well:
> - There's no way to change the folder without changing the service file

Yep, I remember discussing with Gustavo Padovan that this should
probably be set by the agent upon registration.

> - It doesn't default to use the XDG_RUNTIME_DIR

That is a good default considering we don't implement the change
above, otherwise for auto accept I believe tmp is usually a better
option.


-- 
Luiz Augusto von Dentz

Re: obexd broken for absolute paths

[Bastien Nocera]

On Sat, 2013-11-09 at 20:35 +0200, Luiz Augusto von Dentz wrote:
> Hi Bastian,
> 
> On Sat, Nov 9, 2013 at 7:17 PM, Bastien Nocera <hadess@hadess.net> wrote:
> > On Fri, 2013-11-08 at 20:21 +0100, Bastien Nocera wrote:
> >> Heya,
> >>
> >> I was trying to test gnome-user-share's Bluetooth support for BlueZ 5,
> >> and was quite surprised it didn't work one bit, with transfers failing
> >> as soon as they were created.
> >>
> >> I made this simple change to test/simple-obex-agent so you could
> >> replicate the failure. Obviously, change the download path to exist on
> >> your system:
> >> -               return properties['Name']
> >> +               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))
> >>
> >> This will see OBEX Push transfers fail as soon as accepted.
> >
> > Turns out this is a feature of filesystem plugin in obexd, and a bit of
> > a problem as well:
> > - There's no way to change the folder without changing the service file
> 
> Yep, I remember discussing with Gustavo Padovan that this should
> probably be set by the agent upon registration.

I thought about that, but it's really a security issue. obexd might be
running in a different context than the "application" telling it where
to write. For example, my share application might be restricted to write
new files in ~/Downloads, but could tell obexd to write to ~/.ssh/etc.

> > - It doesn't default to use the XDG_RUNTIME_DIR
> 
> That is a good default considering we don't implement the change
> above, otherwise for auto accept I believe tmp is usually a better
> option.

This is what I intend to use in gnome-user-share. obexd would write
files with unique filenames to /run/user/<id>/obexd and move it
~/Downloads after uniquifying the name (eg. sending 2 files called
"foo.jpg" should give me 2 files, not overwrite the first one as it does
now).

Cheers

Re: obexd broken for absolute paths

[Luiz Augusto von Dentz]

Hi Bastian,

On Sat, Nov 9, 2013 at 11:00 PM, Bastien Nocera <hadess@hadess.net> wrote:
> On Sat, 2013-11-09 at 20:35 +0200, Luiz Augusto von Dentz wrote:
>> Hi Bastian,
>>
>> On Sat, Nov 9, 2013 at 7:17 PM, Bastien Nocera <hadess@hadess.net> wrote:
>> > On Fri, 2013-11-08 at 20:21 +0100, Bastien Nocera wrote:
>> >> Heya,
>> >>
>> >> I was trying to test gnome-user-share's Bluetooth support for BlueZ 5,
>> >> and was quite surprised it didn't work one bit, with transfers failing
>> >> as soon as they were created.
>> >>
>> >> I made this simple change to test/simple-obex-agent so you could
>> >> replicate the failure. Obviously, change the download path to exist on
>> >> your system:
>> >> -               return properties['Name']
>> >> +               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))
>> >>
>> >> This will see OBEX Push transfers fail as soon as accepted.
>> >
>> > Turns out this is a feature of filesystem plugin in obexd, and a bit of
>> > a problem as well:
>> > - There's no way to change the folder without changing the service file
>>
>> Yep, I remember discussing with Gustavo Padovan that this should
>> probably be set by the agent upon registration.
>
> I thought about that, but it's really a security issue. obexd might be
> running in a different context than the "application" telling it where
> to write. For example, my share application might be restricted to write
> new files in ~/Downloads, but could tell obexd to write to ~/.ssh/etc.

If obexd is running with a different user then yes, but I don't think
this is the case otherwise we should pass fd not a path since the
files should really belong to the agent not obexd.

>> > - It doesn't default to use the XDG_RUNTIME_DIR
>>
>> That is a good default considering we don't implement the change
>> above, otherwise for auto accept I believe tmp is usually a better
>> option.
>
> This is what I intend to use in gnome-user-share. obexd would write
> files with unique filenames to /run/user/<id>/obexd and move it
> ~/Downloads after uniquifying the name (eg. sending 2 files called
> "foo.jpg" should give me 2 files, not overwrite the first one as it does
> now).

Not sure what is the point of doing this, except if you want to use
tmpfs while downloading and only really store on disk when the
transfer is complete, overwrite is agent problem to select different
path if the file already exist or alert the user it will overwrite.


-- 
Luiz Augusto von Dentz

Re: obexd broken for absolute paths

[Bastien Nocera]

On Sun, 2013-11-10 at 02:49 +0200, Luiz Augusto von Dentz wrote:
> Hi Bastian,
> 
> On Sat, Nov 9, 2013 at 11:00 PM, Bastien Nocera <hadess@hadess.net> wrote:
> > On Sat, 2013-11-09 at 20:35 +0200, Luiz Augusto von Dentz wrote:
> >> Hi Bastian,
> >>
> >> On Sat, Nov 9, 2013 at 7:17 PM, Bastien Nocera <hadess@hadess.net> wrote:
> >> > On Fri, 2013-11-08 at 20:21 +0100, Bastien Nocera wrote:
> >> >> Heya,
> >> >>
> >> >> I was trying to test gnome-user-share's Bluetooth support for BlueZ 5,
> >> >> and was quite surprised it didn't work one bit, with transfers failing
> >> >> as soon as they were created.
> >> >>
> >> >> I made this simple change to test/simple-obex-agent so you could
> >> >> replicate the failure. Obviously, change the download path to exist on
> >> >> your system:
> >> >> -               return properties['Name']
> >> >> +               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))
> >> >>
> >> >> This will see OBEX Push transfers fail as soon as accepted.
> >> >
> >> > Turns out this is a feature of filesystem plugin in obexd, and a bit of
> >> > a problem as well:
> >> > - There's no way to change the folder without changing the service file
> >>
> >> Yep, I remember discussing with Gustavo Padovan that this should
> >> probably be set by the agent upon registration.
> >
> > I thought about that, but it's really a security issue. obexd might be
> > running in a different context than the "application" telling it where
> > to write. For example, my share application might be restricted to write
> > new files in ~/Downloads, but could tell obexd to write to ~/.ssh/etc.
> 
> If obexd is running with a different user then yes, but I don't think
> this is the case otherwise we should pass fd not a path since the
> files should really belong to the agent not obexd.

Not a different user, a different security context, which is going to
happen when applications are sandboxed within the session.

> >> > - It doesn't default to use the XDG_RUNTIME_DIR
> >>
> >> That is a good default considering we don't implement the change
> >> above, otherwise for auto accept I believe tmp is usually a better
> >> option.
> >
> > This is what I intend to use in gnome-user-share. obexd would write
> > files with unique filenames to /run/user/<id>/obexd and move it
> > ~/Downloads after uniquifying the name (eg. sending 2 files called
> > "foo.jpg" should give me 2 files, not overwrite the first one as it does
> > now).
> 
> Not sure what is the point of doing this, except if you want to use
> tmpfs while downloading and only really store on disk when the
> transfer is complete, overwrite is agent problem to select different
> path if the file already exist or alert the user it will overwrite.

There's no way of doing this in a race-free way. The right way to do
this would be for the agent to return a file descriptor, not a path.

A file with that path might be created between the time the filename is
considered "free" by the agent and obexd actually writing it. It might
even be racy when receiving multiple files with the same name.