Bug 699677 - .bindnow still causing side effects
Summary: .bindnow still causing side effects
Status: RESOLVED FIXED
Alias: None
Product: Ghostscript
Classification: Unclassified
Component: Security (public) (show other bugs)
Version: unspecified
Hardware: PC Linux
: P4 major
Assignee: Ken Sharp
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-27 17:55 UTC by Tavis Ormandy
Modified: 2019-05-08 13:47 UTC (History)
2 users (show)

See Also:
Customer:
Word Size: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tavis Ormandy 2018-08-27 17:55:25 UTC
I'm still seeing .bindnow change behaviour when I think it shouldn't, even after bug 699658 and bug 699662 is resolved. For example, here is another example that still works in HEAD:

{ .bindnow } stopped {} if

[ /undefined /undefined /undefined /undefined ] false setcustomcolor

The bad setcustomcolor does nothing usually, but if you type .bindnow first, it causes memory corruption.
Comment 1 Ken Sharp 2018-08-27 18:04:22 UTC
Hmm, well .bindnow shouldn't even be available, and certainly shouldn't do anything, unless DELAYBIND is true. I'm assuming you aren't setting DELAYBIND.

I'll look into it tomorrow, we're going to be auditing the PostScript functions in systemdict anyway, last time we only did the C operator definitions.  As usual the first question is why is there a problem, which we need to solve before removing access to the function.
Comment 2 Tavis Ormandy 2018-08-27 18:11:27 UTC
Thanks Ken, it looks like it isn't available, but still seems to have some effect somehow.

I think this requires more knowledge of gs internals than I possess to understand :)
Comment 3 Ken Sharp 2018-08-28 15:32:08 UTC
This is more a 'note to self' than anything else right now. I've removed .bindnow from systemdict so it can't be run (.bindnow is still open, the 'undefined' error is because one of the variables it uses isn't defined unless DELAYBIND is true). So the error is correct, but slightly misleading in this case.

The relevant commit is 520bb0ea7519aa3e79db78aaf0589dae02103764

However, I still intend to pursue the root cause of the problem here. So, when I can't reproduce this problem; remember to put .bindnow back!

The reason for the change is that we intend to start our release process, and I don't want to leave .bindnow defined in the release (because it should not be).

Tavis, once we start this we're going to be less responsive for about 1 week, that's how long our QA usually takes. It may be longer this time......

So please don't think we're ignoring you or treating these less seriously.
Comment 4 Tavis Ormandy 2018-08-28 19:42:33 UTC
Understood, thanks for letting me know Ken!
Comment 5 Ken Sharp 2018-09-01 08:35:08 UTC
OK this was a complicated one.

The actual problem is that the sampled function continuation procedure wasn't cleaning up the execution stack in the case of an error.

The reason it was getting an error is because setcustomcolor wasn't validating its operands and constructing a tint transform procedure which was invalid.

So we need to address both problems, because there are other ways to run the sampled function code.

This commit cleans up the exec stack in the error case, adds a detection to setcolorspace when the sampled function fails (and cleaned up a similar exec stack corruption oversight in that code), and then finally adds parameter validation to setcustomcolor.

I haven't bothered to add validation to findcmykcustomcolor and findrgbcustomcolor, the only thing you can do with the result of these is pass it to setcustomcolor which will validate it there. PostScript will throw errors if there are insufficient operands to the functions and we don't care about their type.

commit 61ad589fb861d28e2d2c9d0b609ec4da4d7dd247

As I noted in the commit message, the .bindnow is a red herring really, all that does is alter the layout of the exec stack very slightly and cause the underlying problem to exhibit.