Bug 707662

Created attachment 25469 [details]
Videos showing the proof-of-concept exploit

Added two videos of the PoC running on my Ubuntu machine, also showing it impacting desktop usage in case of a LibreOffice file with embedded .eps (for fun but also maybe good to know).

Created attachment 25470 [details]
Minimal test case for just the vulnerability

This is a minimal file showing just the vulnerability.

Run with `ghostscript -dSAFER -dNODISPLAY -dBATCH minipoc.ps`. Then `cat /tmp/uniprint` to see the leaked stack contents.

Change the file path to an accessible location where needed.

(In reply to Thomas Rinsma from comment #1)
> Created attachment 25469 [details]
> Videos showing the proof-of-concept exploit
> 
> Added two videos of the PoC running on my Ubuntu machine, also showing it
> impacting desktop usage in case of a LibreOffice file with embedded .eps
> (for fun but also maybe good to know).

Please don't attach videos to bug reports, they really don't add anything to the report and it's essentially impossible to delete attachments from Bugzilla, we have to go and replace them with empty files instead.

We don't need you to prove something on your machine, we're quite prepared to believe you on that, but we need to see it on a machine we can use in order to debug the problem properly.

I appreciate this is a small file, but we'd still rather not receive video files at all.

(In reply to Ken Sharp from comment #3)
> Please don't attach videos to bug reports, they really don't add anything to
> the report and it's essentially impossible to delete attachments from
> Bugzilla, we have to go and replace them with empty files instead.
> 
> We don't need you to prove something on your machine, we're quite prepared
> to believe you on that, but we need to see it on a machine we can use in
> order to debug the problem properly.

My apologies! Is the attached minimal test case enough for you to reproduce the vulnerability?

For your information, I have requested a CVE for this vulnerability.

CVE-2024-29510 was assigned to this issue.

Just checking, were you able to reproduce the bug, or is any additional information required?

I'd like to stress the severity of the bug. This seems to affect various under-the-hood usages of Ghostscript, like via ImageMagick and LibreOffice, and hence various server-side and desktop use-cases.

As for a potential solution; I realize that these parameters being format strings is somewhat of a feature of uniprint so the string formatting cannot be skipped entirely. Hence it might be simplest to pre-process these two format strings (S_YMOVE and SA_WRITECOMP) in upd_put_params, making sure that they contain the expected number of format-string specifiers (currently seemingly exactly one per string).

(In reply to Thomas Rinsma from comment #7)
> Just checking, were you able to reproduce the bug, or is any additional
> information required?

Yes I can reproduce it. I'm currently completely snowed under due to customer bug reports, security reports and the fact that we are short resourced, partly due to vacations.


> As for a potential solution; I realize that these parameters being format
> strings is somewhat of a feature of uniprint so the string formatting cannot
> be skipped entirely. Hence it might be simplest to pre-process these two
> format strings (S_YMOVE and SA_WRITECOMP) in upd_put_params, making sure
> that they contain the expected number of format-string specifiers (currently
> seemingly exactly one per string).

I suspect the answer, in the short term, will be to prevent PostScript code altering these parameters after SAFER is true, which means they will only be able to be set from the command line.

Since the format strings can legitimately contain anything, it's quite hard to ensure that they contain one and only one parameter, and that the parameter is of a suitable type.

(In reply to Ken Sharp from comment #8)
> Yes I can reproduce it. I'm currently completely snowed under due to
> customer bug reports, security reports and the fact that we are short
> resourced, partly due to vacations.

Clear, and apologies for pushing on this in that case. I would offer to help with mitigation but I am not familiar enough with the codebase and the various usecases to be helpful there. Anyway, let me know when I can help by e.g. double checking a potential fix or anything.

> > As for a potential solution; I realize that these parameters being format
> > strings is somewhat of a feature of uniprint so the string formatting cannot
> > be skipped entirely. Hence it might be simplest to pre-process these two
> > format strings (S_YMOVE and SA_WRITECOMP) in upd_put_params, making sure
> > that they contain the expected number of format-string specifiers (currently
> > seemingly exactly one per string).
> 
> I suspect the answer, in the short term, will be to prevent PostScript code
> altering these parameters after SAFER is true, which means they will only be
> able to be set from the command line.
> 
> Since the format strings can legitimately contain anything, it's quite hard
> to ensure that they contain one and only one parameter, and that the
> parameter is of a suitable type.

I was not sure if this was an intended usecase, but if that's safe to do then indeed that's way better. If `setpagedevice` can be disabled entirely in case of SAFER that would reduce a ton of attack surface (for most conversion usecases).

(In reply to Thomas Rinsma from comment #9)

> > Since the format strings can legitimately contain anything, it's quite hard
> > to ensure that they contain one and only one parameter, and that the
> > parameter is of a suitable type.
> 
> I was not sure if this was an intended usecase, but if that's safe to do
> then indeed that's way better.

I wasn't certain this was going to be possible, but I've looked at some of the uniprint configuration files and convinced myself that it is possible to fully configure the uniprint device from the command line.

I can conceive of cases where the user might want to permit the PostScript program to change device and configuration, in that case they will just have to use NOSAFER. At least by requiring a specific action we can make it clear that this is a potentially risky operation.

I'd expect these cases to be in a controlled environment anyway. That is, I'd expect the PostScript program to be also under the control of the user, not acquired from a source of unknown provenance.


> If `setpagedevice` can be disabled entirely
> in case of SAFER that would reduce a ton of attack surface (for most
> conversion usecases).

We can't disable setpagedevice because it's the specified way to change all kinds of benign settings, in particular the media size but basically any device parameter. Almost all non-trivial, real world, PostScript programs involve executing setpagedevice. Sometimes many times.

We also rely on the internal mechanics for reconfiguring devices with interpreters other than PostScript which don't, obviously, use setpagedevice.

We have had some discussion internally, and will have more next week, about preventing users from switching device once SAFER is active. Apart from limiting the attack surface it occurred to me that there are many cases where the 'user' would want to prevent the PostScript program from switching devices. I'm thinking of SaaS provision in particular but anyone with a workflow involving Ghostscript might reasonably wish to prevent the device being changed by the input program.

Unfortunately we are also aware of cases where Ghostscript is being used in a workflow which requires the ability to change device.

I have a proposed solution for this, up for discussion. It's the kind of change which we'll have to give notice for before implementation, so if we do go down that route it will be flagged up in a patch release 'sometime soon' (to be clear, there definitely *will* be a patch release sometime soon, which may contain this warning) and then rolled into the next main release.

(In reply to Ken Sharp from comment #10)
> We can't disable setpagedevice because it's the specified way to change all
> kinds of benign settings, in particular the media size but basically any
> device parameter. Almost all non-trivial, real world, PostScript programs
> involve executing setpagedevice. Sometimes many times.
Good to know, I didn't consider that. I indeed specifically mean changing the output device.

> We have had some discussion internally, and will have more next week, about
> preventing users from switching device once SAFER is active. Apart from
> limiting the attack surface it occurred to me that there are many cases
> where the 'user' would want to prevent the PostScript program from switching
> devices. I'm thinking of SaaS provision in particular but anyone with a
> workflow involving Ghostscript might reasonably wish to prevent the device
> being changed by the input program.
This is my intuition as well. These conversion processes just expect e.g. a PNG or a PDF as output, usually on stdout, and nothing else. I don't think many of such users even realize what all could be happening to be honest, like in this case writing output to files under /tmp/ and reading it back. I would almost suggest a "SAFEST" mode with only stdin and stdout access (besides indeed no device switching), but I know realistically you'd run into problems and additional complexity with fonts files, etc.

> I have a proposed solution for this, up for discussion. It's the kind of
> change which we'll have to give notice for before implementation, so if we
> do go down that route it will be flagged up in a patch release 'sometime
> soon' (to be clear, there definitely *will* be a patch release sometime
> soon, which may contain this warning) and then rolled into the next main
> release.
Makes sense. As for the short term and this specific issue, do I understand correctly that you plan to change the interface for these uniprint parameters to argv-only in order to patch this for now? Then SaaS-style SAFER users have a relatively quick fix without compatibility change, and legitimate setpagedevice users are not affected yet (at least outside of uniprint).

(In reply to Thomas Rinsma from comment #11)

> like in this case writing output to files under /tmp/ and reading it back. I
> would almost suggest a "SAFEST" mode with only stdin and stdout access

We once had a PARANOIDSAFER but all that happened was SAFER gradually expanded until there was no difference.


> (besides indeed no device switching), but I know realistically you'd run
> into problems and additional complexity with fonts files, etc.

You can't feed a PDF file into Ghostscript via stdin. We do permit that, but all that happens is that we write it to the temp directory anyway and then execute it from there.

If you already have the file on disk, and it's large (many files we get are multi-gigabyte) then that's a performance overhead people wouldn't sit still for.

And some kinds of PDF file can't be sent to stdout, we need to rewrite the file after creation. The same is true of some other file formats too.


> Makes sense. As for the short term and this specific issue, do I understand
> correctly that you plan to change the interface for these uniprint
> parameters to argv-only in order to patch this for now?

Provided that it works correctly, and doesn't introduce any new problems (such as an inability to configure some aspect of the device), then yes. SAFER mode will mean that the uniprint device can't have certain parameters configured from PostScript. There are other devices with similar restrictions already so I don't anticipate a problem but this is Ghostscript, which bites you when you least expect it.

It's been a busy few weeks, and it still hasn't settled down yet, so I'm commenting on all the open security bug threads to reassure people that we are working on these. Due to slow responses on one thread it we are unfortunately not yet able to estimate a patch release date.

Because we know that there is at least one person watching/mining our public repositories for security commits we are keeping these in a private repository until a patch release. I'm afraid that means I can't just point to a proposed commit in Gitweb.

The patch to address this issue is:

diff --git a/devices/gdevupd.c b/devices/gdevupd.c
index c9389e7bc..016a9260a 100644
--- a/devices/gdevupd.c
+++ b/devices/gdevupd.c
@@ -1891,6 +1891,16 @@ out on this copies.
       if(!upd_strings[i]) continue;
       UPD_PARAM_READ(param_read_string,upd_strings[i],value,udev->memory);
       if(0 == code) {
+        if (gs_is_path_control_active(udev->memory)) {
+            if (strings[i].size != value.size)
+              error = gs_error_invalidaccess;
+            else {
+                if (strings[i].data && memcmp(strings[i].data, value.data, strings[i].size) != 0)
+                    error = gs_error_invalidaccess;
+            }
+            if (error < 0)
+                goto exit;
+        }
          if(0 <= error) error |= UPD_PUT_STRINGS;
          UPD_MM_DEL_PARAM(udev->memory, strings[i]);
          if(!value.size) {
@@ -1908,6 +1918,26 @@ out on this copies.
       if(!upd_string_a[i]) continue;
       UPD_PARAM_READ(param_read_string_array,upd_string_a[i],value,udev->memory);
       if(0 == code) {
+          if (gs_is_path_control_active(udev->memory)) {
+              if (string_a[i].size != value.size)
+                  error = gs_error_invalidaccess;
+              else {
+                  int loop;
+                  for (loop = 0;loop < string_a[i].size;loop++) {
+                      gs_param_string *tmp1 = (gs_param_string *)&(string_a[i].data[loop]);
+                      gs_param_string *tmp2 = (gs_param_string *)&value.data[loop];
+
+                      if (tmp1->size != tmp2->size)
+                          error = gs_error_invalidaccess;
+                      else {
+                          if (tmp1->data && memcmp(tmp1->data, tmp2->data, tmp1->size) != 0)
+                              error = gs_error_invalidaccess;
+                      }
+                  }
+              }
+            if (error < 0)
+                goto exit;
+          }
          if(0 <= error) error |= UPD_PUT_STRING_A;
          UPD_MM_DEL_APARAM(udev->memory, string_a[i]);
          if(!value.size) {
@@ -2102,6 +2132,7 @@ transferred into the device-structure. In the case of "uniprint", this may
       if(0 > code) error = code;
    }
 
+exit:
    if(0 < error) { /* Actually something loaded without error */
 
       if(!(upd = udev->upd)) {


There will also be another commit to prevent devices being altered after SAFER is active, which will also add a control (--permit_devices=) to create a list of devices which can be selected by PostScript.

(In reply to Ken Sharp from comment #13)
> It's been a busy few weeks, and it still hasn't settled down yet, so I'm
> commenting on all the open security bug threads to reassure people that we
> are working on these. Due to slow responses on one thread it we are
> unfortunately not yet able to estimate a patch release date.
All good, thanks for the update!

> The patch to address this issue is:
> (...)
I can confirm that this indeed prevents the parameters from being set (and hence the bug from being triggered in this way), but I must admit that I don't quite understand how this works. It seems these comparisons make sure that none of the string (array) values can be changed to different values, but then how can they be set in the first place?

> There will also be another commit to prevent devices being altered after
> SAFER is active, which will also add a control (--permit_devices=) to create
> a list of devices which can be selected by PostScript.
Nice, this will greatly reduce the attack surface!

(In reply to Thomas Rinsma from comment #14)

> I can confirm that this indeed prevents the parameters from being set (and
> hence the bug from being triggered in this way), but I must admit that I
> don't quite understand how this works. It seems these comparisons make sure
> that none of the string (array) values can be changed to different values,

Yep, that's exactly how it works.

> but then how can they be set in the first place?

You have to set them from the command line. We more or less have to trust command line parameters.

If you look in ghostpdl/lib/*.upp these files are all example command lines for setting these (and other!) parameters.

Alternatively, of course, you can just use NOSAFER and accept the fact that PostScript can change them.

 
> > There will also be another commit to prevent devices being altered after
> > SAFER is active, which will also add a control (--permit_devices=) to create
> > a list of devices which can be selected by PostScript.
> Nice, this will greatly reduce the attack surface!

I realise I wasn't absolutely clear here, because we need to give people warning that one won't be in the patch release, instead the patch release will carry notice that we intend to change the behaviour, and the September release will contain the actual commit.

(In reply to Ken Sharp from comment #15)
> (In reply to Thomas Rinsma from comment #14)
> > but then how can they be set in the first place?
> 
> You have to set them from the command line. We more or less have to trust
> command line parameters.
Ah right, because those are of course set before path_control_active is set, makes sense now. I guess what makes it seem a bit convoluted for me is the fact that you do allow "setting" these parameters as long as the value doesn't change (as opposed to just putting the gs_is_path_control_active check higher up). But presumably this is to be extra precise and still allow some code with redundant parameter setting.

Either way, it fixes the bug :)

> I realise I wasn't absolutely clear here, because we need to give people
> warning that one won't be in the patch release, instead the patch release
> will carry notice that we intend to change the behaviour, and the September
> release will contain the actual commit.
Got it.

(In reply to Thomas Rinsma from comment #16)

> Ah right, because those are of course set before path_control_active is set,
> makes sense now. I guess what makes it seem a bit convoluted for me is the
> fact that you do allow "setting" these parameters as long as the value
> doesn't change (as opposed to just putting the gs_is_path_control_active
> check higher up). But presumably this is to be extra precise and still allow
> some code with redundant parameter setting.

Not exactly. It's because the function can be called many times, even just during startup, and each time it is called it gets all the parameters currently set because they are accumulated into a dictionary in the PostScript environment and the entire dictionary is sent each time setpagedevice is executed.

So if you change the media, then you'll *also* get all the device parameters.

So we have to check to see if the parameter is not just being 'set', but actually being changed. As a side-effect it means that the parameters which aren't sensitive (not format strings) can be changed by PostScript, but frankly I doubt anyone would really want to do that. Still the possibility is there if needed.

Ahh right. Thanks for the clarification. It's a complex beast :)