[BUG] selinux.fcontext_policy_present can fail when the filespec is a regex #65340

ndptech · 2023-10-04T08:10:17Z

Description
A change introduced in 3006.3 to the selinux module function _fcontext_add_or_delete_policy checks for the presence of an existing matching context, and if one is found changes the action from "add" to "modify".

In certain corner cases, where regex are used for the filespec, this detection can match other contexts - which then changes the action to "modify", however since the policy does not actually exist, the attempt to modify fails.

Setup

Use a state file selinux.sls containing:

context1:
  selinux.fcontext_policy_present:
    - name: /srv/ssl/ldap/.*[.]key
    - sel_type: slapd_cert_t

context2:
  selinux.fcontext_policy_present:
    - name: /srv/ssl/ldap(/.*[.](pem|crt))?
    - sel_type: cert_t

Steps to Reproduce the behavior

Given the above state file, and neither, fcontext policy already existing, salt <minion_id> state.sls selinux will create the first selinux fcontext policy but not the second.

Output will be:

minion_id:
----------
          ID: context1
    Function: selinux.fcontext_policy_present
        Name: /srv/ssl/ldap/.*[.]key
      Result: True
     Comment: 
     Started: 09:00:08.369900
    Duration: 1480.618 ms
     Changes:   
              ----------
              new:
                  ----------
                  /srv/ssl/ldap/.*[.]key:
                      ----------
                      filetype:
                          all files
                      sel_type:
                          slapd_cert_t
              old:
                  ----------
----------
          ID: context2
    Function: selinux.fcontext_policy_present
        Name: /srv/ssl/ldap(/.*[.](pem|crt))?
      Result: False
     Comment: Error adding new rule: {'pid': 52772, 'retcode': 1, 'stdout': '', 'stderr': 'ValueError: File context for /srv/ssl/ldap(/.*[.](pem|crt))? is not defined'}
     Started: 09:00:09.850697
    Duration: 946.32 ms
     Changes:   

Summary for minion_id
------------
Succeeded: 1 (changed=1)
Failed:    1
------------
Total states run:     2

Expected behavior
Both states should have succeeded.

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

Salt Version:
          Salt: 3006.3
 
Python Version:
        Python: 3.10.13 (main, Sep  6 2023, 02:11:27) [GCC 11.2.0]
 
Dependency Versions:
          cffi: 1.14.6
      cherrypy: unknown
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.2
       libgit2: Not Installed
  looseversion: 1.0.2
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     packaging: 22.0
     pycparser: 2.21
      pycrypto: Not Installed
  pycryptodome: 3.9.8
        pygit2: Not Installed
  python-gnupg: 0.4.8
        PyYAML: 6.0.1
         PyZMQ: 23.2.0
        relenv: 0.13.10
         smmap: Not Installed
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: 4.3.4
 
System Versions:
          dist: rocky 9.2 Blue Onyx
        locale: utf-8
       machine: x86_64
       release: 5.14.0-284.25.1.el9_2.x86_64
        system: Linux
       version: Rocky Linux 9.2 Blue Onyx

The text was updated successfully, but these errors were encountered:

welcome · 2023-10-04T08:10:19Z

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

finalduty · 2023-10-13T01:10:19Z

I have a similar issue relating to the same change in code. In my case, the new modify logic doesn't take in to account when a filetype is specified.

For example, RHEL/CentOS Stream 9 has a default fcontext policy for /dev/log which only applies to symlinks:

[user@minion ~]$ sudo semanage fcontext -l | grep '^/dev/log '
/dev/log                                           symbolic link      system_u:object_r:devlog_t:s0

Our setup puts a socket file at /dev/log rather than a symlink, so we use the following state to add the correct fcontext, which has been working fine:

/dev/log:
  selinux.fcontext_policy_present:
    - filetype: s
    - sel_type: devlog_t

But now results in the following error:

local:
----------
          ID: /dev/log
    Function: selinux.fcontext_policy_present
      Result: False
     Comment: Error adding new rule: {'pid': 2047936, 'retcode': 1, 'stdout': '', 'stderr': 'ValueError: File context for /dev/log is not defined'}
     Started: 11:17:35.234723
    Duration: 2185.499 ms
     Changes:   

Summary for local
------------
Succeeded: 0
Failed:    1
------------
Total states run:     1

dmurphy18 · 2023-11-08T21:14:00Z

The proposed fix in #65341 does not work. the line executed in _fcontext_add_or_delete_policy

cmd = f"semanage fcontext -l | egrep {filespec} "

where filespec is filespec: '/srv/ssl/ldap\\(/\\.\\*\\[\\.\\]\\(pem\\|crt\\)\\)\\?', egrep is matching that and returning /srv/ssl/ldap/.*[.]key

filespec is generated from filespec = re.escape(name) and name is '/srv/ssl/ldap(/.*[.](pem|crt))?'

Problem appears to be egrep and handling the matching

/srv/ssl/ldap/\.\*\[\.\]key
/srv/ssl/ldap\(/\.\*\[\.\]\(pem\|crt\)\)\?

the problem is the '?' - optional or at least once, hence the match is occurring since the grep is matching '/srv/ssl/ldap'

[root@Unknown site-packages]# semanage fcontext -l | egrep '/srv/ssl/ldap/\.\*\[\.\]key'
/srv/ssl/ldap/.*[.]key                             all files          system_u:object_r:slapd_cert_t:s0 
[root@Unknown site-packages]# semanage fcontext -l | egrep '/srv/ssl/ldap\(/\.\*\[\.\]\(pem\|crt\)\)\?'
[root@Unknown site-packages]# semanage fcontext -l | grep '/srv/ssl/ldap\(/\.\*\[\.\]\(pem\|crt\)\)\?'
/srv/ssl/ldap/.*[.]key                             all files          system_u:object_r:slapd_cert_t:s0 
[root@Unknown site-packages]# semanage fcontext -l | grep -E '/srv/ssl/ldap\(/\.\*\[\.\]\(pem\|crt\)\)\?'
[root@Unknown site-packages]# semanage fcontext -l | grep '/srv/ssl/ldap\(/\.\*\[\.\]\(pem\|crt\)\)'
[root@Unknown site-packages]#

So interesting issue, I am using Centos 9 and am getting different results with grep, grep -E and egrep
Rock 9.2 should be similar to Centos 9, which makes me wonder what version of egrep is getting used internally with Salt 3006.4. I will investigate that.

Either way the problem above is the regex matching is picking off the same due to that '?', and that is why the second value is not working since it matches the first, that is, /src/ssl/ldap

The error is in the values for name defined by the user. Please consider closing this issue unless there are further details to consider.

dmurphy18 · 2023-11-08T21:51:02Z

same egrep used inside salt and on the command line

[root@Unknown salt]# which egrep
alias egrep='egrep --color=auto'
	/bin/egrep
[root@Unknown salt]# find / -name egrep
find: ‘/run/user/1000/gvfs’: Permission denied
/usr/bin/egrep
[root@Unknown salt]# l /usr/bin/egrep
-rwxr-xr-x. 1 root root 32 Aug  9  2021 /usr/bin/egrep
[root@Unknown salt]# l /bin/egrep
-rwxr-xr-x. 1 root root 32 Aug  9  2021 /bin/egrep
[root@Unknown salt]# /usr/bin/egrep --version
grep (GNU grep) 3.6
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Mike Haertel and others; see
<https://git.sv.gnu.org/cgit/grep.git/tree/AUTHORS>.
[root@Unknown salt]# /bin/egrep --version
grep (GNU grep) 3.6
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Mike Haertel and others; see
<https://git.sv.gnu.org/cgit/grep.git/tree/AUTHORS>.
[root@Unknown salt]#

Forced to use /bin/egrep in hacked version with debugger, but same result finding the key named line, perhaps I am not typing correctly something on the command line when I try with /bin/egrep, since it still stands, that '?' is a problem for your regex.

And a helping of crow pie, believe I have found the issue, salt needs a pair of quotes, that is,

cmd = f"semanage fcontext -l | /bin/egrep '{filespec}'"

Because of when I try it on the command line, of course I use quotes for the regex 🤦 , and they are missing in the current Salt. Very simple fix, now to write the tests and changelog.

Still worried about the '?' and matching to '/srv/ssl/ldap' but I will brush up on my regex to understand why the ?, and its affect on the grouping for the second name

dmurphy18 · 2023-11-17T20:38:22Z

closing as PR is merged

ndptech added Bug broken, incorrect, or confusing behavior needs-triage labels Oct 4, 2023

ndptech added a commit to ndptech/salt that referenced this issue Oct 4, 2023

fixes saltstack#65340 correct use of egrep to parse semanage output

92bdf25

ndptech mentioned this issue Oct 4, 2023

[3006.x] fixes #65340 correct use of egrep to parse semanage output #65341

Closed

anilsil added this to the Sulfur v3006.5 milestone Oct 4, 2023

anilsil assigned dmurphy18 Oct 4, 2023

anilsil removed the needs-triage label Oct 4, 2023

dmurphy18 added expected-behavior intended functionality info-needed waiting for more info and removed Bug broken, incorrect, or confusing behavior labels Nov 8, 2023

dmurphy18 added Bug broken, incorrect, or confusing behavior and removed expected-behavior intended functionality info-needed waiting for more info labels Nov 8, 2023

dmurphy18 mentioned this issue Nov 9, 2023

[3006.x] Ensure quoted filespec when using egrep to allow for regex with selinux #65524

Merged

3 tasks

dmurphy18 closed this as completed Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] selinux.fcontext_policy_present can fail when the filespec is a regex #65340

[BUG] selinux.fcontext_policy_present can fail when the filespec is a regex #65340

ndptech commented Oct 4, 2023

welcome bot commented Oct 4, 2023

finalduty commented Oct 13, 2023 •

edited

Loading

dmurphy18 commented Nov 8, 2023 •

edited

Loading

dmurphy18 commented Nov 8, 2023 •

edited

Loading

dmurphy18 commented Nov 17, 2023

[BUG] selinux.fcontext_policy_present can fail when the filespec is a regex #65340

[BUG] selinux.fcontext_policy_present can fail when the filespec is a regex #65340

Comments

ndptech commented Oct 4, 2023

welcome bot commented Oct 4, 2023

finalduty commented Oct 13, 2023 • edited Loading

dmurphy18 commented Nov 8, 2023 • edited Loading

dmurphy18 commented Nov 8, 2023 • edited Loading

dmurphy18 commented Nov 17, 2023

finalduty commented Oct 13, 2023 •

edited

Loading

dmurphy18 commented Nov 8, 2023 •

edited

Loading

dmurphy18 commented Nov 8, 2023 •

edited

Loading