Bug 1219985

Summary: Symlinked states not found after fix-cve-2024-22231-and-cve-2024-22232-bsc-1219430-bs.patch
Product: [openSUSE] openSUSE Distribution Reporter: Georg Pfuetzenreuter <georg.pfuetzenreuter>
Component: SaltAssignee: E-Mail List <salt-maintainers>
Status: CONFIRMED --- QA Contact: E-mail List <qa-bugs>
Severity: Normal    
Priority: P2 - High CC: artem.shiliaev, georg.pfuetzenreuter, pablo.suarezhernandez, suse-beta
Version: Leap 15.5Flags: pablo.suarezhernandez: needinfo? (georg.pfuetzenreuter)
Target Milestone: ---   
Hardware: Other   
OS: openSUSE Leap 15.5   
URL: https://github.com/SUSE/spacewalk/issues/24798
Whiteboard:
Found By: --- Services Priority:
Business Priority: Blocker: ---
Marketing QA Status: --- IT Deployment: ---

Description Georg Pfuetzenreuter 2024-02-15 20:05:14 UTC 
Hi,

originating from https://gitlab.infra.opensuse.org/infra/salt/-/issues/20 (internal link) - in the openSUSE infrastructure we notice our complete Salt test suite to fail.

Upon investigation I found the culprit to be a combination of our use of symlinks to install certain formula states, and this update in the Salt package:

```
Thu Feb  1 12:07:20 UTC 2024 - Pablo Suárez Hernández <pablo.suarezhernandez@suse.com>

- Prevent directory traversal when creating syndic cache directory
  on the master (CVE-2024-22231, bsc#1219430)
- Prevent directory traversal attacks in the master's serve_file
  method (CVE-2024-22232, bsc#1219431)

- Added:
  * fix-cve-2024-22231-and-cve-2024-22232-bsc-1219430-bs.patch
```

More specifically, the problem lies with the changes in salt/fileserver/roots.py.

To reproduce, set up the following structure:

```
# more /srv/salt/test.sls 
first_state:
  test.succeed_without_changes

# more /srv/formula/myformula/init.sls 
myformula:
  test.succeed_without_changes

# ls -ld /srv/salt/myformula
lrwxrwxrwx 1 root root 22 Feb 15 19:53 /srv/salt/myformula -> /srv/formula/myformula
```

Before the patch, both the regular state file, as well as the symlink work:


```
# salt-call --local state.test test
local:
----------
          ID: first_state
    Function: test.succeed_without_changes
      Result: True
     Comment: Success!
     Started: 19:58:21.228111
    Duration: 1.578 ms
     Changes:   

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

# salt-call --local state.test myformula
local:
----------
          ID: myformula
    Function: test.succeed_without_changes
      Result: True
     Comment: Success!
     Started: 19:58:25.466603
    Duration: 1.506 ms
     Changes:   

Summary for local
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   1.506 ms
```

After the patch, only the regular state works, and the symlinked one fails with a very misleading error message:

```
# salt-call --local state.test test
local:
----------
          ID: first_state
    Function: test.succeed_without_changes
      Result: True
     Comment: Success!
     Started: 20:03:29.581793
    Duration: 1.299 ms
     Changes:   

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

# salt-call --local state.test myformula
local:
    Data failed to compile:
----------
    No matching sls found for 'myformula' in env 'base'
```

The debug output merely states:

```
[DEBUG   ] Could not find file 'salt://myformula.sls' in saltenv 'base'
[DEBUG   ] Could not find file 'salt://myformula/init.sls' in saltenv 'base'
```

The following is a patch to revert only the problematic parts of fix-cve-2024-22231-and-cve-2024-22232-bsc-1219430-bs.patch:

```
52237e8b96c0:/usr/lib/python3.6/site-packages/salt # diff -u fileserver/roots.py.orig fileserver/roots.py      
--- fileserver/roots.py.orig	2024-02-15 19:53:39.824084515 +0000
+++ fileserver/roots.py	2024-02-15 19:54:03.144242042 +0000
@@ -101,8 +101,8 @@
         full = os.path.join(root, path)
 
         # Refuse to serve file that is not under the root.
-        if not salt.utils.verify.clean_path(root, full, subdir=True):
-            continue
+        #if not salt.utils.verify.clean_path(root, full, subdir=True):
+        #    continue
 
         if os.path.isfile(full) and not salt.fileserver.is_file_ignored(__opts__, full):
             fnd["path"] = full
@@ -135,24 +135,24 @@
     gzip = load.get("gzip", None)
     fpath = os.path.normpath(fnd["path"])
 
-    actual_saltenv = saltenv = load["saltenv"]
-    if saltenv not in __opts__["file_roots"]:
-        if "__env__" in __opts__["file_roots"]:
-            log.debug(
-                "salt environment '%s' maps to __env__ file_roots directory", saltenv
-            )
-            saltenv = "__env__"
-        else:
-            return fnd
-    file_in_root = False
-    for root in __opts__["file_roots"][saltenv]:
-        if saltenv == "__env__":
-            root = root.replace("__env__", actual_saltenv)
-        # Refuse to serve file that is not under the root.
-        if salt.utils.verify.clean_path(root, fpath, subdir=True):
-            file_in_root = True
-    if not file_in_root:
-        return ret
+    #actual_saltenv = saltenv = load["saltenv"]
+    #if saltenv not in __opts__["file_roots"]:
+    #    if "__env__" in __opts__["file_roots"]:
+    #        log.debug(
+    #            "salt environment '%s' maps to __env__ file_roots directory", saltenv
+    #        )
+    #        saltenv = "__env__"
+    #    else:
+    #        return fnd
+    #file_in_root = False
+    #for root in __opts__["file_roots"][saltenv]:
+    #    if saltenv == "__env__":
+    #        root = root.replace("__env__", actual_saltenv)
+    #    # Refuse to serve file that is not under the root.
+    #    if salt.utils.verify.clean_path(root, fpath, subdir=True):
+    #        file_in_root = True
+    #if not file_in_root:
+    #    return ret
 
     with salt.utils.files.fopen(fpath, "rb") as fp_:
         fp_.seek(load["loc"])
```

Would appreciate assistance, as this breaks our setup.
Comment 1 Georg Pfuetzenreuter 2024-02-16 09:34:41 UTC 
Upstream discussion: https://github.com/saltstack/salt/issues/65977.
Comment 2 Pablo Suárez Hernández 2024-02-16 10:57:15 UTC 
Due to the CVE fixes, for security reasons the symlinks would ONLY work if the targetted file is part of the same "file_root".

By default, the "file_roots" for a Salt Master (and also for salt-call --local execution) are:

base:
    - /srv/salt
    - /srv/spm/salt

If you have formulas in other directories that you want to expose in the Salt fileserver, then maybe what you would need is to add it to the "file_roots" in Salt Master configuration (i.a. /etc/salt/master.d/custom.conf or minion configuration). Something like this:

file_roots:
  base:
    - /srv/salt/
    - /srv/spm/salt
    - /srv/formula/

Then you could remove the symlinks as the formulas will be exposed in the Salt fileserver as they are part of the file roots.

Would that work for you?
Comment 3 Georg Pfuetzenreuter 2024-02-16 12:37:03 UTC 
Hi Pablo,

thanks for getting back. We use the following roots:

```
file_roots:
  production:
    - /srv/salt
    - /usr/share/salt-formulas/states
    - /srv/formula
```

Inside `/srv/formula`, we locate symlinks such as the following:

```
# ls -al /srv/formula
total 88
drwxr-xr-x 1 root root 362 Dec 23 18:37 .
drwxr-xr-x 1 root root  98 Dec 26 22:56 ..
lrwxrwxrwx 1 root root  38 Oct 15 19:54 chrony -> /srv/formula-src/chrony-formula/chrony
lrwxrwxrwx 1 root root  52 Oct 15 19:54 elasticsearch -> /srv/formula-src/elasticsearch-formula/elasticsearch
lrwxrwxrwx 1 root root  44 Oct 15 19:54 firewalld -> /srv/formula-src/firewalld-formula/firewalld
lrwxrwxrwx 1 root root  40 Oct 15 19:54 haproxy -> /srv/formula-src/haproxy-formula/haproxy
lrwxrwxrwx 1 root root  44 Oct 26 21:17 hostsfile -> /srv/formula-src/hostsfile-formula/hostsfile
lrwxrwxrwx 1 root root  46 Oct 15 19:54 keepalived -> /srv/formula-src/keepalived-formula/keepalived
lrwxrwxrwx 1 root root  38 Oct 15 19:54 limits -> /srv/formula-src/limits-formula/limits
lrwxrwxrwx 1 root root  38 Oct 15 19:54 locale -> /srv/formula-src/locale-formula/locale
lrwxrwxrwx 1 root root  48 Oct 15 19:54 mirrorcache -> /srv/formula-src/mirrorcache-formula/mirrorcache
drwxr-xr-x 1 root root   0 Oct 15 19:54 _modules
lrwxrwxrwx 1 root root  36 Oct 15 19:54 mysql -> /srv/formula-src/mysql-formula/mysql
lrwxrwxrwx 1 root root  36 Oct 15 19:54 nginx -> /srv/formula-src/nginx-formula/nginx
lrwxrwxrwx 1 root root  42 Oct 15 19:54 openldap -> /srv/formula-src/openldap-formula/openldap
lrwxrwxrwx 1 root root  40 Oct 15 19:54 openssh -> /srv/formula-src/openssh-formula/openssh
lrwxrwxrwx 1 root root  42 Oct 15 19:54 powerdns -> /srv/formula-src/powerdns-formula/powerdns
lrwxrwxrwx 1 root root  46 Nov 26 21:14 prometheus -> /srv/formula-src/prometheus-formula/prometheus
lrwxrwxrwx 1 root root  40 Oct 15 19:54 rsyslog -> /srv/formula-src/rsyslog-formula/rsyslog
lrwxrwxrwx 1 root root  34 Oct 15 19:54 salt -> /srv/formula-src/salt-formula/salt
lrwxrwxrwx 1 root root  34 Oct 15 19:54 sssd -> /srv/formula-src/sssd-formula/sssd
drwxr-xr-x 1 root root   0 Oct 15 19:54 _states
lrwxrwxrwx 1 root root  40 Oct 15 19:54 sudoers -> /srv/formula-src/sudoers-formula/sudoers
lrwxrwxrwx 1 root root  38 Oct 15 19:54 sysctl -> /srv/formula-src/sysctl-formula/sysctl
drwxr-xr-x 1 root root  54 Oct 26 18:19 tayga
lrwxrwxrwx 1 root root  42 Oct 15 19:54 timezone -> /srv/formula-src/timezone-formula/timezone
lrwxrwxrwx 1 root root  36 Dec 23 18:37 users -> /srv/formula-src/users-formula/users
```

This allows us to enable and disable formulas dynamically.
If we were to list all directories as `file_roots` directly, this would on one hand require changing all our tooling (the links and configuration are all managed by carefully curated Salt states), but on the other hand, and probably more critically, any change (enabling/disabling formulas) would then require a full restart of the Salt Master, which I think is a feature decrease.
Comment 4 Georg Pfuetzenreuter 2024-02-16 12:42:53 UTC 
It's a bit odd to me how the patch does not cover the `fileserver_followsymlinks` configuration option. I think instead of blanket ignoring all links, one could have changed the default of this option to `False`, then our use case could have simply been facilitated with a `fileserver_followsymlinks: True`. But of course, I might be missing other considerations.
Comment 5 Pablo Suárez Hernández 2024-02-16 12:50:30 UTC 
(In reply to Georg Pfuetzenreuter from comment #3)
> 
> Inside `/srv/formula`, we locate symlinks such as the following:
> 
> ```
> # ls -al /srv/formula
> total 88
> drwxr-xr-x 1 root root 362 Dec 23 18:37 .
> drwxr-xr-x 1 root root  98 Dec 26 22:56 ..
> lrwxrwxrwx 1 root root  38 Oct 15 19:54 chrony ->
> /srv/formula-src/chrony-formula/chrony
> lrwxrwxrwx 1 root root  52 Oct 15 19:54 elasticsearch ->
> /srv/formula-src/elasticsearch-formula/elasticsearch
> lrwxrwxrwx 1 root root  44 Oct 15 19:54 firewalld ->
> /srv/formula-src/firewalld-formula/firewalld
> lrwxrwxrwx 1 root root  40 Oct 15 19:54 haproxy ->
> /srv/formula-src/haproxy-formula/haproxy
> lrwxrwxrwx 1 root root  44 Oct 26 21:17 hostsfile ->
> /srv/formula-src/hostsfile-formula/hostsfile
> lrwxrwxrwx 1 root root  46 Oct 15 19:54 keepalived ->
> /srv/formula-src/keepalived-formula/keepalived
> lrwxrwxrwx 1 root root  38 Oct 15 19:54 limits ->
> /srv/formula-src/limits-formula/limits
> lrwxrwxrwx 1 root root  38 Oct 15 19:54 locale ->
> /srv/formula-src/locale-formula/locale
> lrwxrwxrwx 1 root root  48 Oct 15 19:54 mirrorcache ->
> /srv/formula-src/mirrorcache-formula/mirrorcache
> drwxr-xr-x 1 root root   0 Oct 15 19:54 _modules
> lrwxrwxrwx 1 root root  36 Oct 15 19:54 mysql ->
> /srv/formula-src/mysql-formula/mysql
> lrwxrwxrwx 1 root root  36 Oct 15 19:54 nginx ->
> /srv/formula-src/nginx-formula/nginx
> lrwxrwxrwx 1 root root  42 Oct 15 19:54 openldap ->
> /srv/formula-src/openldap-formula/openldap
> lrwxrwxrwx 1 root root  40 Oct 15 19:54 openssh ->
> /srv/formula-src/openssh-formula/openssh
> lrwxrwxrwx 1 root root  42 Oct 15 19:54 powerdns ->
> /srv/formula-src/powerdns-formula/powerdns
> lrwxrwxrwx 1 root root  46 Nov 26 21:14 prometheus ->
> /srv/formula-src/prometheus-formula/prometheus
> lrwxrwxrwx 1 root root  40 Oct 15 19:54 rsyslog ->
> /srv/formula-src/rsyslog-formula/rsyslog
> lrwxrwxrwx 1 root root  34 Oct 15 19:54 salt ->
> /srv/formula-src/salt-formula/salt
> lrwxrwxrwx 1 root root  34 Oct 15 19:54 sssd ->
> /srv/formula-src/sssd-formula/sssd
> drwxr-xr-x 1 root root   0 Oct 15 19:54 _states
> lrwxrwxrwx 1 root root  40 Oct 15 19:54 sudoers ->
> /srv/formula-src/sudoers-formula/sudoers
> lrwxrwxrwx 1 root root  38 Oct 15 19:54 sysctl ->
> /srv/formula-src/sysctl-formula/sysctl
> drwxr-xr-x 1 root root  54 Oct 26 18:19 tayga
> lrwxrwxrwx 1 root root  42 Oct 15 19:54 timezone ->
> /srv/formula-src/timezone-formula/timezone
> lrwxrwxrwx 1 root root  36 Dec 23 18:37 users ->
> /srv/formula-src/users-formula/users
> ```

So, you have "/srv/formula" in "file_roots" containing symlinks to "/srv/formula-src" (which is NOT part of your file_roots, therefore symlinks fail)

If you add "/srv/formula-src" directory to your current "file_roots", and then you create the shortcut symlinks inside "/srv/formula-src" (instead of /srv/formula/") then the symlinks should work.

This does not require you restart the Salt Master on every new formula, as you would only add "/srv/formula-src" once, then all the formulas would be available.
Comment 6 Georg Pfuetzenreuter 2024-02-16 12:53:47 UTC 
Hi Pablo,

I understand what you mean now. Unfortunately, I am not quite confident with it - `/srv/formula-src` is a Git repository. Installing untracked symlinks there is somewhat ugly. We could think of tracking the links as part of the repository.
Comment 7 Pablo Suárez Hernández 2024-02-16 13:10:44 UTC 
(In reply to Georg Pfuetzenreuter from comment #4)
> It's a bit odd to me how the patch does not cover the
> `fileserver_followsymlinks` configuration option. I think instead of blanket
> ignoring all links, one could have changed the default of this option to
> `False`, then our use case could have simply been facilitated with a
> `fileserver_followsymlinks: True`. But of course, I might be missing other
> considerations.

Hey Georg,
IIUC "fileserver_followsymlinks" would still affect symlinks that are targetting files which are part of the same file root.
Comment 8 Georg Pfuetzenreuter 2024-02-16 14:46:23 UTC 
I now tried your suggestion.

This works:


```
c0c6bbd75dbf:/srv/formula-src # ls -ld chrony
lrwxrwxrwx 1 root root 21 Feb 16 13:24 chrony -> chrony-formula/chrony
c0c6bbd75dbf:/srv/formula-src # more /etc/salt/minion.d/roots.conf 
file_roots:
  base:
    - /srv/salt
    - /usr/share/salt-formulas/states
    - /srv/formula-src
c0c6bbd75dbf:/srv/formula-src # salt-call --local state.show_sls chrony
local:
    ----------
    chrony-package-install-pkg-installed:
....
```

However this, in my opinion slightly cleaner variant, does not:

```
c0c6bbd75dbf:/srv/formula-src # ls -ld states/chrony
lrwxrwxrwx 1 root root 24 Feb 16 13:23 states/chrony -> ../chrony-formula/chrony
c0c6bbd75dbf:/srv/formula-src # more /etc/salt/minion.d/roots.conf 
file_roots:
  base:
    - /srv/salt
    - /usr/share/salt-formulas/states
    - /srv/formula-src/states
c0c6bbd75dbf:/srv/formula-src # salt-call --local state.show_sls chrony
local:
    - No matching sls found for 'chrony' in env 'base'
```

Then again, this variant does:

```
c0c6bbd75dbf:/srv/formula-src # ls -ld chrony                      
lrwxrwxrwx 1 root root 34 Feb 16 13:32 chrony -> repositories/chrony-formula/chrony
c0c6bbd75dbf:/srv/formula-src # more /etc/salt/minion.d/roots.conf 
file_roots:
  base:
    - /srv/salt
    - /usr/share/salt-formulas/states
    - /srv/formula-src
c0c6bbd75dbf:/srv/formula-src # salt-call --local state.show_sls chrony
local:
    ----------
    chrony-package-install-pkg-installed:
        ----------
...
```

I think I will opt for this last variant as to not clutter our repository top level with both, submodules and symlinks.

> would still affect symlinks that are targetting files which are part of the same file root.

Thanks, makes sense.
Comment 9 Georg Pfuetzenreuter 2024-02-16 21:36:15 UTC 
Some big refactoring in our Salt setup later, we now have an operational setup again:

https://code.opensuse.org/heroes/salt/c/37c6d020e8e6e154717503d8bddacafb3be6aef9?branch=production
https://code.opensuse.org/heroes/salt-formulas-git/c/7e7a1d137c46d24ea8362faeb1b7b9fb0b83428b?branch=production

Now I can think of at least two other Salt infrastructures I maintain where the problematic symlink setup is used .. assuming this new behavior is now expected, I guess I will have to do the refactoring work there as well.

Thank you for the help.