[Printing-architecture] [patch] cups-1.7.0: job history fix

[Printing-architecture] [patch] cups-1.7.0: job history fix

[Tim Waugh]

[-- Attachment #1.1: Type: text/plain, Size: 660 bytes --]

Hi,

While playing around with a logging enhancement¹ I discovered a bug in
the job history code. It was introduced some time after the public
source repository was taken off-line, so I don't know what the
motivation for the change was.

A cups_joblog_t declares 'char message[1]' for the message, and is
allocated with enough storage for the intended message. However, the
message is copied in with strlcpy, with a limit of
sizeof(cups_joblog_t->message). As a result, the message is severely(!)
truncated.

The fix is to undo the change as attached.

Tim.
*/

¹ http://cyberelk.net/tim/2013/10/25/cups-adding-support-for-system-journal/

[-- Attachment #1.2: cups-jobhistory.patch --]
[-- Type: text/x-patch, Size: 537 bytes --]

diff -up cups-1.7rc1/scheduler/log.c.orig cups-1.7rc1/scheduler/log.c
--- cups-1.7rc1/scheduler/log.c.orig	2013-10-24 15:40:42.412062412 +0100
+++ cups-1.7rc1/scheduler/log.c	2013-10-24 15:40:43.329066617 +0100
@@ -534,7 +534,7 @@ cupsdLogJob(cupsd_job_t *job,		/* I - Jo
       if ((temp = malloc(sizeof(cupsd_joblog_t) + strlen(log_line))) != NULL)
       {
         temp->time = time(NULL);
-	strlcpy(temp->message, log_line, sizeof(temp->message));
+	strcpy(temp->message, log_line);
       }
 
       if (!job->history)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 482 bytes --]

Re: [Printing-architecture] [patch] cups-1.7.0: job history fix

[Michael Sweet]

Tim,

Thanks, I filed this as:

    <rdar://problem/15331639> cups.org: Regression in auto-debug logging

That said, your proposed fix actually isn't safe either - the Fortify strcpy stuff in glibc effectively converts the strcpy to a strlcpy, so the code will break if you happen to compile using the -D_FORTIFY_SOURCE=2 compiler option.

I will follow up with a memcpy-based fix.

Thanks for reporting this!

(and we are 1 electronic signature away from getting cups.org's bug tracker back online...)


On Oct 25, 2013, at 11:36 AM, Tim Waugh <twaugh@redhat.com> wrote:

> Hi,
> 
> While playing around with a logging enhancement¹ I discovered a bug in
> the job history code. It was introduced some time after the public
> source repository was taken off-line, so I don't know what the
> motivation for the change was.
> 
> A cups_joblog_t declares 'char message[1]' for the message, and is
> allocated with enough storage for the intended message. However, the
> message is copied in with strlcpy, with a limit of
> sizeof(cups_joblog_t->message). As a result, the message is severely(!)
> truncated.
> 
> The fix is to undo the change as attached.
> 
> Tim.
> */
> 
> ¹ http://cyberelk.net/tim/2013/10/25/cups-adding-support-for-system-journal/
> <cups-jobhistory.patch>_______________________________________________
> Printing-architecture mailing list
> Printing-architecture@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/printing-architecture

_________________________________________________________
Michael Sweet, Senior Printing System Engineer, PWG Chair

Re: [Printing-architecture] [patch] cups-1.7.0: job history fix

[Michael Sweet]

[-- Attachment #1: Type: text/plain, Size: 22 bytes --]

and here is the fix:


[-- Attachment #2: rdar15331639.patch --]
[-- Type: application/octet-stream, Size: 677 bytes --]

Index: scheduler/log.c
===================================================================
--- scheduler/log.c	(revision 11365)
+++ scheduler/log.c	(working copy)
@@ -541,12 +541,13 @@
       */
 
       cupsd_joblog_t *temp;		/* Copy of log message */
+      size_t         log_len = strlen(log_line);
+					/* Length of log message */
 
-
-      if ((temp = malloc(sizeof(cupsd_joblog_t) + strlen(log_line))) != NULL)
+      if ((temp = malloc(sizeof(cupsd_joblog_t) + log_len)) != NULL)
       {
         temp->time = time(NULL);
-	strlcpy(temp->message, log_line, sizeof(temp->message));
+	memcpy(temp->message, log_line, log_len + 1);
       }
 
       if (!job->history)

[-- Attachment #3: Type: text/plain, Size: 2126 bytes --]


On Oct 28, 2013, at 11:30 AM, Michael Sweet <msweet@apple.com> wrote:

> Tim,
> 
> Thanks, I filed this as:
> 
>    <rdar://problem/15331639> cups.org: Regression in auto-debug logging
> 
> That said, your proposed fix actually isn't safe either - the Fortify strcpy stuff in glibc effectively converts the strcpy to a strlcpy, so the code will break if you happen to compile using the -D_FORTIFY_SOURCE=2 compiler option.
> 
> I will follow up with a memcpy-based fix.
> 
> Thanks for reporting this!
> 
> (and we are 1 electronic signature away from getting cups.org's bug tracker back online...)
> 
> 
> On Oct 25, 2013, at 11:36 AM, Tim Waugh <twaugh@redhat.com> wrote:
> 
>> Hi,
>> 
>> While playing around with a logging enhancement¹ I discovered a bug in
>> the job history code. It was introduced some time after the public
>> source repository was taken off-line, so I don't know what the
>> motivation for the change was.
>> 
>> A cups_joblog_t declares 'char message[1]' for the message, and is
>> allocated with enough storage for the intended message. However, the
>> message is copied in with strlcpy, with a limit of
>> sizeof(cups_joblog_t->message). As a result, the message is severely(!)
>> truncated.
>> 
>> The fix is to undo the change as attached.
>> 
>> Tim.
>> */
>> 
>> ¹ http://cyberelk.net/tim/2013/10/25/cups-adding-support-for-system-journal/
>> <cups-jobhistory.patch>_______________________________________________
>> Printing-architecture mailing list
>> Printing-architecture@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/printing-architecture
> 
> _________________________________________________________
> Michael Sweet, Senior Printing System Engineer, PWG Chair
> 
> _______________________________________________
> Printing-architecture mailing list
> Printing-architecture@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/printing-architecture

_________________________________________________________
Michael Sweet, Senior Printing System Engineer, PWG Chair

Re: [Printing-architecture] [patch] cups-1.7.0: job history fix

[Tim Waugh]

[-- Attachment #1: Type: text/plain, Size: 106 bytes --]

On Mon, 2013-10-28 at 11:35 -0400, Michael Sweet wrote:
> and here is the fix:

Thanks.

Tim.
*/


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 482 bytes --]