New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DomU 'su' fails when vm-sudo auth is enabled, update GUI not working #2693

Closed
tasket opened this Issue Mar 11, 2017 · 18 comments

Comments

Projects
None yet
4 participants
@tasket

tasket commented Mar 11, 2017

Qubes OS version (e.g., R3.2):

R3,2

Affected TemplateVMs (e.g., fedora-23, if applicable):

fedora-24
fedora-25
debian-8
debian-9


Expected behavior:

qubes.InstallUpdatesGUI runs normally for templates with root authentication enabled (vm-sudo doc)

Actual behavior:

Update window briefly appears then disappears as the process aborts.

Steps to reproduce the behavior:

Enable vm-sudo authentication according to:
https://www.qubes-os.org/doc/vm-sudo/

...then start update process for that template (from Qubes Manager or rpc).

General notes:

This may be a general failure with su... I just tried the command alone and although I answered 'yes' to the dom0 auth prompt, it failed with "permission denied". Never had a problem with sudo.

I was able to replace su with sudo in /etc/qubes-rpc/qubes.InstallUpdatesGUI. For example,

xterm -title update -e su -l -c "$update_cmd; echo Done.;"

becomes...

xterm -title update -e "sudo -i sh -c \"$update_cmd; echo Done.;\""

That got updates working. It should also be possible to just piggyback su onto sudo.

The qubes.WaitForSession script also uses su.


Related issues:

@andrewdavidwong

This comment has been minimized.

Show comment
Hide comment
@andrewdavidwong

andrewdavidwong Mar 11, 2017

Member

Based on the doc page (in particular, "We do not support it in any of our packages..."), I think this is probably just a doc fix. PRs welcome.

Member

andrewdavidwong commented Mar 11, 2017

Based on the doc page (in particular, "We do not support it in any of our packages..."), I think this is probably just a doc fix. PRs welcome.

@andrewdavidwong andrewdavidwong added this to the Documentation/website milestone Mar 11, 2017

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Mar 12, 2017

I was hoping to defer to @marmarek to see if we can get this changed in qubes-core-agent-linux.

The vm-sudo config is something that should at least be supportable by the community. But positioning it to accumulate a list of errata would hamper that.

tasket commented Mar 12, 2017

I was hoping to defer to @marmarek to see if we can get this changed in qubes-core-agent-linux.

The vm-sudo config is something that should at least be supportable by the community. But positioning it to accumulate a list of errata would hamper that.

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Mar 12, 2017

Member

Replacing su with sudo here is not a good idea, as for example minimal template don't have sudo. This is especially important for qubes.WaitForSession as it must work everywhere (you may argue that qubes.InstallUpdatesGUI require additional packages there to work, but that would be still sub-optimal).
But qubes.WaitForSession may not need su at all, if the current user is already $USERNAME.

I think it's better to fix vm-sudo working with su, than replace everything with sudo. The fact that it doesn't work probably means that config on vm-sudo doc page is broken.

Quick test reveals that it works when you do not remove auth sufficient pam_unix.so line, but just drop nullok parameter. Not sure why, needs some more research.

Member

marmarek commented Mar 12, 2017

Replacing su with sudo here is not a good idea, as for example minimal template don't have sudo. This is especially important for qubes.WaitForSession as it must work everywhere (you may argue that qubes.InstallUpdatesGUI require additional packages there to work, but that would be still sub-optimal).
But qubes.WaitForSession may not need su at all, if the current user is already $USERNAME.

I think it's better to fix vm-sudo working with su, than replace everything with sudo. The fact that it doesn't work probably means that config on vm-sudo doc page is broken.

Quick test reveals that it works when you do not remove auth sufficient pam_unix.so line, but just drop nullok parameter. Not sure why, needs some more research.

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Mar 12, 2017

@marmarek Thanks! I think there is another workaround that doesn't involve more packages in minimal template:

Having Qubes Manager call vm.run_service with user="root" seems to fix the problem with qubes.InstallUpdatesGUI.

tasket commented Mar 12, 2017

@marmarek Thanks! I think there is another workaround that doesn't involve more packages in minimal template:

Having Qubes Manager call vm.run_service with user="root" seems to fix the problem with qubes.InstallUpdatesGUI.

@andrewdavidwong

This comment has been minimized.

Show comment
Hide comment
@andrewdavidwong

andrewdavidwong Mar 12, 2017

Member

I think it's better to fix vm-sudo working with su, than replace everything with sudo. The fact that it doesn't work probably means that config on vm-sudo doc page is broken.

I infer from the above that this should remain classified as a documentation issue.

Member

andrewdavidwong commented Mar 12, 2017

I think it's better to fix vm-sudo working with su, than replace everything with sudo. The fact that it doesn't work probably means that config on vm-sudo doc page is broken.

I infer from the above that this should remain classified as a documentation issue.

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Mar 12, 2017

Member

@marmarek Thanks! I think there is another workaround that doesn't involve more packages in minimal template:

Having Qubes Manager call vm.run_service with user="root" seems to fix the problem with qubes.InstallUpdatesGUI.

Indeed that would be a good idea, independently of fixing su issue.

Member

marmarek commented Mar 12, 2017

@marmarek Thanks! I think there is another workaround that doesn't involve more packages in minimal template:

Having Qubes Manager call vm.run_service with user="root" seems to fix the problem with qubes.InstallUpdatesGUI.

Indeed that would be a good idea, independently of fixing su issue.

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Mar 13, 2017

@marmarek Is it worth it to open a new issue for Qubes Manager workaround?

tasket commented Mar 13, 2017

@marmarek Is it worth it to open a new issue for Qubes Manager workaround?

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Mar 13, 2017

PR for QM run update as root:
QubesOS/qubes-manager#29

tasket commented Mar 13, 2017

PR for QM run update as root:
QubesOS/qubes-manager#29

@jpouellet

This comment has been minimized.

Show comment
Hide comment
@jpouellet

jpouellet Mar 13, 2017

Contributor

Just for reference for future self/others:

Also may affect salt: QubesOS/qubes-doc@c1332f4

Sudo not being in minimal template no longer the case: QubesOS/qubes-builder-rpm@e6a78e3

Contributor

jpouellet commented Mar 13, 2017

Just for reference for future self/others:

Also may affect salt: QubesOS/qubes-doc@c1332f4

Sudo not being in minimal template no longer the case: QubesOS/qubes-builder-rpm@e6a78e3

@tasket tasket referenced this issue in QubesOS/qubes-manager Mar 16, 2017

Merged

Run InstallUpdatesGUI service as root #29

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Mar 18, 2017

Replacing the qrexec-client-vm call with anything that returns 0 exit code (like whoami) still fails for su. So I opened a generic question about the problem at Stackexchange...

Logs that I posted for su and sudo:

Mar 18 08:46:39 localhost kernel: [   61.622184] audit: type=1100 audit(1489841199.166:114): pid=1107 uid=1000 auid=1000 ses=1 msg='op=PAM:authentication acct="root" exe="/bin/su" hostname=? addr=? terminal=/dev/pts/0 res=success'
Mar 18 08:46:39 localhost kernel: [   61.622480] audit: type=1101 audit(1489841199.166:115): pid=1107 uid=1000 auid=1000 ses=1 msg='op=PAM:accounting acct="root" exe="/bin/su" hostname=? addr=? terminal=/dev/pts/0 res=success'
Mar 18 08:46:39 localhost kernel: [   61.623224] audit: type=1103 audit(1489841199.167:116): pid=1107 uid=1000 auid=1000 ses=1 msg='op=PAM:setcred acct="root" exe="/bin/su" hostname=? addr=? terminal=/dev/pts/0 res=failed'
Mar 18 08:47:00 localhost kernel: [   82.750720] audit: type=1123 audit(1489841220.294:117): pid=1110 uid=1000 auid=1000 ses=1 msg='cwd="/home/user" cmd=67726570202D69206175646974202F7661722F6C6F672F6D65737361676573 terminal=pts/0 res=success'
Mar 18 08:47:00 localhost kernel: [   82.751369] audit: type=1110 audit(1489841220.295:118): pid=1110 uid=0 auid=1000 ses=1 msg='op=PAM:setcred acct="root" exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/0 res=failed'
Mar 18 08:47:00 localhost kernel: [   82.751814] audit: type=1105 audit(1489841220.295:119): pid=1110 uid=0 auid=1000 ses=1 msg='op=PAM:session_open acct="root" exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/0 res=success'

(corrected link above)

tasket commented Mar 18, 2017

Replacing the qrexec-client-vm call with anything that returns 0 exit code (like whoami) still fails for su. So I opened a generic question about the problem at Stackexchange...

Logs that I posted for su and sudo:

Mar 18 08:46:39 localhost kernel: [   61.622184] audit: type=1100 audit(1489841199.166:114): pid=1107 uid=1000 auid=1000 ses=1 msg='op=PAM:authentication acct="root" exe="/bin/su" hostname=? addr=? terminal=/dev/pts/0 res=success'
Mar 18 08:46:39 localhost kernel: [   61.622480] audit: type=1101 audit(1489841199.166:115): pid=1107 uid=1000 auid=1000 ses=1 msg='op=PAM:accounting acct="root" exe="/bin/su" hostname=? addr=? terminal=/dev/pts/0 res=success'
Mar 18 08:46:39 localhost kernel: [   61.623224] audit: type=1103 audit(1489841199.167:116): pid=1107 uid=1000 auid=1000 ses=1 msg='op=PAM:setcred acct="root" exe="/bin/su" hostname=? addr=? terminal=/dev/pts/0 res=failed'
Mar 18 08:47:00 localhost kernel: [   82.750720] audit: type=1123 audit(1489841220.294:117): pid=1110 uid=1000 auid=1000 ses=1 msg='cwd="/home/user" cmd=67726570202D69206175646974202F7661722F6C6F672F6D65737361676573 terminal=pts/0 res=success'
Mar 18 08:47:00 localhost kernel: [   82.751369] audit: type=1110 audit(1489841220.295:118): pid=1110 uid=0 auid=1000 ses=1 msg='op=PAM:setcred acct="root" exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/0 res=failed'
Mar 18 08:47:00 localhost kernel: [   82.751814] audit: type=1105 audit(1489841220.295:119): pid=1110 uid=0 auid=1000 ses=1 msg='op=PAM:session_open acct="root" exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/0 res=success'

(corrected link above)

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Jul 1, 2017

Had a response from hildred at Stackexchange, though maybe I'm not interpreting it right:

There are two issues I see right off the bat (other than trying experimental pam configs on common instead of just one service) is tat su always authenticates as root whereas sudo usually authenticates as the user, and su has a auth sufficient line and you use bracket notation in the common file. The bracket notation does not always set your success state. This can be fixed by adding an auth requires permit line before the exec or dropping the bracket notation as for your use case it is not needed or by also using bracket notation on the root ok line so that failing the sufficient test does not set negative success.
https://unix.stackexchange.com/a/374081/191050

tasket commented Jul 1, 2017

Had a response from hildred at Stackexchange, though maybe I'm not interpreting it right:

There are two issues I see right off the bat (other than trying experimental pam configs on common instead of just one service) is tat su always authenticates as root whereas sudo usually authenticates as the user, and su has a auth sufficient line and you use bracket notation in the common file. The bracket notation does not always set your success state. This can be fixed by adding an auth requires permit line before the exec or dropping the bracket notation as for your use case it is not needed or by also using bracket notation on the root ok line so that failing the sufficient test does not set negative success.
https://unix.stackexchange.com/a/374081/191050

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Jul 3, 2017

I've had some success with the suggestion to use pam_permit.so. In this case, I placed it near the end of /etc/pam.d/su just before the @include lines:

auth required pam_permit.so
@include common-auth
@include common-account
@include common-session

This results in su switching to root or specified user if the dom0 prompt (still triggered in common-auth) is answered 'Yes', and a system error if answered 'No'.

However the normal memory/timeout of the auth event doesn't work; subsequent su commands will still trigger the prompt no matter how soon after the prior command. (This has no effect on sudo which still enjoys the memory effect.)

tasket commented Jul 3, 2017

I've had some success with the suggestion to use pam_permit.so. In this case, I placed it near the end of /etc/pam.d/su just before the @include lines:

auth required pam_permit.so
@include common-auth
@include common-account
@include common-session

This results in su switching to root or specified user if the dom0 prompt (still triggered in common-auth) is answered 'Yes', and a system error if answered 'No'.

However the normal memory/timeout of the auth event doesn't work; subsequent su commands will still trigger the prompt no matter how soon after the prior command. (This has no effect on sudo which still enjoys the memory effect.)

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Jul 8, 2017

@marmarek I feel this issue could be resolved with the above change to /etc/pam.d/su (e.g. instructions in the vm-sudo doc), but I'd like your input on this approach of 'priming' the auth stack with pam_permit before letting pam_exec (in common-auth) have the final say.

Its still missing the timeout, but does at least make su usable again (in a secure way, I think). In R4.0 the timeout can perhaps be addressed with the pam-aware qrexec.

tasket commented Jul 8, 2017

@marmarek I feel this issue could be resolved with the above change to /etc/pam.d/su (e.g. instructions in the vm-sudo doc), but I'd like your input on this approach of 'priming' the auth stack with pam_permit before letting pam_exec (in common-auth) have the final say.

Its still missing the timeout, but does at least make su usable again (in a secure way, I think). In R4.0 the timeout can perhaps be addressed with the pam-aware qrexec.

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Jul 8, 2017

Alternately, this could be done in one code block in common-auth/system-auth with something like:

auth       [success=ok default=die]  pam_exec.so seteuid /usr/lib/qubes/qrexec-client-vm dom0 qubes.VMAuth /bin/grep -q ^1$
auth       required  pam_permit.so

tasket commented Jul 8, 2017

Alternately, this could be done in one code block in common-auth/system-auth with something like:

auth       [success=ok default=die]  pam_exec.so seteuid /usr/lib/qubes/qrexec-client-vm dom0 qubes.VMAuth /bin/grep -q ^1$
auth       required  pam_permit.so
@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Jul 8, 2017

I'm noticing now that the above is a shorter version of the usual Debian method of making sure auth works consistently across services that authenticate differently (such as su and sudo). On success this skips a rule so that pam_deny and pam_permit determine the final state:

auth  [success=1 default=ignore]      pam_unix.so nullok_secure
auth  requisite                       pam_deny.so
auth  required                        pam_permit.so

tasket commented Jul 8, 2017

I'm noticing now that the above is a shorter version of the usual Debian method of making sure auth works consistently across services that authenticate differently (such as su and sudo). On success this skips a rule so that pam_deny and pam_permit determine the final state:

auth  [success=1 default=ignore]      pam_unix.so nullok_secure
auth  requisite                       pam_deny.so
auth  required                        pam_permit.so

@tasket tasket referenced this issue in QubesOS/qubes-doc Jul 12, 2017

Merged

Fix PAM auth for 'su' command #446

@tasket

This comment has been minimized.

Show comment
Hide comment

tasket commented Jul 12, 2017

Doc PR submitted: QubesOS/qubes-doc#446

@andrewdavidwong

This comment has been minimized.

Show comment
Hide comment
@andrewdavidwong

andrewdavidwong Jul 16, 2017

Member

@tasket: Merged, thanks! Should this issue be closed now?

Member

andrewdavidwong commented Jul 16, 2017

@tasket: Merged, thanks! Should this issue be closed now?

@tasket

This comment has been minimized.

Show comment
Hide comment
@tasket

tasket Jul 16, 2017

Yes, closing.

tasket commented Jul 16, 2017

Yes, closing.

@tasket tasket closed this Jul 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment