Note that on 3 places is called "sh -c" so there also ';' is problematic and can lead to execution of another command. This problem is fixed in first part of attached patch. Second part fixes other problems.

Related patch https://bugzilla.gnome.org/show_bug.cgi?id=787202

Hi Pali,

Do you know a way to get an arbitrary device name into GParted so that
it might execute an unsafe command using that device name if not quoted?

Using the command line I get:
    # gpartedbin '/dev/sdb ; date > /tmp/log'
    Could not stat device /dev/sdb ; date > /tmp/log - No such file or directory.

Udev seems to make names safely:
    # btrfs filesystem label /dev/sdb31 '" ; date > /tmp/log'
    # ls -l /dev/disk/by-label
    ...
    lrwxrwxrwx. 1 root root 11 Sep 12 20:54 \x22\x20\x3b\x20date\x20\x3e\x20\x2ftmp\x2flog -> ../../sdb31

So far not found a way to get an unsafe path into GParted.

> Do you know a way to get an arbitrary device name into GParted so that
it might execute an unsafe command using that device name if not quoted?

Yes, here is one:

$ printf '#!/bin/sh\necho test > out\n' > script.sh; chmod +x script.sh

$ truncate -s 20M 'jfs;script.sh'
$ mkfs.jfs -q 'jfs;script.sh'

$ ls -l out
ls: cannot access out: No such file or directory

$ sudo PATH=$PWD:$PATH ./gpartedbin 'jfs;script.sh'
$ sudo PATH=$PWD:$PATH ./gpartedbin 'jfs;script.sh'

$ ls -l out
-rw-r--r-- 1 root root 5 Sep 12 23:11 out
$ cat out 
test

Looks like that file is created after second run of gpartedbin.

Basically filename is interpreted as shell fragment. You can replace script.sh by any other executable binary in PATH.

For the record the unsafe execution in comment 4 above works like this:

    jfs::set_used_sectors()
        partition.get_path() -> "jfs.script.sh"
        Utils::execute_command("sh -c 'echo dm | jfs_debugfs " +
                               partition.get_path() + "'", ...)
            command = "sh -c 'echo dm | jfs_debugfs jfs;script.sh'"
            Glib::shell_parse_argv(command) ->
                ["sh","-c","echo dm | jfs_debugfs jfs;script.sh"]
            Glib::spawn_async_with_pipes(...,
                Glib::shell_parse_argv(command), ...)

So FS image named 'jfs;script.sh' is passed to the shell where semicolon
separates the two commands, getting the following run via the shell:
    echo md | jfs_debugfs jfs
    script.sh


Pali,

Do you know of an attach that doesn't use one of the "sh -c" strings in
the code base?

Fix for this problem is in attached patch. It is needed to correctly escape all passed parameters and for such thing there is glib function.

Typo in the above question.  s/attach/attack/.

Do you know of an attack that doesn't use one of the "sh -c" strings in
the code base?

(In reply to Mike Fleetwood from comment #7)
> Do you know of an attack that doesn't use one of the "sh -c" strings in
> the code base?

I do not see any other yet, but I have not looked at it more deeply.

Current implementation which executes external commands is not correct as it does not quote ans escape parameters correctly which can lead in undefined behavior.

What I see is incorrect usage of those "sh -c" strings and missing escaping in labels strings which comes from user UI. Via label user can pass arbitrary parameter to *label executables (e.g. xfs_admin).

And I suspect this is a security related problem as in future any external commands can be extended to support other new options which can cause problems.

Input field for label should set label and not trying to parse also any other parameters for the external command. If user specify that want to have disk labeled '" --param "' then disk should be labeled '" --param "' and not --param parsed and passed as parameter to *label executable.

Created attachment 360196 [details] [review]
Correctly quote and escape arguments of external programs (v2)

Sorry it took so long, there was quite a bit to review and test and I
didn't have extra time for GParted.

Testing:
Debugged the string passed to execute_command() and what
Glib::shell_parse_argv() produced from it.  Then tested (almost)
everything.
* Query every type of recognised file system with and without blkid
  being available.  (without uses FS specific read UUID external
  commands too).
* Exercise every supported file system operation which involves an
  external command.
* Mount/activate/swapon and unmount/deactivate/swapoff.
* Refresh, create and delete partitions on a fake DMRaid array.
* etc.
All worked successfully.

I've made the following changes to patchset v1 from comment 0:
1) Updated the commit messages somewhat.
2) Added Glib::shell_quote() to missing case in
   Win_GParted::activate_mount_partition().
3) White space changes.

I'll commit in a day or so unless I hear otherwise.

Hi Mike and Pali,

Patch set v2 in comment #9 looks good to me and ready to commit.

I did some very basic testing and did not encounter any issues.

Thanks for your work in this area.

Curtis

The following changes from patchset v2 have been pushed upstream to the
public GParted git repo:

Correctly quote and escape command line argument passed to "sh -c" (#787203)
https://git.gnome.org/browse/gparted/commit/?id=f422052356ab594bac044dbd34fca894d2774625

Correctly quote and escape arguments passed to external commands (#787203)
https://git.gnome.org/browse/gparted/commit/?id=8fdc2c21a60cf8999b4328632b18e2bbbf13f9a4

*** Bug 649509 has been marked as a duplicate of this bug. ***

This enhancement was included in the GParted 0.30.0 release on October 10, 2017.