Re: sed suggestion: selinux context based on symlink when using -i

[Jakub Martisko]

As promised, here is the patch. Comments/suggestions are
welcomed.

Jakub

On 23.11.2017 14:26, Jakub Martisko wrote:
> Hi and thank you for your reply, I'll try to provide a
> complete patch as soon as I identify the correct entry for
> the NEWS file.
> As for the --follow-symlinks option - unpatched version
> fails at the first grep in the attached test script (and
> succeeds at the second one), while the patched version
> passes both. I hope this is the expected/wanted behaviour.
> 
> Thanks.
> Jakub
> 
> 
> 
> 
> On 22.11.2017 17:26, Jim Meyering wrote:
>> On Wed, Nov 22, 2017 at 3:23 AM, Jakub Martisko <address@hidden> wrote:
>>> On 11.1.2017 09:19, Jakub Martisko wrote:
>>>> First of all, congratulations and thanks for the GNU sed 4.3
>>>> release.
>>>>
>>>> Now to the main topic. There was a suggestion (see the
>>>> discussion in [1]) to change how the sed handles the selinux
>>>> context when working with symlinked files when using the -i
>>>> option. It was suggested that the context should be based on
>>>> the link instead of the target file itself. Patch that
>>>> changes this behavior was also proposed and is attached to
>>>> this message. Any thoughts about this suggestion?
>>>>
>>>> Regards,
>>>> Jakub
>>>>
>>>> P.S. I hope that his is a correct list where to post this.
>>>>
>>>>
>>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1401442
>>
>> Thank you for reporting that and for taking the time to prepare a patch.
>> Are you interested in making it a complete patch?
>> Since it is a bug fix, the patch should add an entry in the NEWS file.
>> Each bug fix also deserves regression tests: tests that fail before
>> and succeed after the patch is applied.
>> Will this patch have to take into account whether the
>> --follow-symlinks command-line option is specified? (haven't looked
>> yet)
>> If so, then there should be in-place tests that exercise this, both
>> with and without that option.
>>
>> Finally, for each bug fix, we try hard to identify the released
>> version in which the bug was introduced (mention that in NEWS), and
>> the actual commit (mention that in the commit log for the patch).
>>
>> Thanks again for the report and patch.
>> No obligation on your part to do any of the above.
>> Just outlining what's required, in case you're inclined.
>> If you don't do it, someone else will.
>>

>From a2f3cd0980735b4ab8513e68f276631baecb2341 Mon Sep 17 00:00:00 2001
From: Jakub Martisko <address@hidden>
Date: Thu, 23 Nov 2017 10:09:08 +0100
Subject: [PATCH] sed: fix selinux context when working inplace with symlinks

* sed/execute.exe sed -i now sets the selinux context of the file based
  on the symlink instead of its target. --follow-symlinks option remains
  unchanged.
* testsuite/inplace-selinux.sh test the above change
* credits: bug reported by Jakub Jelen
           fix proposed by Petr Lautrbach
---
 NEWS                         |  3 +++
 sed/execute.c                |  2 +-
 testsuite/inplace-selinux.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++
 testsuite/local.mk           |  1 +
 4 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100755 testsuite/inplace-selinux.sh

diff --git a/NEWS b/NEWS
index e9335f0..2035be9 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,9 @@ GNU sed NEWS                                    -*- outline 
-*-
   [Bug introduced in the original implementation of --posix option in
   v4.1a-5-gba68fb4]
 
+  sed -i now creates selinux context based on the context of the symlink
+  instead of the symlink target. [Bug present since at least sed-4.2]
+  sed -i --follow-symlinks remains unchanged.
 
 * Noteworthy changes in release 4.4 (2017-02-03) [stable]
 
diff --git a/sed/execute.c b/sed/execute.c
index 0d8fa2c..977f024 100644
--- a/sed/execute.c
+++ b/sed/execute.c
@@ -607,7 +607,7 @@ open_next_file(const char *name, struct input *input)
       if (is_selinux_enabled () > 0)
         {
           security_context_t con;
-          if (getfilecon (input->in_file_name, &con) != -1)
+          if (lgetfilecon (input->in_file_name, &con) != -1)
             {
               /* Save and restore the old context for the sake of w and W
                  commands.  */
diff --git a/testsuite/inplace-selinux.sh b/testsuite/inplace-selinux.sh
new file mode 100755
index 0000000..2f4a0e6
--- /dev/null
+++ b/testsuite/inplace-selinux.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+# Copyright (C) 2017 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+. "${srcdir=.}/testsuite/init.sh"; path_prepend_ ./sed
+print_ver_ sed
+
+fail=0
+
+#Create the first file and symlink pointing at it.
+echo "Hello World" > inplace-selinux-file || framework_failure_
+ln -s ./inplace-selinux-file inplace-selinux-link || framework_failure_
+
+chcon -h -t user_home_t inplace-selinux-file || framework_failure_
+chcon -h -t user_tmp_t inplace-selinux-link || framework_failure_
+
+
+#Create the second file and symlink pointing at it.
+#These will be used with the --follow-symlink option.
+echo "Hello World" > inplace-selinux-file2 || framework_failure_
+ln -s ./inplace-selinux-file2 inplace-selinux-link2 || framework_failure_
+
+chcon -h -t user_home_t inplace-selinux-file2 || framework_failure_
+chcon -h -t user_tmp_t inplace-selinux-link2 || framework_failure_
+
+#Modify prepared files inplace via the symlinks
+sed -i -e "s~Hello~Hi~" inplace-selinux-link || fail=1
+sed -i --follow-symlinks -e "s~Hello~Hi~" inplace-selinux-link2 || fail=1
+
+#Check selinux context - the first file shoul be created with the context 
+#of the symlink...
+ls -Z inplace-selinux-link | grep  user_tmp_t || fail=1
+#...the second file should use the context of the file itself.
+ls -Z inplace-selinux-file2 | grep  user_home_t || fail=1
+
+Exit $fail
diff --git a/testsuite/local.mk b/testsuite/local.mk
index e30f445..d465759 100644
--- a/testsuite/local.mk
+++ b/testsuite/local.mk
@@ -52,6 +52,7 @@ T =                                   \
   testsuite/help-version.sh            \
   testsuite/in-place-hyphen.sh         \
   testsuite/in-place-suffix-backup.sh  \
+  testsuite/inplace-selinux.sh         \
   testsuite/invalid-mb-seq-UMR.sh      \
   testsuite/mb-bad-delim.sh            \
   testsuite/mb-charclass-non-utf8.sh   \
-- 
2.9.5