Bug 100224 - [PATCH] Seccomp sandbox support for pdftotext
Summary: [PATCH] Seccomp sandbox support for pdftotext
Status: RESOLVED MOVED
Alias: None
Product: poppler
Classification: Unclassified
Component: utils (show other bugs)
Version: unspecified
Hardware: Other Linux (All)
: medium enhancement
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-16 05:51 UTC by valo
Modified: 2018-08-21 10:40 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
seccomp support for pdftotext (12.38 KB, patch)
2017-03-16 05:51 UTC, valo
Details | Splinter Review
improved libsec.cc (5.04 KB, text/x-c++src)
2017-03-16 17:53 UTC, valo
Details

Description valo 2017-03-16 05:51:22 UTC
Created attachment 130253 [details] [review]
seccomp support for pdftotext

Since some of the poopler tools, like pdftotext are used by some file managers to automatically parse pdf files for preview, I thought it might be a good idea to use some sandboxing.

This is a patch that adds seccomp filter to pdftotext. This can also be applied to the other tools that poppler provides, reducing the risk of successful exploitation of poppler (and other used library) vulnerabilities significantly. 

I found this quite easy to apply and would be happy to help if you are interested in using this.

This patch can be applied to poppler 0.52.0 without further changes
Comment 1 Jason Crain 2017-03-16 15:04:13 UTC
I have a few comments, though you'll also have to wait for Albert to comment on whether he wants to add seccomp support.

1. Instead of just adding the library to Makefile.am, you should conditionally depend on the library through checks in both configure.ac and CMakeLists.txt.  The way you have it now is likely to break compilation on anything other than Linux.  And poppler has two parallel build systems, cmake and autotools, so if something is changed, it needs to be changed in both.

2. Patches should not include generated files like Makefile.in.  Using 'git format-patch' on the git repo to generate a patch works better.

3. At least 'stat', 'getdents', 'sysinfo', and 'mremap' need to be added to the list.  I haven't tested very thoroughly so they may be more.

I also wonder how maintainable this is.  It seems to me that the exact syscalls used are going to vary between systems and depend on what poppler's dependencies and libraries do.  pdftotext doesn't depend on a lot, but trying to use seccomp on pdftocairo could be difficult.
Comment 2 valo 2017-03-16 17:52:00 UTC
Yes I did not put much thought into the makefiles, as I mainly wanted to see how well this works and if you are interested in this.

Thank you for the advice about 'git format-patch'. That looks like a nice feature I didn't know about.

Regarding maintainability, you addressed an important point.
I have bee thinking about this for some time and wondered if the pledge approach from openBSD might be better suited in this regard then seccomp is.

Syscalls can change with different libraries and even when their versions change and this makes white listing quite tricky. An easy but less secure sulotion would be to switch to blacklisting only dangerous syscalls that should never be used by the application (like execve and ptrace). On the other hand I believe this can be covered with automated tests to make sure the filter is still correct with every new release.

I would like to see this be tested on some small utility like pdftotext and depending on the results it might be worth to adopt this into other applications as well.

Please note that this kind of syscall filter is currently also available for mupdf and evince https://github.com/LinuxSandboxingProject
Similar difficulties can be observed with those applications.

But pdftotext might be simple enough to be a valid target.
Comment 3 valo 2017-03-16 17:53:25 UTC
Created attachment 130267 [details]
improved libsec.cc
Comment 4 Albert Astals Cid 2017-03-18 10:54:44 UTC
If you want this to be accepted upstream:
 * it needs to support autoconf and cmake build systems
 * It needs to be optional

And not sure on this one, but given this seems to be a hand picked list and thus bound to be broken at any point it time, it needs to default to false even if the dependencies are found.
Comment 5 GitLab Migration User 2018-08-21 10:40:30 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/318.


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.