Skip to contentSkip to conversation

obexd broken for absolute paths

DISCUSSION Bastien Nocera 2 participants 6 messages Nov 8, 2013 – Nov 10, 2013 Bluetooth drivers

lore
Share & tools
lore
#0 lore raw

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

#1 lore raw
Quoted 12 lines · 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:

(quoted diff) +1 -1
-               return properties['Name']
+               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))
Quoted 1 line

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

Quoted 11 lines

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);
                }
Quoted 3 lines

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

#2 lore raw

Hi Bastian,

Quoted 13 lines · Bastien Nocera
Quoted 12 lines · 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:

(quoted diff) +1 -1
-               return properties['Name']
+               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))
Quoted 5 lines
Quoted 1 line

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.

Quoted 1 line

- 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.

Signature · 1 line
Luiz Augusto von Dentz
#3 lore raw
Quoted 16 lines · Luiz Augusto von Dentz

Hi Bastian,

Quoted 13 lines · Bastien Nocera
Quoted 12 lines · 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:

(quoted diff) +1 -1
-               return properties['Name']
+               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))
Quoted 8 lines
Quoted 5 lines
Quoted 1 line

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.

Quoted 5 lines
Quoted 1 line

- 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

#4 lore raw

Hi Bastian,

Quoted 23 lines · Bastien Nocera
Quoted 22 lines · Luiz Augusto von Dentz

Hi Bastian,

Quoted 18 lines · Bastien Nocera
Quoted 12 lines · 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:

(quoted diff) +1 -1
-               return properties['Name']
+               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))
Quoted 1 line

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

Quoted 7 lines
Quoted 2 lines

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.

Quoted 11 lines
Quoted 5 lines
Quoted 1 line

- 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.

Signature · 1 line
Luiz Augusto von Dentz
#5 lore raw
Quoted 29 lines · Luiz Augusto von Dentz

Hi Bastian,

Quoted 26 lines · Bastien Nocera
Quoted 24 lines · Luiz Augusto von Dentz

Hi Bastian,

Quoted 13 lines · Bastien Nocera
Quoted 12 lines · 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:

(quoted diff) +1 -1
-               return properties['Name']
+               return ("%s/%s" % ("/home/hadess/Downloads/", properties['Name']))
Quoted 5 lines
Quoted 1 line

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.

Quoted 8 lines
Quoted 4 lines

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.

Quoted 12 lines
Quoted 11 lines
Quoted 5 lines
Quoted 1 line

- 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).

Quoted 4 lines

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.