Bug 699654

I can't reproduce this, the line:
mark /OutputFile (%pipe%id) currentdevice putdeviceprops


returns:

Error: /undefined in --.putdeviceprops--
Operand stack:

Execution stack:
   %interp_exit   .runexec2   --nostringval--   --nostringval--   --nostringval--   2   %stopped_push   --nostringval--   --nostringval--   %loop_continue   --nostringval--   --nostringval--   false   1   %stopped_push   .runexec2   --nostringval--   --nostringval--   --nostringval--   2   %stopped_push   --nostringval--   1882   4   3   %oparray_pop   --nostringval--   1863   4   3   %oparray_pop
Dictionary stack:
   --dict:984/1684(ro)(G)--   --dict:0/20(G)--   --dict:78/200(L)--
Current allocation mode is local
Last OS error: No such file or directory
Current file position is 57


So the new OutputFile is not set.

Thanks for investigating Chris, are you just building from HEAD?

I used a default build of the last release on Linux, but it also repros on default Ubuntu and RedHat.


I think it would say /invalidaccess if the bug was fixed, so I likely just need to tweak the repro.

Ah, Chris, is it possible you didn't use -sDEVICE=ppmraw? (or pngalpha, or something similar)

I get the same error you do if I use x11alpha, I think it still works but I need to investigate how to make it work with that output device.

I used the command line, and lines you gave - I did use HEAD, but I'm not sure what might have changed in relatively recent history to affect it.

Ah-ha, I think I've got a version that works in HEAD, just change the very first paper size from "legal" to "a0":


$ ./gs -sDEVICE=ppmraw -dSAFER 
GPL Ghostscript GIT PRERELEASE 9.24 (2018-03-21)
Copyright (C) 2018 Artifex Software, Inc.  All rights reserved.
This software comes with NO WARRANTY: see the file PUBLIC for details.
GS>a0
GS>{ null restore } stopped { pop } if
GS>legal
GS>mark /OutputFile (%pipe%id) currentdevice putdeviceprops
GS<1>showpage
uid=1000(taviso) gid=1000(taviso) groups=1000(taviso),10(wheel) context=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023

I just did a clean git clone; autogen.sh; make and this reproduces every time for me.

I am able to reproduce from HEAD using the sequence from Tavis' most recent comment.

Notes on what I found so far (in case somebody else gets to this before me):

in restore_page_device(), which gets called during the 
"{null restore} stopped {pop} if" sequence in the failure case, I see:

    /* If we are going to putdeviceparams in a callout, we need to */
    /* unlock temporarily.  The device will be re-locked as needed */
    /* by putdeviceparams from the pgs_old->pagedevice dict state. */
    if (!samepagedevice)
        dev_old->LockSafetyParams = false;
    dev_new = gs_currentdevice(pgs_new);

At this point, the LockSafetyParams is set to false.
I don't understand without more investigation where this promised re-locking is supposed to occur, but apparently it is not happening.

Later (during "mark /OutputFile (%pipe%id) currentdevice putdeviceprops"), 
we go through this code in gdev_prn_put_params() that is supposed to check that flag:

   switch (code = param_read_string(plist, (param_name = "OutputFile"), &ofs)) {
        case 0:
            if (pdev->LockSafetyParams &&
                    bytes_compare(ofs.data, ofs.size,
                        (const byte *)ppdev->fname, strlen(ppdev->fname))) {
                code = gs_error_invalidaccess;
 
Since LockSafetyParams is now false, the check doesn't happen and no invalidaccess is thrown.

I've got a solution for this (sorry Nancy, I looked at it before realizing you'd also done so):

http://git.ghostscript.com/?p=user/chrisl/ghostpdl.git;a=commitdiff;h=8f6017754377


I don't fully understand the reason for splitting the "restore" operator up in the way it is.... and I don't have time to work it out.

Fixed in:

http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff;h=78911a01b6

It's not possible for restore to fail other than with /typecheck?

(In reply to Tavis Ormandy from comment #10)
> It's not possible for restore to fail other than with /typecheck?

It is, but it's a fatal error, so the interpreter should exit immediately.

Actually, let me double check that (tomorrow - it's late here): I'll reopen this, or open a new bug, depending on what I find.

Thanks so much Chris and Nancy for investigating this.

Okay, there is another way for restore to fail non-fatally but, more to the point, I really didn't like the way parameter checking was being done, in relation to the rest of the restore operation, so I've rejigged the code to do things in a more appropriate order:

http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff;h=5516c614dc33

The remaining failure scenarios should be fatal, and cause the interpreter to exit immediately but, *just in case*, I've added a catch so we never return from a restore with the device no longer "safe", if it started that way.

Hmm, that looks comprehensive for the failure case, Thanks!

What about special devices like nulldevice?

This still works for me in HEAD (git pull 10 minutes ago):

$ ./gs -dSAFER -sDEVICE=ppmraw
GPL Ghostscript GIT PRERELEASE 9.24 (2018-03-21)
Copyright (C) 2018 Artifex Software, Inc.  All rights reserved.
This software comes with NO WARRANTY: see the file PUBLIC for details.
GS>nulldevice save
GS<1>a0
GS<1>restore
GS>a0
GS>mark /OutputFile (%pipe%id) currentdevice putdeviceprops
GS<1>showpage
uid=0(taviso) gid=421(primarygroup)

I am able to reproduce this on linux. Re-opened.

(In reply to Tavis Ormandy from comment #15)
> Hmm, that looks comprehensive for the failure case, Thanks!
> 
> What about special devices like nulldevice?
> 
> This still works for me in HEAD (git pull 10 minutes ago):
> 
> $ ./gs -dSAFER -sDEVICE=ppmraw
> GPL Ghostscript GIT PRERELEASE 9.24 (2018-03-21)
> Copyright (C) 2018 Artifex Software, Inc.  All rights reserved.
> This software comes with NO WARRANTY: see the file PUBLIC for details.
> GS>nulldevice save
> GS<1>a0
> GS<1>restore
> GS>a0
> GS>mark /OutputFile (%pipe%id) currentdevice putdeviceprops
> GS<1>showpage
> uid=0(taviso) gid=421(primarygroup)

That's *not* the same problem, it's not even a closely related problem.

This bug report was for the exploit being available "after a failed restore", what's in the above example is a *successful* restore.

In general, it is preferable to open new bugs for different problems.


The problem here is that pushing the nulldevice using the nulldevice operator does not preserve *any* settings from the "outgoing" device - including the LockSafetyParams setting.

I *believe* this is specific to the NULL device since I think it is the only "standalone" device that can be set without using the "proper" setpagedevice method. *But* I'm not certain, we will have to audit other devices to be sure.

In the meantime, a solution for the nulldevice is:
http://git.ghostscript.com/?p=user/chrisl/ghostpdl.git;a=commitdiff;h=79cccf641

(I won't push that unless it's positively reviewed by another gs developer)

(In reply to Chris Liddell (chrisl) from comment #17)

> I *believe* this is specific to the NULL device since I think it is the only
> "standalone" device that can be set without using the "proper" setpagedevice
> method. *But* I'm not certain, we will have to audit other devices to be
> sure.
> 
> In the meantime, a solution for the nulldevice is:
> http://git.ghostscript.com/?p=user/chrisl/ghostpdl.git;a=commitdiff;
> h=79cccf641
> 
> (I won't push that unless it's positively reviewed by another gs developer)

It looks sensible to me, but we should review the other devices as you say.

(In reply to Chris Liddell (chrisl) from comment #17)
> That's *not* the same problem, it's not even a closely related problem.
> 
> This bug report was for the exploit being available "after a failed
> restore", what's in the above example is a *successful* restore.
> 
> In general, it is preferable to open new bugs for different problems.
> 

Fair enough, I was reviewing your change making sure the LockSafetyParams is restored on error when the idea came to me. I guess I considered it an extension of abusing the save/restore machinery.

OKay, first, for the record, the issue isn't with the null device, but is actually with the nulldevice operator - a subtle distinction, but an important one. The issue is with how the device is installed into the graphics state, rather than the device itself.

So, just to be sure, I've been through the other devices, and (more importantly) the device operators. Only (as far as I can tell) the nulldevice operator provides a way around the normal setpagedevice/setdevice "normal" ways to push a device, and those will fail with an invalidaccess if we're running SAFER. Also, the other devices we ship honour and preserve the LockSafetyParams setting.


So:
http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff;h=79cccf641486

Fixes the exploit with nulldevice.

I have found another way to abuse the save/restore machinery to leave LockSafetyParams unset. In my opinion this is closely related to the previous example, but if you feel strongly it isn't, let me know and I'll open a different bug.

NOTE: I'm not sure why I need to tweak paper sizes for different gs configurations, do you know why? It would help make my testing more efficient if I didn't have to try a few different sizes to convince myself something doesn't work.

This still works in HEAD as of 10 minutes ago:


$ gs -q -dSAFER -sDEVICE=ppmraw
GS>a5
GS>1 .pushpdf14devicefilter
GS>save
GS<1>legal
GS<1>restore
GS>mark /OutputFile (%pipe%id) currentdevice putdeviceprops
GS<1>showpage
uid=1000(taviso) gid=1000(primarygroup) 

(Side note: can I have bugzilla permissions to reopen/confirm issues?)

(In reply to Tavis Ormandy from comment #21)

> NOTE: I'm not sure why I need to tweak paper sizes for different gs
> configurations, do you know why?

Changing paper size causes a setpagdevice to be issued, which (amongst other things) completely resets and reconfigures the device.

This will change the memory layout of the executing process, and may be why you need to do this, its not the changing of the media size so much as changing what's at an actual location in memory.


> It would help make my testing more
> efficient if I didn't have to try a few different sizes to convince myself
> something doesn't work.

Checking it with valgrind might, possibly add confidence, though I can't say if that would help in this case, someone (robably Chris) will have to investigate.


> (Side note: can I have bugzilla permissions to reopen/confirm issues?)

Umm, not sure about that, Bugzilla administration is a bit of a black art. Currently security issues are private, which means only the Artifex group (ie the staff), the reporter, and people on the CC list can open them. I'm not sure what it takes to reopen a bug.

I've moved it back to unresolved for now and assigned it tentatively to Chris, that may change. (I've also removed the customer numbers, there's no point in getting bogged down in the administrivia that involves).

Understood, no problem :)

The primary problem is that the setpagdevice logic gets greatly confused by
the presence of the pdf14 transparency compositor. The .currentpagedevice
returns null, so the setpagedevice thinks that the nulldevice is installed.

The confusion is shown by:
    a5 1 .pushpdf14devicefilter currentpagedevice ===
which returns an empty dictionary, assumed to be the nullpagedevice (see
gs_setpd.ps lines 150-159). 

The pdf14 device doesn't have the LockSafetyParams set (it is not in the
COPY_PARAMS of gs_pdf14_device_copy_params, but setting that makes even the
'save' fail since that would require a setdevice to a device which is not the
current device (restricted by LockSafetyParams) -- see zdevice.c line 469).

Recommendation:

Getting rid of the .pushpdf14devicefilter params from the available user set
of procedures prevents this type of confusion, but, this means that we have to
(finally) drop examples/transparency_example.ps [at least running it would
not work in SAFER mode].

Generally, I don't think our pdf14 device compositor is set to handle any
change in geometry once the page is open (as the "legal" operator does), and
the LockSafetyParams of the pdf14 device is not (AFAICT) relevant during the
composition of the page.

This commit: 520bb0ea7519aa3e79db78aaf0589dae02103764 prevents access to .pdf14pushfilter when SAFER is true.

For now I've chosen to only do this when -dSAFER is set. However we intend to audit systemdict and its entirely possible that we will choose to undefine this operator permanently at that time.

Note that commit also undefines a number of other operators when SAFER is true, and removes .bindnow when DELAYBIND is not true (it should not be available in this case).