Bug 129001 - [vuxml] [patch] print/cups-base: fix NULL-pointer dereference
Summary: [vuxml] [patch] print/cups-base: fix NULL-pointer dereference
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: Dirk Meyer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-20 00:40 UTC by Eygene Ryabinkin
Modified: 2008-12-25 16:10 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eygene Ryabinkin 2008-11-20 00:40:01 UTC
It was discovered [1] that CUPS up to 1.3.9 has code path that will
dereference NULL pointer and it is trivially reproducible when user hits
the subscription limit, for example via repeated commands 'lpr -m
<somefile>'.

[1] http://www.openwall.com/lists/oss-security/2008/11/19/4/ and
    the rest of the thread.

Fix: There is no official fix yet -- I had just informed CUPS developer and
posted the simple patch to the oss-security mailing list.  Here is the
patch that will introduce checks for the values returned by
cupsdAddSubscription() and bump port version:



The preliminary VuXML entry follows:
  <vuln vid="unknown">
    <topic>cups -- Denial of Service by authenticated client</topic>
    <affects>
      <package>
	<name>cups-base</name>
	<range><lt>1.3.9_1</lt></range>
      </package>
    </affects>
    <description>
      <body xmlns="http://www.w3.org/1999/xhtml">
	<p>Josh Bressers discovered that CUPS daemon can be crashed
	via trivial NULL-pointer dereference:</p>
	<blockquote cite="http://www.openwall.com/lists/oss-security/2008/11/19/4/">
	<p>The upstream fix could still obviously let a local
	authenticated user crash the server.</p>
	</blockquote>
      </body>
    </description>
    <references>
      <mlist>http://www.openwall.com/lists/oss-security/2008/11/19/4/</mlist>
    </references>
    <dates>
      <discovery>2008-11-19</discovery>
    </dates>
  </vuln>
--- vuln.xml ends here ---

Please, note that this vulnerability was already disclosed in the
oss-security mailing list, so there is no much sense in hiding this
discussion.--Bt6MZmTlqT1FYuYB5tCxiG3xDkwBTObRlsr48eoUEfzajFye
Content-Type: text/plain; name="1.3.9-to-1.3.9_1-fix-null-deference.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="1.3.9-to-1.3.9_1-fix-null-deference.patch"

diff -urN ./Makefile ../cups-base/Makefile
--- ./Makefile	2008-11-20 02:48:10.000000000 +0300
+++ ../cups-base/Makefile	2008-11-20 03:07:03.000000000 +0300
@@ -7,6 +7,7 @@
 
 PORTNAME=	cups
 PORTVERSION=	1.3.9
+PORTREVISION=	1
 DISTVERSIONSUFFIX=	-source
 CATEGORIES=	print
 MASTER_SITES=	EASYSW/${PORTNAME}/${DISTVERSION}
diff -urN ./files/patch-fix-subscriptions-null-dereference ../cups-base/files/patch-fix-subscriptions-null-dereference
--- ./files/patch-fix-subscriptions-null-dereference	1970-01-01 03:00:00.000000000 +0300
+++ ../cups-base/files/patch-fix-subscriptions-null-dereference	2008-11-20 03:11:26.000000000 +0300
@@ -0,0 +1,48 @@
+--- scheduler/subscriptions.c.orig	2008-11-20 02:57:17.000000000 +0300
++++ scheduler/subscriptions.c	2008-11-20 03:02:06.000000000 +0300
+@@ -728,6 +728,13 @@
+       {
+         sub = cupsdAddSubscription(CUPSD_EVENT_NONE, NULL, NULL, NULL,
+ 	                           atoi(value));
++	if (!sub)
++	{
++          cupsdLogMessage(CUPSD_LOG_ERROR,
++	                  "Unable to add new subscription.  Was parsing line %d of subscriptions.conf.",
++	                  linenum);
++          break;
++	}
+       }
+       else
+       {
+--- scheduler/ipp.c.orig	2008-11-20 02:55:59.000000000 +0300
++++ scheduler/ipp.c	2008-11-20 02:56:03.000000000 +0300
+@@ -2121,6 +2121,14 @@
+ 
+     sub = cupsdAddSubscription(mask, cupsdFindDest(job->dest), job, recipient,
+                                0);
++    if (!sub)
++    {
++      cupsdLogMessage(CUPSD_LOG_ERROR,
++		      "Failed to create subscription for job %d", job->id);
++      send_ipp_status(con, IPP_TOO_MANY_SUBSCRIPTIONS,
++                      _("Unable to add new subscription"));
++      return;
++    }
+ 
+     sub->interval = interval;
+ 
+@@ -5591,6 +5599,14 @@
+       job = NULL;
+ 
+     sub = cupsdAddSubscription(mask, printer, job, recipient, 0);
++    if (!sub)
++    {
++      cupsdLogMessage(CUPSD_LOG_ERROR,
++		      "Failed to create subscription for job %d", job->id);
++      send_ipp_status(con, IPP_TOO_MANY_SUBSCRIPTIONS,
++                      _("Unable to add new subscription"));
++      return;
++    }
+ 
+     if (job)
+       cupsdLogMessage(CUPSD_LOG_DEBUG, "Added subscription %d for job %d",
How-To-Repeat: 
Set 'MaxSubscriptions' in the cupsd.conf to some small value and invoke
'lpr -m <somefile>' multiple times.  You'll see that after some attempt
server will be unreachable due to its crash.  Default value of 100 for
MaxSubscription does not prevent the DoS, because many big files could
be feeded to CUPS daemon.
Comment 1 Edwin Groothuis freebsd_committer freebsd_triage 2008-11-20 00:40:17 UTC
Responsible Changed
From-To: freebsd-ports-bugs->dinoex

Over to maintainer (via the GNATS Auto Assign Tool)
Comment 2 Eygene Ryabinkin 2008-11-20 08:58:25 UTC
Me again.

Thu, Nov 20, 2008 at 03:36:00AM +0300, Eygene Ryabinkin wrote:
> It was discovered [1] that CUPS up to 1.3.9 has code path that will
> dereference NULL pointer and it is trivially reproducible when user hits
> the subscription limit, for example via repeated commands 'lpr -m
> <somefile>'.
> 
> [1] http://www.openwall.com/lists/oss-security/2008/11/19/4/ and
>     the rest of the thread.


Michael Sweet provided more complete patch [2] that is already in the
1.3.x Subversion repository.

[2] http://www.openwall.com/lists/oss-security/2008/11/20/2

Had tested the patch -- it works too.

Attaching modified port patch and reworked VuXML entry.

--- 1.3.9-to-1.3.9_1-fix-null-deference-upstream.patch begins here ---
diff -urN ./Makefile ../cups-base/Makefile
--- ./Makefile	2008-11-20 02:48:10.000000000 +0300
+++ ../cups-base/Makefile	2008-11-20 03:07:03.000000000 +0300
@@ -7,6 +7,7 @@
 
 PORTNAME=	cups
 PORTVERSION=	1.3.9
+PORTREVISION=	1
 DISTVERSIONSUFFIX=	-source
 CATEGORIES=	print
 MASTER_SITES=	EASYSW/${PORTNAME}/${DISTVERSION}
diff -urN ./files/patch-fix-subscriptions-null-dereference ../cups-base/files/patch-fix-subscriptions-null-dereference
--- ./files/patch-fix-subscriptions-null-dereference	1970-01-01 03:00:00.000000000 +0300
+++ ../cups-base/files/patch-fix-subscriptions-null-dereference	2008-11-20 11:33:59.000000000 +0300
@@ -0,0 +1,179 @@
+Obtained from: Michael Sweet, via oss-security list,
+  http://www.openwall.com/lists/oss-security/2008/11/20/2
+
+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",
--- 1.3.9-to-1.3.9_1-fix-null-deference-upstream.patch ends here ---

--- vuln.xml begins here ---
  <vuln vid="unknown">
    <topic>cups scheduler -- Denial of Service by authorized client</topic>
    <affects>
      <package>
	<name>cups-base</name>
	<range><lt>1.3.9_1</lt></range>
      </package>
    </affects>
    <description>
      <body xmlns="http://www.w3.org/1999/xhtml">
	<p>ChangeLog for CUPS 1.3.10 says:</p>
	<blockquote cite="http://svn.easysw.com/public/cups/trunk/CHANGES-1.3.txt">

	<p>The scheduler would crash if you exceeded the
	MaxSubscriptions limit.</p>
	</blockquote>
      </body>
    </description>
    <references>
      <url>http://svn.easysw.com/public/cups/trunk/CHANGES-1.3.txt</url>
      <mlist>http://www.openwall.com/lists/oss-security/2008/11/19/4/</mlist>
    </references>
    <dates>
      <discovery>2008-11-19</discovery>
    </dates>
  </vuln>
--- vuln.xml ends here ---
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual   
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook 
    {_.-``-'         {_/            #
Comment 3 Dirk Meyer freebsd_committer freebsd_triage 2008-11-21 18:45:23 UTC
State Changed
From-To: open->feedback


The patch do not apply. 

patch <1 
Hmm...  Looks like a unified diff to me... 
The text leading up to this was: 
-------------------------- 
|diff -urN ./Makefile ../cups-base/Makefile 
|--- ./Makefile 2008-11-20 02:48:10.000000000 +0300 
|+++ ../cups-base/Makefile      2008-11-20 03:07:03.000000000 +0300 
-------------------------- 
Patching file ./Makefile using Plan A... 
Hunk #1 failed at 7. 
1 out of 1 hunks failed--saving rejects to ./Makefile.rej 
Hmm...  The next patch looks like a unified diff to me... 
The text leading up to this was: 
-------------------------- 
|diff -urN ./files/patch-fix-subscriptions-null-dereference ../cups-base/fil= 
|es/patch-fix-subscriptions-null-dereference 
|--- ./files/patch-fix-subscriptions-null-dereference   1970-01-01 03:00:00.00= 
|0000000 +0300 
|+++ ../cups-base/files/patch-fix-subscriptions-null-dereference        2008-11-20 = 
|11:33:59.000000000 +0300 
-------------------------- 
(Creating file ./files/patch-fix-subscriptions-null-dereference...) 
Patching file ./files/patch-fix-subscriptions-null-dereference using Plan A... 
patch: **** malformed patch at line 24: =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= 
dm@home3:/usr/ports/current/cups-base$ ls -tlr 
total 430
Comment 4 dfilter service freebsd_committer freebsd_triage 2008-11-28 07:33:08 UTC
dinoex      2008-11-28 07:32:56 UTC

  FreeBSD ports repository

  Modified files:
    print/cups-base      Makefile 
  Added files:
    print/cups-base/files patch-maxsubscriptions 
  Log:
  - Fix NULL pointer dereference in MaxSubscription
  Security: http://www.openwall.com/lists/oss-security/2008/11/19/4/
  Security: http://www.openwall.com/lists/oss-security/2008/11/20/2
  Obtained from:  Michael Sweet
  PR:             129001
  
  Revision  Changes    Path
  1.104     +1 -0      ports/print/cups-base/Makefile
  1.1       +179 -0    ports/print/cups-base/files/patch-maxsubscriptions (new)
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
Comment 5 Dirk Meyer freebsd_committer freebsd_triage 2008-11-28 08:11:46 UTC
State Changed
From-To: feedback->closed

The patch was mangled again. 
committed, thanks.
Comment 6 Eygene Ryabinkin 2008-11-28 11:43:16 UTC
Dirk, good day.

Fri, Nov 28, 2008 at 09:12:47AM +0100, dinoex@FreeBSD.org wrote:
> Synopsis: [vuxml] [patch] print/cups-base: fix NULL-pointer dereference
> 
> State-Changed-From-To: feedback->closed
> State-Changed-By: dinoex
> State-Changed-When: Fri Nov 28 09:11:46 CET 2008
> State-Changed-Why: 
> The patch was mangled again.


In the interface that is provided by query-pr.cgi -- yes.  But I had
sent it to you directly.  Was it mangled too?

> committed, thanks.


Thank you.  Again, what about VuXML entry?
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual   
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook 
    {_.-``-'         {_/            #
Comment 7 Eygene Ryabinkin 2008-12-25 16:05:12 UTC
Gentlemen!

What about the VuXML entry for this -- is it going to be added?
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #