[UBUNTU] zkey: Fails to run commands generated by 'zkey cryptsetup'

[bugproxy]

[Impact]

 * zkey utility attempts to execute cryptsetup binary and fails to find it, due to setting invalid PATH variable (which is missing /sbin). Stop resetting the PATH, and use the correct system default PATH which leads to correct execution of cryptsetup binary.

[Test Case]

 * enroll system with crypto adapters / get ready to use zkey
 * generate appropriate zkey for an encryption device
 * Use `$ zkey cryptsetup --run` to perform cryptsetup commands via zkey

[Regression Potential]

 * Currently only '--run' argument is broken, without '--run' the commands are printed that one can simply copy&paste and execute correctly under sudo. The effects of broken path, and using system path, have been discussed in depth at https://github.com/ibm-s390-tools/s390-tools/commit/9100327092c470c2e5b5819087c8094822a1c751 and will result in correctly attempting to execute cryptsetup binary. (it may later fail for other reasons, but that's beyond the scope here).

[Other Info]

 * Original bug report.

Description: zkey: Fails to run commands generated by 'zkey cryptsetup'

Symptom: Fails to run commands generated by 'zkey cryptsetup'.

Problem: When using 'zkey cryptsetup' with --run option the execution of the generated commands may fail, when the executable to be run is located in '/sbin'.

Solution: Include /sbin into PATH when executing commands.

Reproduction: Use 'zkey cryptsetup' with option --run on a distribution
              where 'cryptsetup' is located in '/sbin'.

Upstream commit:
https://github.com/ibm-s390-tools/s390-tools/commit/9100327092c470c2e5b5819087c8094822a1c751

[Steve Langasek (vorlon)]

Why is s390-tools overriding the path at all? The default path on Ubuntu already includes /sbin and /usr/sbin. Allowing use of the system path would seem preferable.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-11-20 02:56 EDT-------
We set the PATH before calling system() to execute the generated program for security reasons. That way a user can not manipulate the PATH environment variable and that way cause a different executable to be used. By setting the path we restrict the search path to the well known executable locations. Remember zkey may run as root or somewhat privileged so that it can execute "cryptsetup luksFormat" or similar.

There is also a similar note in the man page for system():
"Do not use system() from a privileged program (a set-user-ID or set-group-ID program, or a program with capabilities) because strange values for some environment variables might be used to subvert system integrity. For example, PATH could be manipulated so that an arbitrary program is executed with privilege. Use the exec(3) family of functions instead, but not execlp(3) or execvp(3) (which also use the PATH environment variable to search for an executable).

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-11-20 04:36 EDT-------
Additional info from Hendrik:

Ingo is correct here and we had a discussion about setting PATH explicitly for security reasons. For running zkey as regular user or as root is not the problem. But it becomes a security subject for running zkey with sudo. Assume you have granted a user the permission to run sudo zkey and the user constructs a PATH for finding the cryptsetup binary in a directory controlled by the user. If sudo zkey would then call this cryptsetup binary, the user can gain more privileges.

The alternative is to hard-code the path to the cryptsetup binary but that's typically a problem because it might be installed in different location depending on the Linux distributions.

So if you want to remove the PATH for Ubuntu, please either ensure that all calls to external programs use hard-coded paths or ensure that the default configuration for sudo sets up a pre-defined path (overriding any existing settings). Of course, this PATH configuration needs to be done for all kinds of such invocations, for example, su, ?

[Dimitri John Ledkov (xnox)]

sudo is enabled by default for the first installed user.

In sudoers we do specify secure_path as
Defaults secure_path="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin"

We do not include the current working directory in PATH.

Thus the recommendation is to fork&exec, rather than use system(). I.e. using execlp(), execvp(), execvp() as appropriate.

I do not see setting PATH or using hard-coded names to binaries as a protective measure. If one has root, one can bind mount their own binary on top of /usr/bin/cryptsetup if one really wants zkey to execute their own cryptsetup.

I am actually not sure why the choice is to exec cryptsetup, instead of using libcryptsetup.so.12 like cryptsetup itself does. It is not an additional runtime dependency, because /lib/systemd/systemd-cryptsetup is typically there and also uses libcryptsetup.so.12 directly.

Having a command as a long string, rather than list of args or an array, looks like there might be shell escape vulnerabilities. I wonder if I can trick the program to have key_file_name as passed to system() to be "filename; rm -rf ~/*" for example.

ps. I see system() is used elsewhere in the codebase too.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-11-20 10:32 EDT-------
Regarding fork&exec: This would not solve the PATH security problem either. So we would also need to build our own PATH environment for exec.

Regarding libcryptsetup use: Yes we could do this, but we are generating different type of command, "cryptsetup luksFormat" and "cryptsetup plainOpen". Implementing this based ob libcryptsetup would basically mean to re-implement lost of what is in cryptsetup already. So we would have to maintain it, keep it current with cryptsetup, etc. Lost of things that we don't have to do the in the current approach. Also, these commands are only generated when --run option is specified, otherwise we just output the command string.

Regarding shell escape vulnerabilities: The key file name is passed quoted to system, so that should be safe. Also if the key file name is checked when generating the key already, so you won't be able to generate a key with such a name. If you find a way to escape, then please open a new Bugzilla to report that.

[Dimitri John Ledkov (xnox)]

Are you deploying zkey/zkey-cryptsetup with setuid bit on?
Do you allow calls to zkey/zkey-cryptesetup with sudoers?
Do you allow to elevate to root whilst executing zkey/zkey-cryptsetup with policykit?

as in opening it up to execute zkey/zkey-cryptsetup with escalated privileges by otherwise non-privileged users?

Cause by default, zkey/zkey-cryptsetup is shipped without setuid, and effectively is harmless when called by non-privileged users without an ability to escalate privileges (by setting/controlling PATH environment, or any other means).

[Dimitri John Ledkov (xnox)]

I feel like I misunderstand the security implication here. Especially since it is my understanding, the binary requires to be executed by a priviledged user, and despite user-controlled PATH would not allow a non-priviledged user to escalate to root or execute arbitrary code.

[Dimitri John Ledkov (xnox)]

As discussed on https://github.com/ibm-s390-tools/s390-tools/commit/9100327092c470c2e5b5819087c8094822a1c751 it is agreed that it is sufficient if the default configurations of sudo and su (or anything similar) set a predefined secured path. Which has always been true on Ubuntu. Thus, on Ubuntu, one need not set PATH in this instance.

[Launchpad Janitor (janitor)]

This bug was fixed in the package s390-tools - 2.6.0-0ubuntu10

---------------
s390-tools (2.6.0-0ubuntu10) disco; urgency=medium

  * On Ubuntu, PATH is reset to a known secure path, when running programs
    as root, thus there is no need to reset PATH in zkey before calling
    system(). This fixes zkey failing to find cryptsetup. LP: #1803958

 -- Dimitri John Ledkov <email address hidden> Fri, 23 Nov 2018 02:45:09 +0000

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-11-23 02:01 EDT-------
Fine with me.

[Dimitri John Ledkov (xnox)]

However, i'd then to also want to bring back the use PBKDF2 as default PBKDF, patch too.

https://github.com/ibm-s390-tools/s390-tools/commit/a69470d7e02af2d7d6a4c781eef57727c0672fc0

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2019-02-21 06:55 EDT-------
I guess you are referring to the GitHub issue #56 that you just submitted (and closed)?

I have some more patches in the queue to ease things up even more with cryptsetup 2.1. You might want to wait for them, too....

[Dimitri John Ledkov (xnox)]

Yeap, that was it. However, I do need to get v2.8.0 into disco first....

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2019-02-21 08:35 EDT-------
These patches should be easy to backport to whatever zkey code level you have currently...

[Robie Basak (racb)]

The fix for this bug seems to have been inadvertently dropped from the current development release as far as I can tell. Something happened between 2.6.0-0ubuntu7 and 2.7.1-0ubuntu1 - 2.6.0-0ubuntu8, 2.6.0-0ubuntu9 and 2.6.0-0ubuntu10 are missing from debian/changelog in 2.7.1-0ubuntu1, and the fix for this bug was in 2.6.0-0ubuntu10 but vanished in 2.7.1-0ubuntu1. It looks like other fixes were also dropped somewhere, but I haven't investigated further.

Please could you investigate, fix up as needed, and explain what happened and how it was resolved so I can understand how to review this?

[Łukasz Zemczak (sil2100)]

That is also something I would like to know before reviewing the SRU further. Was the patch dropped in the development series by accident, or was that intentional? Should we get it back there?

[Dimitri John Ledkov (xnox)]

In later releases PATH is set to a value that includes cryptsetup utilities and `zkey cryptsetup` operates correctly there. Thus this patch is not needed in later releases.

The SRU is to fix broken PATH in cosmic only, as relevant to the cosmic codebase.

[Łukasz Zemczak (sil2100)]

Thanks for the context!

[Łukasz Zemczak (sil2100)]

Hello bugproxy, or anyone else affected,

Accepted s390-tools into cosmic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/s390-tools/2.6.0-0ubuntu7.3 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-cosmic to verification-done-cosmic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-cosmic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2019-05-23 05:08 EDT-------
We/IBM already verified it and confirm that it's working.

[Frank Heimes (fheimes)]

Adjusting tags according to #20

[Launchpad Janitor (janitor)]

This bug was fixed in the package s390-tools - 2.6.0-0ubuntu7.3

---------------
s390-tools (2.6.0-0ubuntu7.3) cosmic; urgency=medium

  * Guard against needlesly resetting cpi vars. LP: #1805841
  * Use system path to find cryptsetup binary. LP: #1803958
  * Cherrypick upstream patch to add OSA-Express7S defines in
    qethqoat. LP: #1815022

 -- Dimitri John Ledkov <email address hidden> Thu, 02 May 2019 14:21:10 +0100

[Łukasz Zemczak (sil2100)]

The verification of the Stable Release Update for s390-tools has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2019-06-11 02:34 EDT-------
IBM bugzilla status -> Fix released for all requested distros