Skip to content

Fix buffer overflow in ppdLoadAttributes().#51

Closed
dcoppa wants to merge 1 commit intoOpenPrinting:masterfrom
dcoppa:master
Closed

Fix buffer overflow in ppdLoadAttributes().#51
dcoppa wants to merge 1 commit intoOpenPrinting:masterfrom
dcoppa:master

Conversation

@dcoppa
Copy link

@dcoppa dcoppa commented Nov 6, 2024

On my musl libc based Linux distribution (Chimera Linux), all my print jobs were failing:

D [06/Nov/2024:07:34:56 +0100] [Job 18] Applying default options...
D [06/Nov/2024:07:34:56 +0100] [Job 18] Adding start banner page "none".
D [06/Nov/2024:07:34:56 +0100] [Job 18] Queued on "ENVY_4520" by "dcoppa".
D [06/Nov/2024:07:34:56 +0100] [Job 18] Auto-typing file...
D [06/Nov/2024:07:34:56 +0100] [Job 18] Request file type is text/plain.
D [06/Nov/2024:07:34:56 +0100] [Job 18] File of type text/plain queued by "dcoppa".
D [06/Nov/2024:07:34:56 +0100] [Job 18] Adding end banner page "none".
D [06/Nov/2024:07:34:56 +0100] [Job 18] time-at-processing=1730874896
D [06/Nov/2024:07:34:56 +0100] [Job 18] 3 filters for job:
D [06/Nov/2024:07:34:56 +0100] [Job 18] - (text/plain to application/vnd.universal-input, cost 0)
D [06/Nov/2024:07:34:56 +0100] [Job 18] universal (application/vnd.universal-input to application/vnd.cups-raster, cost 0)
D [06/Nov/2024:07:34:56 +0100] [Job 18] hpcups (application/vnd.cups-raster to printer/ENVY_4520, cost 0)
D [06/Nov/2024:07:34:56 +0100] [Job 18] job-sheets=none,none
D [06/Nov/2024:07:34:56 +0100] [Job 18] Mapping media to Pagesize=A4
D [06/Nov/2024:07:34:56 +0100] [Job 18] After mapping finishings PageSize=A4
D [06/Nov/2024:07:34:56 +0100] [Job 18] argv[0]="ENVY_4520"
D [06/Nov/2024:07:34:56 +0100] [Job 18] argv[1]="18"
D [06/Nov/2024:07:34:56 +0100] [Job 18] argv[2]="dcoppa"
D [06/Nov/2024:07:34:56 +0100] [Job 18] argv[3]="trial.txt"
D [06/Nov/2024:07:34:56 +0100] [Job 18] argv[4]="1"
D [06/Nov/2024:07:34:56 +0100] [Job 18] argv[5]="finishings=3 media=a4 number-up=1 print-color-mode=color job-uuid=urn:uuid:9b22db21-acf4-3780-6912-ecac5aa9da16 job-originating-host-name=localhost date-time-at-creation= date-time-at-processing= time-at-creation=1730874896 time-at-processing=1730874896 document-name-supplied=trial.txt PageSize=A4"
D [06/Nov/2024:07:34:56 +0100] [Job 18] argv[6]="/var/spool/cups/d00018-001"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[0]="CUPS_CACHEDIR=/var/cache/cups"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[1]="CUPS_DATADIR=/usr/share/cups"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[2]="CUPS_DOCROOT=/usr/share/cups/doc"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[3]="CUPS_REQUESTROOT=/var/spool/cups"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[4]="CUPS_SERVERBIN=/usr/lib/cups"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[5]="CUPS_SERVERROOT=/etc/cups"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[6]="CUPS_STATEDIR=/run/cups"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[7]="HOME=/var/spool/cups/tmp"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[8]="PATH=/usr/lib/cups/filter:/usr/bin:/usr/bin:/bin:/usr/bin"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[9]="SERVER_ADMIN=root@t14s"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[10]="SOFTWARE=CUPS/2.4.11"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[11]="TMPDIR=/var/spool/cups/tmp"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[12]="USER=root"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[13]="CUPS_MAX_MESSAGE=2047"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[14]="CUPS_SERVER=/run/cups/cups.sock"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[15]="CUPS_ENCRYPTION=IfRequested"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[16]="IPP_PORT=631"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[17]="CHARSET=utf-8"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[18]="LANG=en_US.UTF-8"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[19]="PPD=/etc/cups/ppd/ENVY_4520.ppd"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[20]="CONTENT_TYPE=text/plain"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[21]="DEVICE_URI=ipps://192.168.1.2:443/ipp/print"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[22]="PRINTER_INFO=HP ENVY 4520"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[23]="PRINTER_LOCATION=Home"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[24]="PRINTER=ENVY_4520"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[25]="PRINTER_STATE_REASONS=none"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[26]="CUPS_FILETYPE=document"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[27]="FINAL_CONTENT_TYPE=application/vnd.cups-raster"
D [06/Nov/2024:07:34:56 +0100] [Job 18] envp[28]="AUTH_I****"
D [06/Nov/2024:07:34:56 +0100] [Job 18] Started filter /usr/lib/cups/filter/universal (PID 20446)
D [06/Nov/2024:07:34:56 +0100] [Job 18] Started filter /usr/lib/cups/filter/hpcups (PID 20447)
D [06/Nov/2024:07:34:56 +0100] [Job 18] Started backend /usr/lib/cups/backend/ipps (PID 20448)
D [06/Nov/2024:07:34:56 +0100] [Job 18] Sending stdin for job...
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: +connecting-to-device
D [06/Nov/2024:07:34:56 +0100] [Job 18] Looking up "192.168.1.2"...
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -connecting-to-device
D [06/Nov/2024:07:34:56 +0100] [Job 18] 192.168.1.2=192.168.1.2
D [06/Nov/2024:07:34:56 +0100] [Job 18] ppdFilterLoadPPD: Color profile qualifier determined from job and PPD data 'RGB.Plain.'
D [06/Nov/2024:07:34:56 +0100] [Job 18] ppdFilterLoadPPD: No ICC profiles specified in PPD
D [06/Nov/2024:07:34:56 +0100] [Job 18] ppdFilterLoadPPD: Searching for profile "-/Plain"...
D [06/Nov/2024:07:34:56 +0100] [Job 18] prnt/hpcups/HPCupsFilter.cpp 592: cupsRasterOpen failed, fd = 0
D [06/Nov/2024:07:34:56 +0100] [Job 18] PID 20446 (/usr/lib/cups/filter/universal) crashed on signal 4.
D [06/Nov/2024:07:34:56 +0100] [Job 18] Hint: Try setting the LogLevel to "debug" to find out more.
D [06/Nov/2024:07:34:56 +0100] [Job 18] PID 20447 (/usr/lib/cups/filter/hpcups) stopped with status 1.
D [06/Nov/2024:07:34:56 +0100] [Job 18] Hint: Try setting the LogLevel to "debug" to find out more.
D [06/Nov/2024:07:34:56 +0100] [Job 18] hrDeviceDesc="ENVY 4520 series"
D [06/Nov/2024:07:34:56 +0100] [Job 18] prtMarkerColorantValue.1.1 = "tri-color ink cartridge"
D [06/Nov/2024:07:34:56 +0100] [Job 18] prtMarkerColorantValue.1.2 = "black ink cartridge"
D [06/Nov/2024:07:34:56 +0100] [Job 18] ATTR: marker-colors=none,none
D [06/Nov/2024:07:34:56 +0100] [Job 18] ATTR: marker-names='"tri-color ink cartridge HP unknown"','"black ink cartridge HP unknown"'
D [06/Nov/2024:07:34:56 +0100] [Job 18] ATTR: marker-types=ink,ink
D [06/Nov/2024:07:34:56 +0100] [Job 18] ATTR: marker-levels=-1,-1
D [06/Nov/2024:07:34:56 +0100] [Job 18] new_supply_state=0, change_state=ffff
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -developer-low-report
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -developer-empty-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -marker-supply-low-report
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -marker-supply-empty-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -opc-near-eol-report
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -opc-life-over-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -toner-low-report
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -toner-empty-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -waste-receptacle-almost-full-report
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -waste-receptacle-full-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -cleaner-life-almost-over-report
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -cleaner-life-over-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] new_state=0, change_state=ffff
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -media-empty-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -door-open-report
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -media-jam-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -input-tray-missing-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -output-tray-missing-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -marker-supply-missing-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -output-area-almost-full-report
D [06/Nov/2024:07:34:56 +0100] [Job 18] STATE: -output-area-full-warning
D [06/Nov/2024:07:34:56 +0100] [Job 18] backendWaitLoop(snmp_fd=5, addr=0x7727dd1b1a88, side_cb=0x5e88371d3c90)
D [06/Nov/2024:07:34:56 +0100] [Job 18] PID 20448 (/usr/lib/cups/backend/ipps) exited with no errors.
D [06/Nov/2024:07:34:56 +0100] [Job 18] End of messages
D [06/Nov/2024:07:34:56 +0100] [Job 18] printer-state=3(idle)
D [06/Nov/2024:07:34:56 +0100] [Job 18] printer-state-message="Filter failed"
D [06/Nov/2024:07:34:56 +0100] [Job 18] printer-state-reasons=none
E [06/Nov/2024:07:42:35 +0100] [Job 19] Job stopped due to filter errors; please consult the /var/log/cups/error_log file for details.
D [06/Nov/2024:07:42:35 +0100] [Job 19] The following messages were recorded from 07:42:35 to 07:42:35

@dcoppa
Fix buffer overflow in ppdLoadAttributes().
82d22e9 
On my musl libc based Linux distribution (Chimera Linux), all my print jobs were failing:

D [06/Nov/2024:07:34:56 +0100] [Job 18] ppdFilterLoadPPD: Color profile qualifier determined from job and PPD data \'RGB.Plain.\'
D [06/Nov/2024:07:34:56 +0100] [Job 18] ppdFilterLoadPPD: No ICC profiles specified in PPD
D [06/Nov/2024:07:34:56 +0100] [Job 18] ppdFilterLoadPPD: Searching for profile \"-/Plain\"...
D [06/Nov/2024:07:34:56 +0100] [Job 18] prnt/hpcups/HPCupsFilter.cpp 592: cupsRasterOpen failed, fd = 0
D [06/Nov/2024:07:34:56 +0100] [Job 18] PID 20446 (/usr/lib/cups/filter/universal) crashed on signal 4.
D [06/Nov/2024:07:34:56 +0100] [Job 18] Hint: Try setting the LogLevel to "debug" to find out more.

Signed-off-by: David Coppa <dcoppa@gmail.com>
tillkamppeter
tillkamppeter reviewed Nov 6, 2024
View reviewed changes
ppd/ppd-ipp.c
{
strcpy(buf, ptr);
snprintf(buf, sizeof(buf), "%s", ptr);
ptr = buf;
Copy link
Member

@tillkamppeter tillkamppeter Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that, at least for the string in buf comming from a correct *cupsFilter(2): ... line, the crash cannot have been caused by the string pointed at by ptr being too long for buf, as ptr is pointing somewhere into buf. The problem is that, according to the man page, strcpy() is not suitable for copying overlapping strings/buffers, and so that could have caused the crash here.

Now, with the replacement by snprintf() the crash has gone away in this particular case, but here the man page also says that this function is not suitable for overlapping strings/buffers, so one should expect that a crash still can happen in other situations.

To be safe one could eaither do a for() loop moving the characters to the beginning of the buffer one-by-one or the memmove() function which according to its man page supports overlapping memory areas.

memmove(buf, ptr, strnlen(ptr, sizeof(buf) - 1) + 1);

could be used here and should work reliably, at least if buf contains a correctly \0-terminated string.

Copy link
Member

@michaelrsweet michaelrsweet Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strlen(ptr) + 1 (to also move the terminating nul character)

@tillkamppeter
Copy link
Member

tillkamppeter commented Nov 6, 2024

@dcoppa Could you try with my suggested memmove()?

@dcoppa
Copy link
Author

dcoppa commented Nov 6, 2024

I've just tested, and I can confirm that printing works fine also with "memmove(buf, ptr, strnlen(ptr, sizeof(buf) - 1) + 1);".
Thank you.

@tillkamppeter
Copy link
Member

tillkamppeter commented Nov 6, 2024

Starting at line 540 we have

     if (*ptr)
	*ptr = '\0';
      // Check whether the second word is not the cost value, then we have
      // a "*cupsFilter2:* line and the second word is the printer's input
      // format
      ptr ++;

Before doing the ptr ++; we do not check whether we are already beyond the terminating zero byte of the string in buf. If in kline 540 (the if (*ptr)) ptr had already pointed to the terminating zero (forged PPD with only one word as argument string of *cupsFilter(2): ... line), we do move ptr beyond the string with the ptr ++;

To solve this, this snipped should be changed into

      if (*ptr)
      {
	*ptr = '\0';
	// Check whether the second word is not the cost value, then we have
	// a "*cupsFilter2:* line and the second word is the printer's input
	// format
	ptr ++;
      }

@dcoppa
Copy link
Author

dcoppa commented Nov 6, 2024 •
edited
Loading

I'm running with this now, and everything works:

diff -ur a/ppd/ppd-ipp.c b/ppd/ppd-ipp.c
--- a/ppd/ppd-ipp.c	2024-10-18 00:15:03.000000000 +0200
+++ b/ppd/ppd-ipp.c	2024-11-06 15:13:28.860584389 +0100
@@ -538,16 +538,18 @@
       ptr = buf;
       while (*ptr && !isspace(*ptr)) ptr ++;
       if (*ptr)
+      {
 	*ptr = '\0';
 
       // Check whether the second word is not the cost value, then we have
       // a "*cupsFilter2:* line and the second word is the printer's input
       // format
       ptr ++;
+      }
       while (*ptr && isspace(*ptr)) ptr ++;
       if (!isdigit(*ptr))
       {
-	strcpy(buf, ptr);
+	memmove(buf, ptr, strnlen(ptr, sizeof(buf) - 1) + 1);
 	ptr = buf;
 	while (*ptr && !isspace(*ptr)) ptr ++;
 	if (*ptr)

tillkamppeter added a commit that referenced this pull request Nov 6, 2024
@tillkamppeter
Fix crash bugs in ppdLoadAttributes()
7da4e58 
When parsing the "*cupsFilter(2): ..." lines in the PPD file

- use memmove() instead of strcpy() as the latter does not support
  handling overlapping memory portions

- do not move running pointer beyond the end of the input string

Pull request #51
@tillkamppeter
Copy link
Member

tillkamppeter commented Nov 6, 2024

Applied these changes in commit 7da4e58

@dcoppa thanks a lot for the report.

I'm running with this now, and everything works:

diff -ur a/ppd/ppd-ipp.c b/ppd/ppd-ipp.c
--- a/ppd/ppd-ipp.c	2024-10-18 00:15:03.000000000 +0200
+++ b/ppd/ppd-ipp.c	2024-11-06 15:13:28.860584389 +0100
@@ -538,16 +538,18 @@
       ptr = buf;
       while (*ptr && !isspace(*ptr)) ptr ++;
       if (*ptr)
+      {
 	*ptr = '\0';
 
       // Check whether the second word is not the cost value, then we have
       // a "*cupsFilter2:* line and the second word is the printer's input
       // format
       ptr ++;
+      }
       while (*ptr && isspace(*ptr)) ptr ++;
       if (!isdigit(*ptr))
       {
-	strcpy(buf, ptr);
+	memmove(buf, ptr, strnlen(ptr, sizeof(buf) - 1) + 1);
 	ptr = buf;
 	while (*ptr && !isspace(*ptr)) ptr ++;
 	if (*ptr)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelrsweet michaelrsweet michaelrsweet left review comments

@tillkamppeter tillkamppeter tillkamppeter left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dcoppa @tillkamppeter @michaelrsweet