Improve desktop shortcut creation:

Improve desktop shortcut creation:

- remove more comments (which generally only apply to the browser itself)
- add #!/usr/bin/env xdg-open shebang
- make the .desktop file placed on the desktop executable
- add more tests to make sure we're still secure

TEST=Covered by unit_tests.
BUG=22589
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27194

[agl, 2009-09-23 18:44:59]

LGTM in current form if you're happy there's a good reason for the sanitisation
as is.

http://codereview.chromium.org/232003/diff/1/2
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/232003/diff/1/2#newcode148
Line 148: S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
(unimportant) I actually find 0755 to be clearer

http://codereview.chromium.org/232003/diff/1/2#newcode154
Line 154: // a window of opportunity to put something other at the path).
This is only a concern if an attacker can control one of the directories between
the root and destination. Otherwise it's harmless.

Just for reference: one can open the parent directory, hold the fd and use
unlinkat to unlink safely in the case that an attacker controls the path.

http://codereview.chromium.org/232003/diff/1/2#newcode155
Line 155: HANDLE_EINTR(write(fd, contents.data(), contents.length()));
In practice, with short writes to the disk, this will always succeed.
Technically, however, it's possible that write can return less than
|contents.length()|. Possibly it's not worth the extra code if |contents| is
known to be small.

http://codereview.chromium.org/232003/diff/1/2#newcode271
Line 271: // Sanitize the command line string.
This is a little scary. Sanitize for what reason? Where's the command line going
to be used and what assumptions are going to be made?

[Paweł Hajdan Jr., 2009-09-23 18:50:40]

On 2009/09/23 18:44:59, agl wrote:
> http://codereview.chromium.org/232003/diff/1/2#newcode154
> Line 154: // a window of opportunity to put something other at the path).
> This is only a concern if an attacker can control one of the directories
between
> the root and destination. Otherwise it's harmless.
> 
> Just for reference: one can open the parent directory, hold the fd and use
> unlinkat to unlink safely in the case that an attacker controls the path.

Hmm... unlinkat still takes a const char* for the last path component. Are you
sure it's safe? I considered using it, but couldn't find any strong proofs that
it behaves as supposed.

> http://codereview.chromium.org/232003/diff/1/2#newcode155
> Line 155: HANDLE_EINTR(write(fd, contents.data(), contents.length()));
> In practice, with short writes to the disk, this will always succeed.
> Technically, however, it's possible that write can return less than
> |contents.length()|. Possibly it's not worth the extra code if |contents| is
> known to be small.

Ooops. I should fix it to be sure.

> http://codereview.chromium.org/232003/diff/1/2#newcode271
> Line 271: // Sanitize the command line string.
> This is a little scary. Sanitize for what reason? Where's the command line
going
> to be used and what assumptions are going to be made?

See the bug and lcamtuf's comment #3.

[Mike Mammarella, 2009-09-23 18:55:52]

http://codereview.chromium.org/232003/diff/1/2
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/232003/diff/1/2#newcode145
Line 145: // condition (the attacker can put something other at the path).
Is there any way to use fchmod() here? It took me a bit to understand this
comment's point: changing the permissions after writing the file using the
file's path is vulnerable to a race condition.

http://codereview.chromium.org/232003/diff/1/2#newcode154
Line 154: // a window of opportunity to put something other at the path).
This is also unfortunate given that you use O_EXCL above; a failure once will
prevent any later retries without user intervention.

[Mike Mammarella, 2009-09-23 19:00:57]

On 2009/09/23 18:50:40, Paweł Hajdan Jr. wrote:
> On 2009/09/23 18:44:59, agl wrote:
> > http://codereview.chromium.org/232003/diff/1/2#newcode154
> > Line 154: // a window of opportunity to put something other at the path).
> > This is only a concern if an attacker can control one of the directories
> between
> > the root and destination. Otherwise it's harmless.
> > 
> > Just for reference: one can open the parent directory, hold the fd and use
> > unlinkat to unlink safely in the case that an attacker controls the path.
> 
> Hmm... unlinkat still takes a const char* for the last path component. Are you
> sure it's safe? I considered using it, but couldn't find any strong proofs
that
> it behaves as supposed.

As long as the string you pass doesn't itself contain a /, then you'll be
unlinking the directory entry from the directory you think you are. Even if an
attacker puts something there, you're just undoing what the attacker did. (E.g.
attacker creates symlink to /bin/su, you delete symlink. Attacker creates
hardlink to /etc/passwd, you unlink again. Attacker creates new file, you delete
it.)

[Paweł Hajdan Jr., 2009-09-23 20:50:41]

Patch updated.

[Mike Mammarella, 2009-09-23 21:00:05]

LGTM

http://codereview.chromium.org/232003/diff/3003/3004
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/232003/diff/3003/3004#newcode163
Line 163: // Even if an attacker manager to put something other at
manages

[agl, 2009-09-23 21:02:49]

LGTM

http://codereview.chromium.org/232003/diff/3003/3004
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/232003/diff/3003/3004#newcode154
Line 154: return;
leaking |desktop_fd|

http://codereview.chromium.org/232003/diff/3003/3004#newcode156
Line 156: int bytes_written = file_util::WriteFileDescriptor(fd,
contents.data(),
s/int/ssize_t/

http://codereview.chromium.org/232003/diff/3003/3004#newcode160
Line 160: if (bytes_written != static_cast<int>(contents.length())) {
s/int/ssize_t/

[Paweł Hajdan Jr., 2009-09-23 21:34:52]

Patch updated.

lcamtuf, could you take a look?

[agl, 2009-09-23 21:39:35]

LGTM