[redhat-lspp] cups uds and lspp patches

[Steve Grubb]

On Wednesday 15 March 2006 17:55, Matt Anderson wrote:
> Attached are patches and a spec file for cups to add support for meeting
> the lspp requirements along with a back port of the unix domain sockets
> from the cups 1.2 subversion tree.

In cups-uds.patch

httpAddrString
+#ifdef AF_LOCAL
+  if (addr->addr.sa_family == AF_LOCAL)
+    strlcpy(s, addr->un.sun_path, slen);
+  else
+#endif /* AF_LOCAL */

Shouldn't the length be based on the buffer's size? Potential for overflow 
here.

+    char        servname[1024];         /* Service name (not used) */

Magic number...sb NI_MAXSERV

+    if (getnameinfo(&addr->addr, httpAddrLength(addr), s, slen,
+                    servname, sizeof(servname), NI_NUMERICHOST

Should length be based on buffer size? Passed values are dangerous.

httpAddrLookup
+#ifdef AF_LOCAL
+  if (addr->addr.sa_family == AF_LOCAL)
+    strlcpy(name, addr->un.sun_path, namelen);
+  else
+#endif /* AF_LOCAL */

Same

+    char        servname[1024];                 /* Service name (not used) */
+
+
+    if (getnameinfo(&addr->addr, httpAddrLength(addr), name, namelen,
+                    servname, sizeof(servname), 0))

Same as above

@@ -2090,7 +2098,7 @@
 get_address(char               *value,         /* I - Value string */
             unsigned           defaddress,     /* I - Default address */
            int                defport,         /* I - Default port */
-            struct sockaddr_in *address)       /* O - Socket address */
+            http_addr_t        *address)       /* O - Socket address */
 {
   char                 hostname[256],          /* Hostname or IP */
                        portname[256];          /* Port number or name */
@@ -2102,10 +2110,22 @@
   * Initialize the socket address to the defaults...
   */

-  memset(address, 0, sizeof(struct sockaddr_in));
-  address->sin_family      = AF_INET;
-  address->sin_addr.s_addr = htonl(defaddress);
-  address->sin_port        = htons(defport);
+  memset(address, 0, sizeof(struct sockaddr));

address appears to be http_addr_t, sb sizeof(http_addr_t) ?

+
+  if (value[0] == '/')
+  {
+     address->addr.sa_family = AF_LOCAL;
+
+     if (strlen(value) > 107)
+       return (0);
+
+     strncpy(address->un.sun_path, (const char*)value, (size_t)108);
+     return (1);
+  }

magic numbers...sb UNIX_PATH_MAX

General comment, sprintf is being used. snprintf is preferable. strtol is not 
being used safely. You have to set errno to 0, use strtol, then check if 
errno != 0.

------------------------------------------------------------------

In cups-lspp.patch:

+            AC_CHECK_LIB(audit,audit_send_user_message, [LIBAUDIT="-laudit" 
AC_SUBST(LIBAUDIT)])
+            AC_CHECK_HEADER(libaudit.h)

You are checking for a deprecated function. sb audit_log_user_message.


+    if (getsockopt(con->http.fd, SOL_SOCKET, SO_PEERCRED, &cr, &cl)==0) {
+     /*
+      * get_client_auid() can be racey
+      * In this case the pid is based on a socket connected to the client
+      */
+      con->auid = get_client_auid(cr.pid);
+      LogMessage(L_INFO, "AcceptClient: peer's pid=%d, uid=%d, gid=%d, 
auid=%d",
+                 cr.pid, cr.uid, cr.gid, con->auid);
+    } else 

Are you able to detect the socket being disconnected? I think you can setup a 
sigpipe handler to detect this. Then again, I think labled networking was 
supposed to have the peer's label. That may be the best approach going 
forward. You'd have to detect its support in the configure script and maybe 
drop back to this other technique in the event that cups is on a kernel that 
doesn't support labled networking. This can be made non-racey.

+uid_t get_client_auid(pid_t clipid)

Why set errno to 0 so far away ? If errno was EINTR and a retry was done, you 
need to reset errno before strtol or it will appear to have failed.

+  if (ClassifyOverride)
+    audit_log_user_message(AuditLog, AUDIT_USYS_CONFIG,

This does not fit the intended use of AUDIT_USYS_CONFIG. I think you want to 
use AUDIT_LABEL_OVERRIDE.

+
+#ifdef WITH_LSPP
+  AuditLog = audit_open();
+#endif /* WITH_LSPP */
+

What if AuditLog == -1 ? Should job abort? Which brings up a point...return 
code of audit_log_user_message is never being checked. If the audit message 
is being sent prior to the job being printed, the job should be aborted if 
the audit message cannot be sent.

-Steve