Compatibility with or without Usr-Merge

[shundhammer]

Trello

https://trello.com/c/vySWWKUv/

Bugzilla / GitHub Issue

• https://bugzilla.suse.com/show_bug.cgi?id=1196086
• https://bugzilla.suse.com/show_bug.cgi?id=1196085
• yast-iscsi-client assumes iscsi commands will be in /sbin #111

Problem

External programs were called with an absolute path, even those from known secure directories like /sbin, /usr/sbin, /bin, /usr/bin. But for the usr-merge, they are being moved from /sbin to /usr/sbin and from /bin to /usr/bin, so those absolute paths would need to be adapted for distros that do that.

openSUSE Tumbleweed already did that, SLE-15 (GA, SPx) and Leap 15.x did not, so we would need to use different paths to call those external programs, depending on the target distro.

Solution

Don't call them with the full path, use $PATH instead; we are using a well-defined secure $PATH that we set up at the very start of YaST. That $PATH is inherited by all child processes.

Why Doesn't this Fail right now in TW?

Right now, TW has some compatibility symlinks, including:

• /bin -> /usr/bin
• /sbin -> /usr/sbin

So those programs from the open-iscsi package can still be found via those. How long those symlinks will remain available is anyone's guess; while /bin -> /usr/bin will probably not be removed for the forseeable future (too many third-party scripts would break), the same is not so sure about /sbin -> /usr/sbin.

We want to make sure, not make assumptions.

More Details

• Gist
• Same document in the yast-yast2/doc directory

Target Branch

This should not to merged to master before we branched off SLE-15 SP4 / Leap 15.4.

[coveralls]

Pull Request Test Coverage Report for Build 2153740547

• 3 of 7 (42.86%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 22.032%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/clients/inst_iscsi-client.rb	0	1	0.0%
src/clients/iscsi-client_finish.rb	0	1	0.0%
src/modules/IscsiClientLib.rb	3	5	60.0%

Totals
Change from base Build 2109452658:	0.0%
Covered Lines:	579
Relevant Lines:	2628

---

💛 - Coveralls

[shundhammer]

@jsegitz: Please have a look from the security PoV.

See also https://github.com/yast/yast-yast2/blob/master/doc/yast-invoking-external-commands.md

[mgerstner]

The original intention of using absolute paths was more kind of a hardening thought I guess. For a code reviewer it feels better that way since there are less possibilities of surprises.

If it is made sure, as the PR description says, that the PATH is sanitized to only contain trusted paths then this change is unproblematic.

That the shell is used for all external commands is a bit unlucky. It mostly could cause problems when the code is invoked in a privilege escalation context where also environment variables are inherited, like in a setuid-root context or su/sudo context with such configuration. In that case it is the responsibility of the people designing or invoking the privilege escalation to keep things safe.

[shundhammer]

The original intention of using absolute paths was more kind of a hardening thought I guess. For a code reviewer it feels better that way since there are less possibilities of surprises.

Sure, that were the original intentions of the change after the security audit some years ago. It wasn't easy for the auditor to to assess in what different ways external commands are being called.

That's why we collected them in that document so everybody reading the code (within or outside the YaST team) can have an overview:

https://github.com/yast/yast-yast2/blob/master/doc/yast-invoking-external-commands.md

If it is made sure, as the PR description says, that the PATH is sanitized to only contain trusted paths then this change is unproblematic.

Okay, thank you, that was what we wanted to be confirmed!

That the shell is used for all external commands is a bit unlucky. It mostly could cause problems when the code is invoked in a privilege escalation context where also environment variables are inherited, like in a setuid-root context or su/sudo context with such configuration. In that case it is the responsibility of the people designing or invoking the privilege escalation to keep things safe.

Sure. That's why our modern way of invoking commands (YaST::Execute()) calls them directly without a shell. But that old code wit SCR::Execute() is still in wide use in all that legacy code (and we have a lot of that).

Long term, our intention is of course to modernize the code when we touch an area anyway. But that's a question of available resources; not least at all because when we do a change of such magnitude, we need good test coverage first, and that's also a sore point of much of that code that in part goes back until the early 2000s.

Thank you for your assessment!

[lslezak]

Looks like a mistake in merge or missing merge, lots of comments were lost...

[yast-bot]

✔️ Public Jenkins job #42 successfully finished
✔️ Created OBS submit request #969363