Re: CVE request: CUPS DoS via RSS subscriptions

[Michael Sweet <mike () easysw com>]

Eygene Ryabinkin wrote:
> Josh, Mike, *, good day.
>
> Wed, Nov 19, 2008 at 03:14:43PM -0500, Josh Bressers wrote:
> > So from looking at cups 1.3.7 on Fedora 8, here is what I see:
> >
> > (gdb) bt
> > #0  create_subscription (con=0xb88975c0, uri=0xb889ae00) at ipp.c:5858
> > #1  0xb7facba7 in cupsdProcessIPPRequest (con=0xb88975c0) at ipp.c:615
> > #2  0xb7f88bfc in cupsdReadClient (con=0xb88975c0) at client.c:2253
> > #3  0xb7fc0606 in cupsdDoSelect (timeout=1) at select.c:537
> > #4  0xb7f98710 in main (argc=1, argv=0xbfdd6194) at main.c:817
> > (gdb) list
> > 5853        else if (printer)
> > 5854          cupsdLogMessage(CUPSD_LOG_DEBUG,
> > 5855                          "Added subscription %d for printer \"%s\"",
> > 5856                          sub->id, printer->name);
> > 5857        else
> > 5858          cupsdLogMessage(CUPSD_LOG_DEBUG, "Added subscription %d for server",
> > 5859                          sub->id);
> > 5860
> > 5861        sub->interval = interval;
> > 5862        sub->lease    = lease;
> > (gdb) print sub
> > $1 = (cupsd_subscription_t *) 0x0
> >
> > It would appear to be a NULL pointer dereference.  It seems that this call a
> > few lines above the snippet shown above:
> >  sub = cupsdAddSubscription(mask, printer, job, recipient, 0);
> >
> > will return NULL when the hardcoded value of 100 subscriptions is hit.
>
> Not really hardcoded -- it is settable with the 'MaxSubscriptions'
> directive.  I had just reproduced the bug with CUPS 1.3.9 at FreeBSD.
> MaxSubscriptions was set to 3 to ease the PoC.  Just repeated
> invocations of 'lpr -m <somefile>' were crashing cups daemon
> reproducibly.
>
> The attached patch fixes the things for me, but perhaps it needs
> some more polishing.  Will try to take a fresh look at this tomorrow.
>
> Mike, please, take a look at this!

You'll find a much more complete patch already in CUPS svn for both
1.3.x and 1.4.x, along with a new subscription test for the
"make check" target.  I didn't withhold the patch since the browser
attack vector was closed in 1.3.8...

I've attached my 1.3.x patch...

--
______________________________________________________________________
Michael Sweet, Easy Software Products           mike at easysw dot com

Index: test/run-stp-tests.sh
===================================================================
--- test/run-stp-tests.sh       (revision 8145)
+++ test/run-stp-tests.sh       (revision 8146)
@@ -307,6 +307,7 @@
 DocumentRoot $root/doc
 RequestRoot /tmp/cups-$user/spool
 TempDir /tmp/cups-$user/spool/temp
+MaxSubscriptions 3
 MaxLogSize 0
 AccessLog /tmp/cups-$user/log/access_log
 ErrorLog /tmp/cups-$user/log/error_log
Index: test/4.4-subscription-ops.test
===================================================================
--- test/4.4-subscription-ops.test      (revision 8145)
+++ test/4.4-subscription-ops.test      (revision 8146)
@@ -116,7 +116,33 @@
        EXPECT notify-events
        DISPLAY notify-events
 }
+{
+       # The name of the test...
+       NAME "Check MaxSubscriptions limits"
 
+       # The operation to use
+       OPERATION Create-Printer-Subscription
+       RESOURCE /
+
+       # The attributes to send
+       GROUP operation
+       ATTR charset attributes-charset utf-8
+       ATTR language attributes-natural-language en
+       ATTR uri printer-uri $method://$hostname:$port/printers/Test1
+
+        GROUP subscription
+       ATTR uri notify-recipient-uri testnotify://
+       ATTR keyword notify-events printer-state-changed
+       ATTR integer notify-lease-duration 5
+
+       # What statuses are OK?
+       STATUS client-error-too-many-subscriptions
+
+       # What attributes do we expect?
+       EXPECT attributes-charset
+       EXPECT attributes-natural-language
+}
+
 #
 # End of "$Id$"
 #
Index: scheduler/subscriptions.c
===================================================================
--- scheduler/subscriptions.c   (revision 8145)
+++ scheduler/subscriptions.c   (revision 8146)
@@ -341,9 +341,55 @@
   * Limit the number of subscriptions...
   */
 
-  if (cupsArrayCount(Subscriptions) >= MaxSubscriptions)
+  if (MaxSubscriptions > 0 && cupsArrayCount(Subscriptions) >= MaxSubscriptions)
+  {
+    cupsdLogMessage(CUPSD_LOG_DEBUG,
+                    "cupsdAddSubscription: Reached MaxSubscriptions %d",
+                   MaxSubscriptions);
     return (NULL);
+  }
 
+  if (MaxSubscriptionsPerJob > 0 && job)
+  {
+    int        count;                          /* Number of job subscriptions */
+
+    for (temp = (cupsd_subscription_t *)cupsArrayFirst(Subscriptions),
+             count = 0;
+         temp;
+        temp = (cupsd_subscription_t *)cupsArrayNext(Subscriptions))
+      if (temp->job == job)
+        count ++;
+
+    if (count >= MaxSubscriptionsPerJob)
+    {
+      cupsdLogMessage(CUPSD_LOG_DEBUG,
+                     "cupsdAddSubscription: Reached MaxSubscriptionsPerJob %d "
+                     "for job #%d", MaxSubscriptionsPerJob, job->id);
+      return (NULL);
+    }
+  }
+
+  if (MaxSubscriptionsPerPrinter > 0 && dest)
+  {
+    int        count;                          /* Number of printer subscriptions */
+
+    for (temp = (cupsd_subscription_t *)cupsArrayFirst(Subscriptions),
+             count = 0;
+         temp;
+        temp = (cupsd_subscription_t *)cupsArrayNext(Subscriptions))
+      if (temp->dest == dest)
+        count ++;
+
+    if (count >= MaxSubscriptionsPerPrinter)
+    {
+      cupsdLogMessage(CUPSD_LOG_DEBUG,
+                     "cupsdAddSubscription: Reached "
+                     "MaxSubscriptionsPerPrinter %d for %s",
+                     MaxSubscriptionsPerPrinter, dest->name);
+      return (NULL);
+    }
+  }
+
  /*
   * Allocate memory for this subscription...
   */
@@ -758,7 +804,6 @@
       cupsdLogMessage(CUPSD_LOG_ERROR,
                       "Syntax error on line %d of subscriptions.conf.",
                      linenum);
-      break;
     }
     else if (!strcasecmp(line, "Events"))
     {
Index: scheduler/ipp.c
===================================================================
--- scheduler/ipp.c     (revision 8145)
+++ scheduler/ipp.c     (revision 8146)
@@ -2119,24 +2119,25 @@
     if (mask == CUPSD_EVENT_NONE)
       mask = CUPSD_EVENT_JOB_COMPLETED;
 
-    sub = cupsdAddSubscription(mask, cupsdFindDest(job->dest), job, recipient,
-                               0);
+    if ((sub = cupsdAddSubscription(mask, cupsdFindDest(job->dest), job,
+                                    recipient, 0)) != NULL)
+    {
+      sub->interval = interval;
 
-    sub->interval = interval;
+      cupsdSetString(&sub->owner, job->username);
 
-    cupsdSetString(&sub->owner, job->username);
+      if (user_data)
+      {
+       sub->user_data_len = user_data->values[0].unknown.length;
+       memcpy(sub->user_data, user_data->values[0].unknown.data,
+              sub->user_data_len);
+      }
 
-    if (user_data)
-    {
-      sub->user_data_len = user_data->values[0].unknown.length;
-      memcpy(sub->user_data, user_data->values[0].unknown.data,
-             sub->user_data_len);
+      ippAddSeparator(con->response);
+      ippAddInteger(con->response, IPP_TAG_SUBSCRIPTION, IPP_TAG_INTEGER,
+                   "notify-subscription-id", sub->id);
     }
 
-    ippAddSeparator(con->response);
-    ippAddInteger(con->response, IPP_TAG_SUBSCRIPTION, IPP_TAG_INTEGER,
-                  "notify-subscription-id", sub->id);
-
     if (attr)
       attr = attr->next;
   }
@@ -5590,7 +5591,12 @@
     else
       job = NULL;
 
-    sub = cupsdAddSubscription(mask, printer, job, recipient, 0);
+    if ((sub = cupsdAddSubscription(mask, printer, job, recipient, 0)) == NULL)
+    {
+      send_ipp_status(con, IPP_TOO_MANY_SUBSCRIPTIONS,
+                     _("There are too many subscriptions."));
+      return;
+    }
 
     if (job)
       cupsdLogMessage(CUPSD_LOG_DEBUG, "Added subscription %d for job %d",