Several fixes for security issues #720

meaksh · 2025-06-16T16:01:38Z

What does this PR do?

This PR backports several fixes for security issues:

It backports upstream commits from v3006.11..v3006.12 and adjust them to fit into our openSUSE/release/3006.0 branch.

Additionally, it also backports some related PRs that should fix the regression introduced in the v3006.12 release:

Tracks: https://github.com/SUSE/spacewalk/issues/27523

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Docs
Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
Tests written/updated

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

agraul

I compared this PR to git diff v3006.11..saltstack/3006.x, i.e. the changes upstream did between their last release and the current HEAD of v3006.x. The latter includes the CVE fixes and the fixes to regressions introduced in 3006.12.

I found some inconsistencies, it would be nice to double check them. I didn't run any checks, just compared the diffs.

salt/channel/client.py

agraul · 2025-06-25T10:02:41Z

salt/channel/client.py

+        if self.crypt == "aes":
+            ret["id"] = self.opts["id"]
+        return ret


Upstream differs here a bit, is that okay? See https://github.com/saltstack/salt/blob/e34f5ce3e3fc86edd564ce90aad822cbd70dfd93/salt/channel/client.py#L185-L189

Yes, this part is correct. It differs on purpose.

The encryption_algorithm and signing_algorithm is one of these other features in 3006.x that we don't currently have in our 3006.0 version and not really required for the CVE fixes. It is a big PR which also removed the M2Crypto compat layer, so I decided to not include it. See saltstack/salt#66589

agraul · 2025-06-25T11:36:21Z

salt/channel/client.py

+REQUEST_CHANNEL_TIMEOUT = 60
+REQUEST_CHANNEL_TRIES = 3


The timeout/retry changes (this and some other hunks in this file) are not part of the upstream changes since v3006.11. Do we need these to for the fixes?

Yeah, these changes are also expected in this PR. They are coming from 3006.x via saltstack/salt@5d4f358

I decided to bring these in as I saw different bits of the CVE fixes relying on these values. I didn't went too deep in here, but since these changes from saltstack/salt@5d4f358 are not to big and not too intrusive I decided to include them here instead of readjusting the code for the CVE fixes.

agraul · 2025-06-25T12:24:21Z

salt/minion.py

                ret["return"] = "ERROR executing '{}': {}".format(function_name, exc)
                ret["out"] = "nested"
                ret["retcode"] = salt.defaults.exitcodes.EX_GENERIC
+            except SaltClientError as exc:


This hunk is not part of the upstream changes since v3006.11, is it intended to have them in this PR?

This is also expected, as part of the timeout & retries changes. Coming from saltstack/salt@5d4f358

agraul · 2025-06-25T12:28:44Z

salt/minion.py

                force_refresh=data.get("force_refresh", False),
                notify=data.get("notify", False),
            )
+        elif tag.startswith("__master_req_channel_payload"):


This hunk is not part of the upstream changes since v3006.11, is it intended to have them in this PR?

Same as above. Part of saltstack/salt@5d4f358 (I should probably had mentioned it explicitely in the PR description)

agraul · 2025-06-25T12:59:10Z

tests/pytests/unit/test_crypt.py

    assert not salt.crypt.verify_signature(str(tmp_path.joinpath("bar.pub")), msg, sig)
+
+
+async def test_auth_aes_key_rotation(minion_root, io_loop):


Same question about async def here

We can use async def here. Lot of current tests we have in openSUSE/release/3006.0 are using it.

agraul · 2025-06-25T13:01:13Z

tests/pytests/unit/transport/test_zeromq.py

    assert res.result()["enc"] == "aes"


-def test_req_server_chan_encrypt_v2(pki_dir):


This hunk is not part of the upstream changes since v3006.11, is it intended to have them in this PR?

This change makes sense. I had to adjust this file a lot to resolve the conflicts backporting the CVE fixes and make the tests to pass.

The changes I did are basically taken from saltstack/salt#66589. I didn't want to include the whole PR as it is really big one. I prefer to restrict the scope of this PR to the CVE fixes as much as I can, without introducing lot of other features.

agraul · 2025-06-25T13:04:52Z

tests/pytests/unit/transport/test_zeromq.py

    def mocksend(msg, timeout=60, tries=3):
        client.transport.msg = msg
-        load = client.auth.crypticle.loads(msg["load"])
+        load = client.auth.session_crypticle.loads(msg["load"])


Further down in this test, upstream differs (created a new coroutine mockauthenticate. Do we not need it?

This expected. Same reason that above comment. This is actually aligned with 3006.x.

agraul · 2025-06-25T13:05:42Z

tests/pytests/unit/transport/test_zeromq.py

I find it hard to review this test file, there are a lot of changes that are not part of upstream's changes since v3006.11

This expected. Same reason that above comment. Changes are coming from saltstack/salt#66589

agraul · 2025-06-25T13:07:59Z

tools/pkg/build.py

Are we using this? Not that it hurts if we don't...

No, we don't really use it, but the file is present in our openSUSE/release/3006.0 branch, that's probably why we got the changes when backporting :)

agraul · 2025-06-25T13:17:14Z

I also saw that upstream has a few more tests, some of which we might want to run as well. For example, syndic scenarios, or the fileclient tests.

meaksh · 2025-06-25T15:00:53Z

I also saw that upstream has a few more tests, some of which we might want to run as well. For example, syndic scenarios, or the fileclient tests.

Hmm, which tests do you mean? I see upstream diff has a small change for tests/pytests/unit/fileclient/test_fileclient.py, but in our branch this file doesn't even exist. I don't see anything about syndic scenarios between v3006.11..v3006.12 neither in the other PRs.

UPDATE: I think you refer to tests that appaers between v3006.11..saltstack/3006.x. In such case, I was not thinking on backporting them as they are not part of the CVE release (v3006.11..v3006.12) neither required in the context of the CVE fixes. If we really want them, IMO that would be a separated PR.

(bsc#1244561, CVE-2024-38822) (bsc#1244564, CVE-2024-38823) (bsc#1244565, CVE-2024-38824) (bsc#1244566, CVE-2024-38825) (bsc#1244567, CVE-2025-22240) (bsc#1244568, CVE-2025-22236) (bsc#1244570, CVE-2025-22241) (bsc#1244571, CVE-2025-22237) (bsc#1244572, CVE-2025-22238) (bsc#1244574, CVE-2025-22239) (bsc#1244575, CVE-2025-22242) Request server hardening - Each minion get's it's own aes session for request server communication. - Request client always includes id and token, these are always validated server side. - Add timestamp and enforce configurable ttl for request server messages. Other relevant commit messages: - Add deprecation message to salt.auth.pki - Add test and fix for file_recv cve - Prevent traversal in local_cache::save_minions - Fix traversal in gitfs find_file - Fix traversals in salt.utils.virt - Fix traversal in pub_ret - On-demand pillar fix - Include url validation tests - Minion event filtering - Reasonable failures when pillars timeout - Adjust and fix code and tests after backporting to openSUSE/release/3006.0 Co-authored-by: Pablo Suárez Hernández <[email protected]>

fix file_recv path verification for subdirs Adapt backport to fit openSUSE/release/3006.0

Allow send_req_async to wait longer when sending a return back to the master. The minion should wait at least as long as the max possible return timeout. Update creds when session key changes Add unit test to validate session key rotation Add changelog for #68079

Clean up verify_load calls in master request server Remove tok in salt.channel.ReqServer.validate_token so it is not passed to the request handlers. Add tests around payload token removal Add changelog for #68076

Handle [email protected]/.. style remotes Fix checking of non-url style git remotes Fixes handling of git@hostname:/path/repo style remotes. Takes initial version from #68082 and fixes it. Still uses the shortcut of converting the remote to ssh:// URL style. Split out converting remote to URL Splits out converting remotes to URL form to allow testing of that conversion - without doing that risk issues with the regex Add additional test cases Add additional test cases based on gitfs docs and what they say should be valid Make utility functions classmethods Fix key vs remote wart Allow subdirs in GitFS find_file check (#68083) Add test for find_file in sub directories Add changelog for #68072

agraul

Thank you for the clarifications!

meaksh force-pushed the openSUSE/fix/3006.0-several-fixes-for-security-issues branch 3 times, most recently from 293a7b1 to befbf14 Compare June 25, 2025 09:12

agraul requested changes Jun 25, 2025

View reviewed changes

dwoz and others added 8 commits June 25, 2025 16:01

Fix test_pillar_timeout unit test in Salt Shaker

1834ce3

Fix tests failures on functional/channel/test_req_channel.py

45fe392

Fix gitfs test failures due uncomplete cleanup

addfbba

Fix cp.push module function and its integration test (#68053)

f365b90

fix file_recv path verification for subdirs Adapt backport to fit openSUSE/release/3006.0

Remove token to prevent decoding errors (#68084)

127f0c2

Clean up verify_load calls in master request server Remove tok in salt.channel.ReqServer.validate_token so it is not passed to the request handlers. Add tests around payload token removal Add changelog for #68076

meaksh force-pushed the openSUSE/fix/3006.0-several-fixes-for-security-issues branch from befbf14 to 7357906 Compare June 25, 2025 15:02

meaksh requested a review from agraul June 25, 2025 15:02

agraul approved these changes Jun 25, 2025

View reviewed changes

meaksh merged commit 398b545 into openSUSE/release/3006.0 Jun 26, 2025
8 of 9 checks passed

meaksh deleted the openSUSE/fix/3006.0-several-fixes-for-security-issues branch June 26, 2025 09:15

		assert not salt.crypt.verify_signature(str(tmp_path.joinpath("bar.pub")), msg, sig)


		async def test_auth_aes_key_rotation(minion_root, io_loop):

		assert res.result()["enc"] == "aes"


		def test_req_server_chan_encrypt_v2(pki_dir):

		REQUEST_CHANNEL_TIMEOUT = 60
		REQUEST_CHANNEL_TRIES = 3

Several fixes for security issues #720

Several fixes for security issues #720

Uh oh!

Conversation

meaksh commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Merge requirements satisfied?

Commits signed with GPG?

Uh oh!

agraul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meaksh Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meaksh Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meaksh Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meaksh Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meaksh Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agraul commented Jun 25, 2025

Uh oh!

meaksh commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agraul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

meaksh commented Jun 16, 2025 •

edited

Loading

meaksh Jun 25, 2025 •

edited

Loading

meaksh Jun 25, 2025 •

edited

Loading

meaksh Jun 25, 2025 •

edited

Loading

meaksh Jun 25, 2025 •

edited

Loading

meaksh Jun 25, 2025 •

edited

Loading

meaksh commented Jun 25, 2025 •

edited

Loading