Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

comics: Remove support for tar and tar-like commands #259

Closed
wants to merge 2 commits into from

Conversation

raveit65
Copy link
Member

When handling tar files, or using a command with tar-compatible syntax,
to open comic-book archives, both the archive name (the name of the
comics file) and the filename (the name of a page within the archive)
are quoted to not be interpreted by the shell.

But the filename is completely with the attacker's control and can start
with "--" which leads to tar interpreting it as a command line flag.

This can be exploited by creating a CBT file (a tar archive with the
.cbt suffix) with an embedded file named something like this:
"--checkpoint-action=exec=bash -c 'touch ~/hacked;'.jpg"

CBT files are infinitely rare (CBZ is usually used for DRM-free
commercial releases, CBR for those from more dubious provenance), so
removing support is the easiest way to avoid the bug triggering. All
this code was rewritten in the development release for GNOME 3.26 to not
shell out to any command, closing off this particular attack vector.

This also removes the ability to use libarchive's bsdtar-compatible
binary for CBZ (ZIP), CB7 (7zip), and CBR (RAR) formats. The first two
are already supported by unzip and 7zip respectively. libarchive's RAR
support is limited, so unrar is a requirement anyway.

Discovered by Felix Wilhelm from the Google Security Team.

https://bugzilla.gnome.org/show_bug.cgi?id=784630

fixes #257

When handling tar files, or using a command with tar-compatible syntax,
to open comic-book archives, both the archive name (the name of the
comics file) and the filename (the name of a page within the archive)
are quoted to not be interpreted by the shell.

But the filename is completely with the attacker's control and can start
with "--" which leads to tar interpreting it as a command line flag.

This can be exploited by creating a CBT file (a tar archive with the
.cbt suffix) with an embedded file named something like this:
"--checkpoint-action=exec=bash -c 'touch ~/hacked;'.jpg"

CBT files are infinitely rare (CBZ is usually used for DRM-free
commercial releases, CBR for those from more dubious provenance), so
removing support is the easiest way to avoid the bug triggering. All
this code was rewritten in the development release for GNOME 3.26 to not
shell out to any command, closing off this particular attack vector.

This also removes the ability to use libarchive's bsdtar-compatible
binary for CBZ (ZIP), CB7 (7zip), and CBR (RAR) formats. The first two
are already supported by unzip and 7zip respectively. libarchive's RAR
support is limited, so unrar is a requirement anyway.

Discovered by Felix Wilhelm from the Google Security Team.

https://bugzilla.gnome.org/show_bug.cgi?id=784630

fixes #257
@raveit65
Copy link
Member Author

@sc0w @sunweaver
feel free to test this.

@sc0w
Copy link
Member

sc0w commented Jul 15, 2017

@raveit65

by the moment I don't have test this PR, but please, make sure the commit 77fc877 is working after this, I see you deleted "lsar" line

@raveit65
Copy link
Member Author

@sc0w
I don't think, it's there at line 450.

@sc0w
Copy link
Member

sc0w commented Jul 17, 2017

I have tested now, atril and unarchiver (unar/lsar) can't open .cbt files with the PR

@sc0w
Copy link
Member

sc0w commented Jul 17, 2017

evince doesn't support unarchiver, but atril yes, and we need a little changes

unarchiver works with .cbt files, we need that lines, and keep the configure.ac with no changes

I suggest something like this: sc0w@7c98975

Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what to do with this. I don't have a any .cbt files to test this with, but .cbr files did not open either.

We could push this as an emergency security update and bring back support later with GNOME's fix, or take the time to find a fix directly if this is not considered a likely mode of attack.

People using atril to read .cbr files might not be happy with an update that forces them back to Evince to read their files, but a linux machine somewhere getting botted because MATE was slow with a security update would be far worse.

@lukefromdc
Copy link
Member

Anyway, I'm keeping this installed as I am considered by certain opponents to be a high-value target and its a way into a Linux box. I don't want to forget about this allow attackers into my machines

@raveit65
Copy link
Member Author

@sc0w
Can you please test again?
You should be able to open a .cbt file with atril/unarchiver only.
But, i am not really shure if that fixes the secure issue.
@monsta @flexiondotorg @XRevan86 @lukefromdc
Thoughts?

@raveit65
Copy link
Member Author

@sc0w
offtopic on
Do you use a pluma plugin to remove unwanted garbage at end of the lines?
See you link to your repo.
I noticed that in other PRs too.
Such cleaning should be done in a single commit, imo.
offtopic off

@monsta
Copy link
Contributor

monsta commented Jul 17, 2017

I don't see any links to a sample .cbt with the exploit - did I miss something?

@raveit65
Copy link
Member Author

@monsta
no,
i only backport gnome commit.
Sorry, i don't have any .cbt with or without exploit.

@lukefromdc
Copy link
Member

Latest change does not give ability to open .cbr files, don't have any .cbt files

@monsta
Copy link
Contributor

monsta commented Jul 17, 2017

We need a test file for this because of re-added unarchiver support - we need to know if the security issue is still fixed after that.

@hadess: maybe you have some ideas?

@lukefromdc
Copy link
Member

The test file itself needs to be a known safe file because of the exploit

@hadess
Copy link

hadess commented Jul 17, 2017

I already answered in the issue I filed about this issue. I can't help more. I'm not going to post a reproducer on a public forum.

@lukefromdc
Copy link
Member

We need a known safe file, not a known attack file with a dummy payload. The latter would require testing in a quarantined environment, probably by installing over a live USB stick install of Ubuntu MATE and testing off-network,.

@sc0w
Copy link
Member

sc0w commented Jul 18, 2017

[offtopic on]

@raveit65

Do you use a pluma plugin to remove unwanted garbage at end of the lines?

I noticed that now with your comment, I was not sure what happened here xD

you remember mate-desktop/pluma#237 ?

I activated the plugin "save without trailing spaces" for testing and I forgot disable it

Now, it is disabled :)

[offtopic off]

@raveit65 with your latest changes the PR is ok for me, cbt works again, but, I can't reproduce the issue

we need a killer file, we don't know if unarchiver is vulnerable too

@raveit65 @monsta @lukefromdc

anyway, honestly, we must consider the issue exists if we can't reproduce it?

@sc0w
Copy link
Member

sc0w commented Jul 18, 2017

sample.zip

I can't reproduce the issue with this file (I compressed the .cbt into the .zip file)

It contains the file: "--checkpoint-action=exec=bash -c 'touch 123;'.jpg"

and atril doesn't create the file 123

I have tested with evince 3.22.1 and same result, I can't reproduce the issue

@lukefromdc
Copy link
Member

I just tested this same file with atril from master, in order to get file 123 created I had to extract the png image with the filename and bogus .jpg extension, remove the extension, and execute that code in terminal directly

@andreygursky
Copy link
Contributor

@sc0w, I've used your image to craft a tar sample.cbt (sample.zip) with an absolute path to a touched file:

$ tar tf sample.cbt
--checkpoint-action=exec=bash -c 'touch /tmp/covfefe.evince;'.jpg

Still cannot reproduce the original issue.

@lukefromdc
Copy link
Member

Same here, didn't get the file in /tmp.

@andreygursky
Copy link
Contributor

OK, using an image with a greater size does indeed work (sample.zip).

@lukefromdc
Copy link
Member

Confirmed: both Atril and Engrampa are vulnerable, file-roller has .cbt support disabled. Got the file in /tmp when opening the sample.cbt file with either Atril or Engrampa, and file-roller refused to open it as an unsupported archive type.

To safely open a real attack file of this nature requires changing the extension to .tar and then using engrampa or file-roller to open it.

@andreygursky
Copy link
Contributor

BTW, in case you're testing not in the safe box, please don't forget to test such "bad" files prior to run atril/... on it with tar tf sample.cbt to check that the shell command being called is really harmless.

@lukefromdc
Copy link
Member

Attempting to pre-check these files with engrampa is NOT safe, as the code executes same as it would in Atril. This in turn implies future potential for real attack files aimed at nailing users not when they try to use them, but rather when they try to see what is inside to determine whether they are safe.
Thus we have a lesson in handling suspicious archives in general.

The sample files could be shown to be harmless only after the fact if graphical tools were used. Far worse would be a file that has malicious behavior that cannot be detected by the user even after the fact. In this case, the exploit code is readable on opening with Engrampa but has already run. In the case of Atril a user would not see the code at all and could get a silent surprise,in the real world something more than an image of an empty Evince window named cofefe in /tmp.

@lukefromdc
Copy link
Member

thought running tar -tf also ran the code, but I was just looking at the already existing file from the graphical test so no big deal there. Tar is OK, Engrampa and Atril are not for this issue.

@sc0w
Copy link
Member

sc0w commented Jul 19, 2017

we don't need to remove the tar support

I have an alternative fix: sc0w@f57bb4d

#261

@lukefromdc
Copy link
Member

Update: in today's testing with sc0w's file (same file it seems), Engrampa did not produce the cofefe file in /tmp. Maybe seeing one left over from Atril instead.

@raveit65 raveit65 closed this Jul 21, 2017
@raveit65 raveit65 deleted the dev-comics branch July 21, 2017 18:05
cgrzemba pushed a commit to cgrzemba/oi-userland that referenced this pull request Sep 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command injection vulnerability in CBT handler
6 participants