bugQuilt - Bugs: bug #56969, Add confinment with nsjail

 
 

bug #56969: Add confinment with nsjail

Submitter:  Johannes <jsegitz>
Submitted:  Mon 30 Sep 2019 02:46:41 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Wont Fix
Privacy:  Public Assigned to:  khali
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Tue 04 Feb 2020 04:44:13 PM UTC, comment #4: 

OK, closing as requested.

Jean Delvare <khali>
Group administrator
Tue 04 Feb 2020 03:57:38 PM UTC, comment #3: 

I worked with this for a while and colleagues also used it. I don't think anymore that this should be added here since to much will break. We ran into various problems with non-default configurations and hunting down all of them seems useless.

Please close this issue (it seems I can't do that), I'll have a look if this can be soled at a lower level.

Johannes <jsegitz>
Wed 13 Nov 2019 03:07:17 PM UTC, comment #2: 

Two more comments:

  • Could we save the nsjail configuration file as $PC/.quilt_nsjail and then reuse it for subsequent commands? This would solve my concerns about the performance cost.


  • This piece of code:


> + mount_dir=$(pwd)
> + if [ -L patches ]; then
> +   # are we inside the already unpacked dir? If so mount the parent dir so we
> +   # have access to the patches
> +   mount_dir=$mount_dir/..
> + fi


looks frail. This assumes "quilt setup" was called without parameters such as -d or --sourcedir. What you want is to ensure that both the patches directory and the source tree directory are within $mount_dir, so you need to find the common ancestor. quilt/setup.in has utility functions dealing with that kind of problem, which could come handy here.

Jean Delvare <khali>
Group administrator
Wed 06 Nov 2019 04:07:50 PM UTC, comment #1: 

Thanks for your contribution and sorry for the delay.

First of all a high level question: what is the rationale for running all quilt commands in nsjail, as opposed to just "quilt setup"? I'm worried about the impact on performance. I know that "patch" had bugs which allowed crafted patches to do evil things, and while most of them are fixed now, I understand that there are certainly more such bugs lurking, but I also believe that you should not apply a patch without reviewing it first anyway. So from my perspective, "setup" is the only quilt command which would deal with potentially untrusted material.

If you really believe that all quilt commands should run in nsjail then I'm afraid the approach of creating a new nsjail environment for every command is wrong and we would rather initialize such an environment once and let all future commands reuse it.

Secondly, the patch makes the test suite ("make check") fail for me:

[add-filename-check.test]
[1] $ mkdir patches -- ok
[3] $ quilt new test.diff -- failed
quilt new: /home/khali/src/quilt/quilt/new: No such file or directory != Patch patches/test.diff is now on top

It's the first quilt command of the first test case so it seems to be a very general issue and not a corner case. This needs to be addressed before the patch can be considered for inclusion. I am really curious why this fails for me and not in the Build Service.

About the code itself, a few comments:

> +nsjail_config() {


The quilt style is opening curly brace on the next line for function declarations.

> + if [ -e "$QUILTRC" ]; then
> +   cat <<END_CONFIG >> $TMPFILE


"then" goes on the next line as well, and please use tabs for indentation.

> +if which nsjail >/dev/null 2>/dev/null; then


I think I would be more comfortable with a command line option. You may argue that your way is transparent while a command line option requires the user to know about the new option and actually use it. But on the other hand, with your way, if nsjail is not installed or not found for any reason, it won't be used even if the user would like to.

> + # we need to create a temporary config file since mounts with
> + # : don't work on the command line


This comment is really not clear to me, as you are not calling "mount" and I see no ":" in mount points in the generated configuration file either.

> + TMPFILE=`mktemp -t squilt.nsjail.XXXXXXXXXX` || exit 1


Please use gen_tempfile() instead (found in quilt/scripts/utilfns).

> + trap "rm -f -- '$TMPFILE'" EXIT


Please use add_exit_handler() instead (found in quilt/scripts/patchfns.in).

> + nsjail -Mo -q --config $TMPFILE -- /bin/bash $BASH_OPTS -c ". $QUILT_DIR/$command" "quilt ${command##*/}" "$@"


Do not hard-code "/bin/bash". The use may have asked for a different bash with --with-bash at configuration time (I do it frequently to test different versions of bash).

Jean Delvare <khali>
Group administrator
Mon 30 Sep 2019 02:46:41 PM UTC, original submission:  

A while ago we noticed that using quilt on spec files from untrusted sources it probably not a good idea
https://www.openwall.com/lists/oss-security/2018/09/27/2

I've used a wrapper for a while that uses nsjail to lock in quilt to prevent malicious spec files from harming the system
https://github.com/jsegitz/squilt

Me and my colleagues used it for a while. But since this requires users to change the default this will not be widely adopted.

I have a small patch that uses nsjail if it's available. Would you consider adding this to quilt? Users that don't have nsjail get the default behavior. For users that have nsjail installed this should also be transparent according to our testing.

Johannes <jsegitz>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #47594:  nsjail.patch added by jsegitz (3KiB - text/x-patch - Patch adding nsjail confinment)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by khali (Posted a comment)
  • -email is unavailable- added by jsegitz (Submitted the item)
  • -email is unavailable- added by jsegitz
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-02-04 khali StatusNeed Info Wont Fix
        Open/ClosedOpen Closed
    2019-11-13 khali StatusConfirmed Need Info
    2019-11-06 khali StatusNone Confirmed
        Assigned toNone khali
    2019-09-30 jsegitz Attached File- Added nsjail.patch, #47594
        Carbon-Copy- Added jsegitz

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code