Implement optional admin authorization via qrexec by HW42 · Pull Request #613 · QubesOS/qubes-core-agent-linux

HW42 · 2025-10-14T13:19:26Z

See included README for details.

marmarek

I've looked at the PAM module implementation for now, but not much at the PAM configs. On the first sight looks like there is quite a bit of duplication in the PAM configs, but that's probably unavoidable to make it work with any authselect profile...

marmarek · 2025-10-14T13:56:45Z

+all: qubes-admin-authzd pam_qubes_admin_authz.so
+
+qubes-admin-authzd: qubes-admin-authzd.c qubes-admin-authz-common.h
+	gcc -O2 -Wall -Wextra -Werror -fPIC -pie $< -o $@


Include standard CFLAGS var too (both debian and fedora add preferred hardening options here)

marmarek · 2025-10-14T13:56:51Z

+	gcc -O2 -Wall -Wextra -Werror -fPIC -pie $< -o $@
+
+pam_qubes_admin_authz.so: pam_qubes_admin_authz.c qubes-admin-authz-common.h
+	gcc -O2 -Wall -Wextra -Werror -fPIC -pie -shared $< -o $@ -lpam


and here too

marmarek · 2025-10-14T14:00:08Z

+dom0. For example:
+
+```
+qubes.AuthorizeInVMAdminAccess * * @default target=dom0 default_target=dom0


"ask" is missing

Fixed. I'm wondering if we should use "VM" in the service name or change it to "qube"?

I wonder about even simpler name like qubes.AuthorizeAdminAccess? But it might be confusing where that admin is (whether it gives dom0 admin access for example)... But OTOH that concern should be covered by the custom prompt already, right? (at least for the "ask" case)

@marmarta any opinion? Past related discussion starting at this comment

I think AuthorizeAdminAccess is ok, within context of the custom prompt.

As in, if we got a prompt that just had the service name I'm with @HW42 , but if there's a custom prompt, it's fine to omit it. If we do keep it, lets make it InQube, because from what I understand in can apply to things like remote qubes which might not be vms..

changed to qubes.AuthorizeAdminAccess

marmarek · 2025-10-14T14:05:49Z

+    if (!(strcmp(service, "su") == 0 ||
+          strcmp(service, "su-l") == 0 ||
+          strcmp(service, "sudo") == 0 ||
+          strcmp(service, "sudo-i") == 0 ||
+          strcmp(service, "polkit-1") == 0)) {


Is this necessary, instead of just adding it to only relevant PAM configuration files? If it's necessary, better have the list in the module parameter, so it can be adjusted without rebuilding (for example to use it for other services too, like doas or login).

I think yes, given how Debian and Fedora structure their PAM configurations. They have common stacks (like /etc/pam.d/common-auth) and those are then included by the service specific configs (like /etc/pam.d/su). Since the later are owned by those packages they are not so easy to update, unlike the former, for with their is authselect in Fedora and pam-auth-update in Debian.

My original plan was to configure it like this:

auth [default=1 success=ignore] pam_succeed_if.so use_uid user ingroup qubes service in su:su-l:sudo:sudo-i:polkit-1 auth sufficient pam_qubes_admin_authz.so

This works under Debian but the PAM stack under Fedora is broken because how of how it interacts with the relative jump (in default=1) works:

jump over the next N modules in the stack. Note that N equal to 0 is not allowed, it would be treated as ignore in such case. The side effect depends on the PAM function call: for pam_authenticate, pam_acct_mgmt, pam_chauthtok, and pam_open_session it is ignore; for pam_setcred and pam_close_session it is one of ignore, ok, or bad depending on the module's return value.

This means that for non matching requests pam_setcred get a failed state for non-matching requests and this is propagated through. This even breaks login on the console!

This can be worked around by negating the logic:

auth [default=ignore success=1] pam_succeed_if.so use_uid user ingroup qubes service in su:su-l:sudo:sudo-i:polkit-1 auth [default=1] pam_permit.so auth sufficient pam_qubes_admin_authz.so

But I disliked this relatively complex jumping around.

But making it configurable via a module parameter is easy. In the end you will need to re-build the package anyway. But it would be easier for experimenting or other unusual usage of the module.

this (and the group) is now configured by pam module arguments

marmarek · 2025-10-14T16:35:04Z

+            return 1;
+        }
+
+        execlp("qrexec-client-vm", "qrexec-client-vm", "@default", "qubes.AuthorizeInVMAdminAccess", NULL);


Just an idea: add PAM service name as the call argument? This way you could for example allow polkit but not sudo. Or do you think it it's not really useful?

Not sure about a use case, since you get full admin access in all cases. But if you think it's useful, I would not object.

I'm not really sure. It could be useful if there are cases where it allows some limited root access, but even for polkit it would allow pkexec (right?) so a full shell anyway... So, maybe not that useful?

Yes currently the polkit config allows full access. But yes, in the future their might be an PAM using program that doesn't give full access.

sudo has an pam_service option, so you might build something based on that. Since polkit is scriptable there probably too, but I didn't looked into that.

This is now implemented (Turned out to be a bit more involved than expected, since the info needs to be passed all the way).

HW42 · 2025-10-14T19:01:12Z

[...] but not much at the PAM configs. On the first sight looks like there is quite a bit of duplication in the PAM configs, but that's probably unavoidable to make it work with any authselect profile...

Those are direct copies of the Fedora configs, just with our line added. And yes AFAIU that's how you are supposed to use authselect: You can only define a full profile not a small addition like with pam-auth-config.

DemiMarie

Mostly looks good and I like the mechanism. The main caveat is that privilege escalation via various Xen devices is likely still possible. This is because libvchan does not have kernel support.

DemiMarie · 2025-10-15T06:08:17Z

+    if (read_ret < 0) {
+        pam_syslog(pamh, LOG_ERR, "failed to read from socket: %i", errno);
+        rc = PAM_SYSTEM_ERR;
+        goto ret;
+    }
+
+    // Since the other side is trusted this isn't strictly necessary. But it's
+    // probably still nicer to ensure that we don't put unexpected bytes into
+    // the log.
+    for (size_t i = 0; i < sizeof(res) - 1; i += 1) {
+        if (res[i] == '\0') {
+            break;
+        }
+        if (res[i] < 0x20 || res[i] > 0x7e) {
+            res[i] = '.';
+        }
+    }


Maybe check that the other side didn’t send a NUL byte in the string?

While not really helpful, I initially implemented this. It actually made the loop a bit simpler. But I have now switched to fgets which doesn't provide the read length (C's stdlib is really lacking ...). The code now handles a NUL like an incomplete read, which isn't nice, but not a problem either.

marmarek · 2025-10-15T06:50:18Z

The main caveat is that privilege escalation via various Xen devices is likely still possible.

Offtopic for this PR...

ArrayBolt3

A couple of notes and a few typo fixes. I didn't review the code thoroughly, but I skimmed part of it and read the documentation page.

ArrayBolt3 · 2025-10-16T01:30:32Z

+# If systemd is running try starting qubes-admin-authzd even if the user
+# blocked the normal start by dh (above) via policy-rc.d. Not running it will
+# break passwordless auth, so this is most likely not what the user wanted. But
+# if they really want to they still can mask the service.
+
+if [[ -e /run/systemd/system ]] &&
+   ! systemctl is-active --quiet qubes-admin-authzd.service &&
+   systemctl is-enabled --quiet qubes-admin-authzd.service
+then
+    systemctl start qubes-admin-authzd.service || true
+fi


Is there a concrete scenario where policy-rc.d blocks startup of something incorrectly? I'm a bit worried about assuming the user's intentions here, since Kicksecure's build system uses a "deny everything" policy-rc.d to prevent everything from starting, because of the fallout it can have on the rest of the build. Bypassing this mechanism seems dangerous at best. If a user does end up blocked, they can always open a DispVM console and log in as root, or use qvm-run -u root ....

I have this in (some of) my Debian templates as a stopgap solution to allow disabling services of newly installed packages before they are run (Debian's default policy).

since Kicksecure's build system uses a "deny everything" policy-rc.d to prevent everything from starting

But in that case you don't have a running systemd, right? So this is a nop.

That being said I don't feel too strongly about handling this corner case. So would be fine with dropping it, if preferred.

adrelanos · 2025-10-18T10:16:53Z

@DemiMarie

The main caveat is that privilege escalation via various Xen devices is likely still possible. This is because libvchan does not have kernel support.

harden insecure permissions inside /dev/xen folder / research security impact of the Qubes /dev/xen folder permissions qubes-issues#9717

See included README for details. Issue: QubesOS/qubes-issues#2695

HW42 · 2026-06-11T14:24:50Z

[...] but not much at the PAM configs. On the first sight looks like there is quite a bit of duplication in the PAM configs, but that's probably unavoidable to make it work with any authselect profile...

Those are direct copies of the Fedora configs, just with our line added. And yes AFAIU that's how you are supposed to use authselect: You can only define a full profile not a small addition like with pam-auth-config.

When reviewing diff it against /usr/share/authselect/default

HW42 · 2026-06-11T18:39:18Z

Bookworm fails, since it's gcc doesn't know the w64 format option. Since we don't support bookworm anymore, I don't think we care.

Will fix the archlinux error.

marmarek · 2026-06-11T21:20:13Z

since it's gcc doesn't know the w64 format option.

My manual of printf doesn't know it either... Maybe better use PRIu64 from inttypes.h instead?

HW42 · 2026-06-12T00:35:59Z

My manual of printf doesn't know it either... Maybe better use PRIu64 from inttypes.h instead?

It's a C23 feature and the manpages only recently catched up.

Given that all distros we still support seem to have an gcc that understands it, seems nicer than "PRIu64", but I can change it if you like.

marmarek · 2026-06-16T17:53:51Z

but I can change it if you like.

Yes, I would prefer that.

marmarek

Besides some minor comments below, it looks nice already. I've tested it on Fedora and seems to be working fine, including not breaking just after install.

One thing I'm not sure is default log verbosity. At least connected and waiting for new client messages do not add much.

marmarek · 2026-06-16T17:56:50Z

+    }
+
+    if (allowed_group == NULL) {
+        pam_syslog(pamh, LOG_ERR, "missing 'group' option");


I'm not sure if it's really necessary to require it. Do you think misconfiguration (like typo in the option name) is too big of a risk?

marmarek · 2026-06-16T17:56:59Z

+    }
+
+    if (allowed_services == NULL) {
+        pam_syslog(pamh, LOG_ERR, "missing 'services' option");


And similar question here

marmarek requested changes Oct 14, 2025

View reviewed changes

HW42 force-pushed the simon/admin-authz branch 2 times, most recently from e7b9577 to b7decf6 Compare October 14, 2025 18:58

HW42 mentioned this pull request Oct 14, 2025

Automate vm sudo authorization setup QubesOS/qubes-issues#2695

Open

DemiMarie reviewed Oct 15, 2025

View reviewed changes

ArrayBolt3 reviewed Oct 16, 2025

View reviewed changes

Implement optional admin authorization via qrexec

09e6885

See included README for details. Issue: QubesOS/qubes-issues#2695

HW42 force-pushed the simon/admin-authz branch from b7decf6 to 09e6885 Compare June 11, 2026 14:19

HW42 mentioned this pull request Jun 11, 2026

WIP: In-qube admin authorization helpers QubesOS/qubes-core-admin#828

Draft

marmarek reviewed Jun 16, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

HW42 commented Oct 14, 2025

Uh oh!

marmarek left a comment

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!

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!

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!

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!

HW42 commented Oct 14, 2025

Uh oh!

DemiMarie left a comment

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!

Uh oh!

marmarek commented Oct 15, 2025

Uh oh!

ArrayBolt3 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrelanos commented Oct 18, 2025

Uh oh!

HW42 commented Jun 11, 2026

Uh oh!

HW42 commented Jun 11, 2026

Uh oh!

marmarek commented Jun 11, 2026

Uh oh!

HW42 commented Jun 12, 2026

Uh oh!

ArrayBolt3 left a comment •

edited

Loading