Interface request: "cups-control" on CUPS snap and including D-Bus

I’m not at all convinced that this is true. Either implementation is going to be doing roughly the same thing:

  1. do an SO_PEERCRED lookup on the socket to determine the client uid and pid.
  2. gather more information about that client from /proc/$pid
  3. for each request the client makes on the connection, check to see if it is privileged.
  4. for privileged operations, issue a request to a second daemon to check the client’s authorisation.
  5. when the second daemon’s response comes back, either serve the client’s request or return an error.

I’ve got a pretty full plate at the moment, and you’ve got a lot more experience with the CUPS source code than me. I’m happy to give you some pointers though.

That part of the code, sure, but the problem is with polkit, the APIs need to be carved up into polkit chunks that may or may not align with the current group checks. Even if they are, there is generating the xml and deciding on how the default policy should be written. This could be shortened to reimplement the group membership, but then the patch didn’t help our snap mediation needs.

@jamesh, @jdstrand, I have started the CUPS patch now (see below, is there really no way to add attachments in this forum?). I have found out that all authentication is done at a central place in cupsd, in the cupsdIsAuthorized() function in the file scheduler/auth.c.
To define which of the many IPP operations which CUPS supports are administrative operations (the ones which should only be allowed through the “cups-control” interface and not through the “cups” interface) I have taken the ones which are only allowed by the pseudo-group @SYSTEM in the policies in /etc/cups/cupsd.conf. This way I can simply check in the cupsdIsAuthorized() function whether the authorization is done through @SYSTEM and if so, call an extra function, which I have called cupsdCheckAdminTask() and only if this one tests positive, allow the operation.
In cupsdCheckAdminTask() I check whether the client connects via domain socket (network address family AF_LOCAL) and if not, the function simply passes. If the client connects through a domain socket I poll its peer credentials and so get the PID of the client process. After the line

/* Examine client process here */

one only needs to insert the checking for whether this process is from a Snap and whether this Snap plugs with “cups-control”.

Till

diff --git a/scheduler/auth.c b/scheduler/auth.c
index 4fbad6e24..466a0e529 100644
--- a/scheduler/auth.c
+++ b/scheduler/auth.c
@@ -43,6 +43,7 @@
 #  include <sys/ucred.h>
 typedef struct xucred cupsd_ucred_t;
 #  define CUPSD_UCRED_UID(c) (c).cr_uid
+#  define CUPSD_UCRED_PID(c) (c).cr_pid
 #else
 #  ifndef __OpenBSD__
 typedef struct ucred cupsd_ucred_t;
@@ -50,6 +51,7 @@ typedef struct ucred cupsd_ucred_t;
 typedef struct sockpeercred cupsd_ucred_t;
 #  endif
 #  define CUPSD_UCRED_UID(c) (c).uid
+#  define CUPSD_UCRED_PID(c) (c).pid
 #endif /* HAVE_SYS_UCRED_H */
 
 
@@ -1520,6 +1522,61 @@ cupsdFreeLocation(cupsd_location_t *loc)/* I - Location to free */
 }
 
 
+/*
+ * 'cupsdCheckAdminTask()' - Do additional checks on administrative tasks
+ */
+
+int                                      /* O - 1 if admin task authorized */
+cupsdCheckAdminTask(cupsd_client_t *con) /* I - Connection */
+{
+  cupsdLogMessage(CUPSD_LOG_DEBUG,
+		  "cupsdCheckAdminTask: ADMINISTRATIVE TASK!!");
+
+#if defined(SO_PEERCRED) && defined(AF_LOCAL)
+ /*
+  * Get the client's PID if it accesses locally via domain socket
+  */
+
+  if (httpAddrFamily(con->http->hostaddr) == AF_LOCAL)
+  {
+    cupsd_ucred_t	peercred;	/* Peer credentials */
+    socklen_t		peersize;	/* Size of peer credentials */
+    int                 client_pid;     /* PID of client */
+
+    peersize = sizeof(peercred);
+
+#  ifdef __APPLE__
+    if (getsockopt(httpGetFd(con->http), 0, LOCAL_PEERCRED, &peercred,
+		   &peersize))
+#  else
+    if (getsockopt(httpGetFd(con->http), SOL_SOCKET, SO_PEERCRED, &peercred,
+		   &peersize))
+#  endif /* __APPLE__ */
+    {
+      cupsdLogMessage(CUPSD_LOG_ERROR,
+		      "cupsdCheckAdminTask: Unable to get peer credentials - %s",
+		      strerror(errno));
+    }
+    else
+    {
+      cupsdLogMessage(CUPSD_LOG_DEBUG,
+		      "cupsdCheckAdminTask: Client UID %d PID %d",
+		      CUPSD_UCRED_UID(peercred),
+		      CUPSD_UCRED_PID(peercred));
+      client_pid = CUPSD_UCRED_PID(peercred);
+
+      /* Examine client process here */
+      cupsdLogMessage(CUPSD_LOG_DEBUG,
+		      "cupsdCheckAdminTask: Examining process %d ...",
+		      client_pid);
+    }
+  }
+#endif /* SO_PEERCRED && AF_LOCAL */
+
+  return 1;
+}
+
+
 /*
  * 'cupsdIsAuthorized()' - Check to see if the user is authorized...
  */
@@ -1714,12 +1771,9 @@ cupsdIsAuthorized(cupsd_client_t *con,	/* I - Connection */
 
  /*
   * OK, got a username.  See if we need normal user access, or group
-  * access... (root always matches)
+  * access...
   */
 
-  if (!strcmp(username, "root"))
-    return (HTTP_OK);
-
  /*
   * Strip any @domain or @KDC from the username and owner...
   */
@@ -1749,6 +1803,21 @@ cupsdIsAuthorized(cupsd_client_t *con,	/* I - Connection */
   else
     pw = NULL;
 
+ /*
+  * For matching user and group memberships below we will first go
+  * through all names except @SYSTEM to authorize the task as
+  * non-administrative, like printing or deleting one's own job, if this
+  * fails we will check whether we can authorize via the special name
+  * @SYSTEM, as an administrative task, like creating a print queue or
+  * deleting someone else's job.
+  * Note that tasks are considered as administrative by the policies
+  * in cupsd.conf, when they require the user or group @SYSTEM.
+  * We do this separation because if the client is a Snap connecting via
+  * domain socket, we need to additionally check whether it plugs to us
+  * through the "cups-control" interface which allows administration and
+  * not through the "cups" interface which allows only printing.
+  */
+
   if (best->level == CUPSD_AUTH_USER)
   {
    /*
@@ -1779,8 +1848,15 @@ cupsdIsAuthorized(cupsd_client_t *con,	/* I - Connection */
       {
 	if (!_cups_strncasecmp(name, "@AUTHKEY(", 9) && check_authref(con, name + 9))
 	  return (HTTP_OK);
-	else if (!_cups_strcasecmp(name, "@SYSTEM") && SystemGroupAuthKey &&
-		 check_authref(con, SystemGroupAuthKey))
+      }
+
+      for (name = (char *)cupsArrayFirst(best->names);
+           name;
+	   name = (char *)cupsArrayNext(best->names))
+      {
+	if (!_cups_strcasecmp(name, "@SYSTEM") && SystemGroupAuthKey &&
+	    check_authref(con, SystemGroupAuthKey) &&
+	    cupsdCheckAdminTask(con))
 	  return (HTTP_OK);
       }
 
@@ -1797,9 +1873,8 @@ cupsdIsAuthorized(cupsd_client_t *con,	/* I - Connection */
 	return (HTTP_OK);
       else if (!_cups_strcasecmp(name, "@SYSTEM"))
       {
-        for (i = 0; i < NumSystemGroups; i ++)
-	  if (cupsdCheckGroup(username, pw, SystemGroups[i]))
-	    return (HTTP_OK);
+	/* Do @SYSTEM later, when every other entry fails */
+	continue;
       }
       else if (name[0] == '@')
       {
@@ -1810,6 +1885,19 @@ cupsdIsAuthorized(cupsd_client_t *con,	/* I - Connection */
         return (HTTP_OK);
     }
 
+    for (name = (char *)cupsArrayFirst(best->names);
+	 name;
+	 name = (char *)cupsArrayNext(best->names))
+    {
+      if (!_cups_strcasecmp(name, "@SYSTEM"))
+      {
+        for (i = 0; i < NumSystemGroups; i ++)
+	  if (cupsdCheckGroup(username, pw, SystemGroups[i]) &&
+	      cupsdCheckAdminTask(con))
+	    return (HTTP_OK);
+      }
+    }
+
     return (con->username[0] ? HTTP_FORBIDDEN : HTTP_UNAUTHORIZED);
   }
 
@@ -1827,16 +1915,31 @@ cupsdIsAuthorized(cupsd_client_t *con,	/* I - Connection */
        name;
        name = (char *)cupsArrayNext(best->names))
   {
+    if (!_cups_strcasecmp(name, "@SYSTEM"))
+    {
+      /* Do @SYSTEM later, when every other entry fails */
+      continue;
+    }
+
     cupsdLogMessage(CUPSD_LOG_DEBUG2, "cupsdIsAuthorized: Checking group \"%s\" membership...", name);
 
+    if (cupsdCheckGroup(username, pw, name))
+      return (HTTP_OK);
+  }
+
+  for (name = (char *)cupsArrayFirst(best->names);
+       name;
+       name = (char *)cupsArrayNext(best->names))
+  {
     if (!_cups_strcasecmp(name, "@SYSTEM"))
     {
+      cupsdLogMessage(CUPSD_LOG_DEBUG2, "cupsdIsAuthorized: Checking group \"%s\" membership...", name);
+
       for (i = 0; i < NumSystemGroups; i ++)
-	if (cupsdCheckGroup(username, pw, SystemGroups[i]))
+	if (cupsdCheckGroup(username, pw, SystemGroups[i]) &&
+	    cupsdCheckAdminTask(con))
 	  return (HTTP_OK);
     }
-    else if (cupsdCheckGroup(username, pw, name))
-      return (HTTP_OK);
   }
 
  /*

Here is the CUPS patch development in progress:

The patch is the sum of the newest commits, currently only this commit which lets administrative operation requests from Snaps simply get denied.

1 Like

And here is a mini Snap for testing. Create a directory and put this snapcraft.yaml inside:

name: cups-admin-test
base: core18 # The base snap is the execution environment for this snap
version: 0.1.0
summary: CUPS admin and non-admin tasks out of a Snap 
description: Testing interfaces for the CUPS Snap
grade: devel
confinement: strict

apps:
  lpinfo:
    command: lpinfo
    plugs: [network, avahi-control, raw-usb, cups-control]
  lpadmin:
    command: lpadmin
    plugs: [network, avahi-control, home, cups-control]
  lpstat:
    command: lpstat
    plugs: [network, avahi-control, cups-control]
  lpoptions:
    command: lpoptions
    plugs: [network, avahi-control, home, cups-control]
  lp:
    command: lp
    plugs: [network, avahi-control, home, cups-control]
  cancel:
    command: cancel
    plugs: [network, avahi-control, cups-control]
  lpmove:
    command: lpmove
    plugs: [network, avahi-control, cups-control]
  cupsenable:
    command: cupsenable
    plugs: [network, avahi-control, cups-control]
  cupsdisable:
    command: cupsdisable
    plugs: [network, avahi-control, cups-control]
  cupsaccept:
    command: cupsaccept
    plugs: [network, avahi-control, cups-control]
  cupsreject:
    command: cupsreject
    plugs: [network, avahi-control, cups-control]
  accept:
    command: accept
    plugs: [network, avahi-control, cups-control]
  reject:
    command: reject
    plugs: [network, avahi-control, cups-control]
  cupsctl:
    command: cupsctl
    plugs: [network, avahi-control, cups-control]

parts:
  cups-client:
    plugin: dump
    source: .
    stage-packages:
        - cups-client
        - libcups2
    prime:
        - usr/bin/*
        - usr/sbin/*
        - usr/lib/*

In this directory run:

snapcraft snap
sudo snap install --dangerous cups-admin-test_0.1.0_amd64.snap
sudo snap connect cups-admin-test:cups-control

This snap provides the command line tools of CUPS in a Snap, so you can run administrative tasks as follows:

cups-admin-test.lpadmin -p authtest -E -v /dev/null &
cups-admin-test.lpadmin -x authtest &

The & at the end makes the PID of the commands appear on the screen, so that you can compare in the CUPS error_log.
Non-administrative tasks are:

cups-admin-test.lpstat -v &
cups-admin-test.lp -d authtest ~/.bashrc &

If you try the patch on the system’s non-Snapped CUPS, run

sudo aa-complain cupsd

as libapparmor checks for the AppArmor profile reading /proc/PID/attr/current.

Also set LogLevel debug2 in cupsd.conf and search for lines containing cupsdCheckAdminTask: in /var/log/cups/error_log.

Quick way to test the patch with the system’s CUPS installation:

git clone git@github.com:tillkamppeter/cups.git
cd cups
autoconf
./configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
make
sudo aa-complain cupsd
sudo systemctl stop cups
sudo cp scheduler/cupsd /usr/sbin/
sudo systemctl start cups

Then run test commands, out of the Snap and from the system and see what works and what not.

Till

3 Likes

Now I have done the second part of the patch, actually granting permission based on the fact whether the client connects via “cups-control”, meaning that the we have now functionality equivalent to PulseAudio.
In contrary to PulseAudio we use synchronous functions of the snapd-glib library to make the code simpler, and in CUPS it seems to work. According to error_log the whole check happens in well less than a second, and when entering client commands one gets instant answers. Probably PulseAudio uses the asynchronous versions to assure continuity of sound playing (am I right @jamesh?).
The work still needed is improving the error handling, as any error on
the way makes the operation be allowed, destroying the connection to
snapd when done, to avoid memory leaks, and some code polishing.
@jdstrand, @jamesh, could you have a look into this, whether it is what we need and does not have some security problems which I perhaps have not seen?

Till

1 Like

Pulse Audio uses a single threaded event driven daemon, so direct calls to the snapd-glib synchronous API would have been a problem. Complicating matters, it doesn’t use the GLib main loop, so the async API couldn’t be called directly either. I could have performed synchronous calls in a thread pool, but opted to use the async API in another thread running the GLib main loop.

If cupsd is built on a “one connection per thread” model, then snapd-glib’s synchronous API is a reasonable choice.

In snapcraft.yaml I can define for each contained app individually, which interfaces it uses, so in my little test Snap I could let the lpadmin app use “cups-control”, but the lpstat app only use “cups” (assuming that our new CUPS interfaces are implemented in snapd), as the latter is supposed to never do administrative tasks.
Now when cupsd knows the PID of a client it can find out that it is from a Snap and from which Snap via libapparmor, and it could also find out the executable name from /proc/PID/comm. So with the executable name one can find out which app from the Snap was used.
But what I did not find out is can I poll vis snapd-glib which interfaces this app is plugging?
For now I can only determine which interfaces the whole Snap is plugging and so my patch grants admin access to inquiries from any app of the Snap, even if lpstat is used and it is only plugging “cups” and not “cups-control”.
@jdstrand, @ijohnson, @jamesh, does the snapd-glib API support this?

Correction: The name the app within the client Snap cupsd could also determine from the AppArmor profile name. The last part after the second dot at the end of the Snap name is the app name.

Completed the CUPS patch with two more commits: One to free data structures from snapd-glib after use and another to improve the error handling and the log messages.
Now errors in communication with snapd lead to a denial of access.
Note that I did not find a solution for my question in the previous posts here, so the patch only checks whether the client Snap plugs “cups-control” and not whether the individual app of the Snap plugs it.
My next step is to apply this patch to the CUPS snap. Then I will also rename the CUPS Snap.
@ijohnson, @jdstrand, so I think that now you could do the changes on the CUPS interfaces, to have “cups” for user tasks and “cups-control” for administrative tasks.

@till.kamppeter - I’ve added this to the list for 2.45 and I’m going to try to get to it this week. If you find yourself wanting to demo it before the snapd changes are committed, you can always install the snap with --devmode; it should still mediate clients connecting to it.

@jdstrand, thanks, is there some documentation about the specification of the new “cups” and “cups-control” interfaces? Can you also post a link to the commits/changes you have done on the snapd code to implement the new interfaces? Does installing snapd with “–devmode” automatically select the development snapshot with these interfaces?

I think Jamie was suggesting you install your ipp-printing snap in devmode, not the snapd snap. I don’t think anything has been added to snapd yet, and so in the meantime installing your snap in devmode will get around any denials your snap might run into, and allow you to test the patches to cups mediation.

1 Like

@ijohnson is exactly right. Sorry if I wasn’t clear.

Patch is now committed to the CUPS Snap.

Till

@jdstrand, @ijohnson, libapparmor used in the cupsd patch needs to access /proc/PID/attr/current. Is “system-observe” the correct interface for that?
@jamesh, how is PulseAudio doing that?

This and other accesses will need to be in the updates to the cups-control interface that I will make. For now, please use --devmode when installing your snap. I’ll post something here when there is something to test.

As for pulseaudio, the pulseaudio snap does not have the mediation patches and so the existing pulseaudio slot policy is sufficient. That policy will need updating if/when the pulseaudio snap is updated with the mediation patches.

@jdstrand do you think the cups-control interface should use an attribute that only the cups snap gets to use to get this access since it is somewhat privileged and we may not necessarily want to give it out to any snaps that already are using cups-control ?

We’ll ensure the base implementation is such that cups-control operates like other slot implementations like it so that declaring slots: [ cups-control ] gets flagged for manual review by the store. This is a common pattern within the base declaration and snap declaration for slot implementations.

@jdstrand, @ijohnson, @jamesh, I have done another commit to the printing-stack-snap repo now to assure that the Snap works with the new CUPS patch and also to make sure that if the Snap is started with no other CUPS (from .deb for example) running, that the Snap’s CUPS uses the standard domain socket (/ver)/run/cups/cups.sock and port 631.
With this you should be able to do all tests for the interface changes in snapd.
Please read also the description of my commit, to see what are all the things I changed.

2 Likes