FS#58626 - [file][pacman] seccomp-enabled file causes makepkg regression

Attached to Project: Pacman
Opened by userwithuid (userwithuid) - Thursday, 17 May 2018, 01:06 GMT
Last edited by Allan McRae (Allan) - Monday, 25 November 2019, 13:10 GMT
Task Type Bug Report
Category General
Status Closed
Assigned To Eli Schwartz (eschwartz)
Architecture All
Severity Low
Priority Normal
Reported Version 5.0.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

file 5.33-2, currently in testing, enabled libseccomp support, see  bug 58259  [0] .

This causes a regression where 'makepkg' is unable to extract plain compressed files (e.g. *.gz, *.xz but not e.g. *.tar.gz) from the sources array in a PKGBUILD.

* To reproduce, try building libpng, it has a *.patch.gz in its sources.
* This is expected as per `man 1 file` [1] (one needs to use -S when using -z or -Z now).
* This is trivial to fix for makepkg:

pacman sources, file.sh.in line 99 [2], add -S

- local file_type=$(file -bizL "$file")
+ local file_type=$(file -bizSL "$file")

* Outside of Arch, people might also be using file -z in their scripts though, and it will fail with a message "Bad system call". Considering that, Arch might want to play it safe and disable libseccomp for file again. Please make your decision before moving file from testing to core.



[0] https://bugs.archlinux.org/task/58259
[1] https://github.com/file/file/blob/FILE5_33/doc/file.man#L360
[2] https://git.archlinux.org/pacman.git/tree/scripts/libmakepkg/source/file.sh.in?id=652438772e0b1a5bdc48699a9f72057ea377df3c#n99
This task depends upon

This task blocks these from closing
 FS#58259 - [file] Add seccomp support 
Closed by  Allan McRae (Allan)
Monday, 25 November 2019, 13:10 GMT
Reason for closing:  Fixed
Additional comments about closing:  Fixed in 5.1.0 with de6249ce221aae4062ea131d4f676f7e3d44af28
Comment by Eli Schwartz (eschwartz) - Thursday, 17 May 2018, 01:17 GMT
  • Field changed: Attached to Project (Arch Linux → Pacman)
Um, this is clearly a pacman bug.
Comment by Eli Schwartz (eschwartz) - Thursday, 17 May 2018, 01:24 GMT
$ file -bizL /var/cache/makepkg/srcdest/libpng-1.6.34-apng.patch.gz
text/x-diff; charset=us-ascii compressed-encoding=application/x-gzip; charset=binary
$ pacman -Q file
file 5.33-2

Am I missing something?
Comment by userwithuid (userwithuid) - Thursday, 17 May 2018, 01:46 GMT
My bad, I posted a wrong reproducer (libpng). Can you try firefox instead, it currently has *.patch.xz files which should work to illustrate the problem.
Comment by Allan McRae (Allan) - Thursday, 17 May 2018, 01:57 GMT
Not patching makepkg like this. It would make makepkg incompatible with any file that is not built with libseccomp.

Probably the only way to handle this is to add a configure check for the -S flag and add it if needed.

Also, I'm not sure that this is not a bug in file. I not all that familiar with libseccomp, but shouldn't file give itself enough permissions to do its job?
Comment by userwithuid (userwithuid) - Thursday, 17 May 2018, 02:17 GMT
"but shouldn't file give itself enough permissions to do its job".

Problem is that file -z calls an external compressor binary (e.g. xz -d) and that one might use any syscall.

To me it seems more reasonable if file would just auto-disable seccomp when using -z , their own manpage [0] says "This enforcement does not provide any security benefit when file is asked to decompress input files running external programs with the -z option". Might want to ask upstream but the mailing list/bugtracker links at [1] appear to be down...



[0] https://github.com/file/file/blob/FILE5_33/doc/file.man#L531
[1] https://www.darwinsys.com/file/
Comment by Eli Schwartz (eschwartz) - Thursday, 17 May 2018, 02:58 GMT
So does it only break *.xz files, not *.gz ones?

Anyway the current file package now disables seccomp, but we should consider how to handle the general case, and potentially even re-enable seccomp.
Comment by userwithuid (userwithuid) - Thursday, 17 May 2018, 03:13 GMT
It breaks *.xz *.bz2 *.lz4 *.zst and so on. Anything where an external compressor binary is called. *.gz might just be handled internally.
Comment by userwithuid (userwithuid) - Thursday, 17 May 2018, 03:19 GMT
FWIW, I asked upstream about auto-disabling seccomp when -z is used, mailed to christos at astron (only active contact I could find):
--------------------
file 5.33 introduced libseccomp support. You mention in your man page
that -S (=disabling the sandbox) is needed when using -z or -Z
(=external decompressors).

This is not backwards-compatible. Scripts that previously worked fine
using -z on for example *.xz files will now fail with an obscure "Bad
system call". Supporting both a seccomp-enabled file 5.33 (where -S
must be added) and a seccomp-disabled file 5.33 (where -S does not
work) is also awkward.

In light of that, would it not make sense to simply disable the
sandbox automatically when -z or -Z is used? Or was this intentional
on your part?

A real world case where this causes issues can be found here:
https://bugs.archlinux.org/task/58626
--------------------
Comment by userwithuid (userwithuid) - Friday, 18 May 2018, 18:46 GMT
Response 1:
Disabling security features automatically renders them useless because
users think they are protected when they are not.

Me:
[...] Looking at the source code: For *.gz files, if file is compiled with zlib, it will happily use that lib internally for -z (BUILTIN_DECOMPRESS ifdefs in compress.c ). This implementation requires no additional -S by the user because calling the lib to do the decompressing already works with the current seccomp whitelist. If file were to also use liblzma, libzstd, liblz4, libbz2, etc instead of calling external progs, this would also work for those. [...]

Response 2a:
This is the plan. If you want it to happen faster, please supply patches.

Me:
[...] adopting file 5.33 with seccomp enabled might be hard because of compatibility [...] So this could end with downstreams not interested in enabling seccomp at all, which to me seems like a very undesirable outcome.

Response 2b:
It is a new feature and not fully backed yet. I know it is easy to add
fork and exec to the list of syscalls with -z or disable seccomp but
this is not desirable. If distros want to change that, they have the
source.



So I guess it's easiest to just keep seccomp disabled until file is updated to support -z with seccomp out-of-the-box.
Comment by Levente Polyak (anthraxx) - Thursday, 21 June 2018, 09:21 GMT
you can't support -z with seccomp out of the box without making seccomp mostly useless. you would need to add clone, wait4 and pipe whitelists to make shelling out work. that would indeed defeat the very purpose of having seccomp filter to avoid breaking out via syscalls. Security wise it would be a bad decision of file to imply -P on -z as pople have no clue they enger a danger zone by that. Implementing every possible decompressor also won't happen in any near future...
The right way for time being would indeed be to have a configure check for the availabillity of -P in file and use it when its available.
Comment by Eli Schwartz (eschwartz) - Wednesday, 24 July 2019, 15:21 GMT
https://github.com/file/file/commit/2a1bb655e4cf0a436a5d5313fcca95e55ba56a65

Great news! The latest version of file from git (future 5.38 release) supports -S as a no-op when built without libseccomp. Furthermore, it can be built with libbz2 and liblzma support in which case these filetypes will use internal decompression and be compatible with the sandbox.

On the makepkg front, this means we can now rely on file -S -bizl to work for any filetype, once we can guarantee the version of file which is installed. Question: how best to set this?

- Should we document a minimum version of file?
- Should we do a configure check to see what the file --version is, and decide whether to include -S?
-- If so, should this be a --with-file-seccomp=[yes|no|auto]
Comment by Eli Schwartz (eschwartz) - Wednesday, 24 July 2019, 16:32 GMT
Attached patch implements the feature detection via a configure option. Thoughts?

Assumptions:
- file 5.38 is going to be the next version of file, and contain the referenced commit.
- it's okay to have autoreconf/autogen.sh depend on the dist tarball being generated from a system with autoconf-archive installed.
- use clumsy piped run_commands in meson, until the next feature release of meson reduces complexity again.

EDIT: missed a couple hunks the first time.

Loading...