From 65a9046ded8e9edd5d33bc812a9e94ae29607a1e Mon Sep 17 00:00:00 2001
From: Ken Sharp <ken.sharp@artifex.com>
Date: Fri, 7 Sep 2018 15:22:29 +0100
Subject: [PATCH] Bug #699707 "Security review bug - continuation procedures"
As a result of the recent security review, this bug was raised to go
through the PostScript interpreter looking for places where we exit the
'C' level and return control to PostScript. This is done when we need
to evaluate something in the PostScript environment, such as a transfer
function or a tint transform.
Because these functions are written in PostScript we need to run them
in the PostScript environment.
To do this we push the procedure (or at least 'a' procedure) onto the
exec stack and exit with an o_push_estack error. In many cases that's
all we need to do, but sometimes we want to return control back to the
'C' environment and, in some of those cases, we want to store some state
for the C code. We can't use the operand stack (because the PostScript
function will alter that) so we store stuff on the exec stack instead.
When we complete the C level, we should restore the exec stack, so if
we stored any state on it, we should remove it. Sometimes we were not
doing so if there was an error.
Generally this did not cause a problem, because in general on an error
we would stop. However if the error handler had been altered it was
possible we might carry on. 'Sometimes' that would mean we tried to
execute something which wasn't executable, and sometimes it might mean
that we tried to return to the C level, but without the expected
state on the exec stack.
This could lead to memory corruption and crashes.
This commit tries to find everywhere where we might end up leaving
extra items on the exec stack in the case of an error, and either
removes the required number of items from the exec stack or uses
whatever cleanup routine was established for the C code.
Its important to note that, in normal use, none of these could actually
cause a problem. This makes it hard to test. all the cases here I have
tested, though in many cases the only way I could produce an error was
by forcing an error return in the debugger. I suspect some error cases
simply aren't possible but its good practice to check the return codes
anyway, even if its only a theoretical problem.
---
psi/zalg.c | 5 +++-
psi/zcie.c | 82 ++++++++++++++++++++++++++++++++--------------------------
psi/zcolor1.c | 9 ++++++-
psi/zcontrol.c | 4 ++-
psi/zfile.c | 8 ++++--
psi/zht1.c | 5 +++-
psi/zht2.c | 5 +++-
psi/zpath1.c | 2 ++
psi/zpcolor.c | 12 ++++++---
9 files changed, 86 insertions(+), 46 deletions(-)
diff --git a/psi/zalg.c b/psi/zalg.c
index e2400d9..2677e62 100644
--- a/psi/zalg.c
+++ b/psi/zalg.c
@@ -165,8 +165,10 @@ H6: H = 6;
ref_assign(&op[0], &Rn[j]);
break;
case 6:
-/*H6_cont:*/if (!r_has_type(&op[0], t_boolean))
+ /*H6_cont:*/if (!r_has_type(&op[0], t_boolean)) {
+ esp -= 9;
return_error(gs_error_typecheck);
+ }
if (op[0].value.boolval) {
/* H7: */ ref_assign_old(&arry, &Rn[i], &Rn[j], ".sort(H7)");
goto H4;
@@ -176,6 +178,7 @@ H8: ref_assign_old(&arry, &Rn[i], &R, ".sort(H8)");
}
default:
pop(1);
+ esp -= 9;
return_error(gs_error_unregistered); /* Must not happen. */
}
esp += 2;
diff --git a/psi/zcie.c b/psi/zcie.c
index c3c2cb4..efe2a2f 100644
--- a/psi/zcie.c
+++ b/psi/zcie.c
@@ -455,25 +455,29 @@ ciedefgspace(i_ctx_t *i_ctx_p, ref *CIEDict, ulong dictkey)
push(1); /* Sacrificial */
procs = istate->colorspace[0].procs.cie;
if (pcs == NULL ) {
- if ((code = dict_find_string(CIEDict, "Table", &ptref)) <= 0)
- return (code < 0 ? code : gs_note_error(gs_error_rangecheck));
- check_read_type(*ptref, t_array);
- if (r_size(ptref) != 5)
- return_error(gs_error_rangecheck);
- /* Stable memory due to current caching of color space */
- code = gs_cspace_build_CIEDEFG(&pcs, NULL, mem->stable_memory);
- if (code < 0)
- return code;
- pcie = pcs->params.defg;
- pcie->Table.n = 4;
- pcie->Table.m = 3;
+ if ((code = dict_find_string(CIEDict, "Table", &ptref)) <= 0) {
+ if (code == 0)
+ gs_note_error(cie_set_finish(i_ctx_p, pcs, &procs, edepth, gs_error_rangecheck));
+ else
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
+ }
+ check_read_type(*ptref, t_array);
+ if (r_size(ptref) != 5)
+ return_error(gs_error_rangecheck);
+ /* Stable memory due to current caching of color space */
+ code = gs_cspace_build_CIEDEFG(&pcs, NULL, mem->stable_memory);
+ if (code < 0)
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
+ pcie = pcs->params.defg;
+ pcie->Table.n = 4;
+ pcie->Table.m = 3;
code = cie_cache_push_finish(i_ctx_p, cie_defg_finish, imem, pcie);
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
code = cie_defg_param(i_ctx_p, imemory, CIEDict, pcie, &procs,
&has_abc_procs, &has_lmn_procs, &has_defg_procs,ptref);
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
/* Add the color space to the profile cache */
gsicc_add_cs(igs, pcs,dictkey);
} else {
@@ -561,25 +565,29 @@ ciedefspace(i_ctx_t *i_ctx_p, ref *CIEDict, ulong dictkey)
push(1); /* Sacrificial */
procs = istate->colorspace[0].procs.cie;
if (pcs == NULL ) {
- if ((code = dict_find_string(CIEDict, "Table", &ptref)) <= 0)
- return (code < 0 ? code : gs_note_error(gs_error_rangecheck));
- check_read_type(*ptref, t_array);
- if (r_size(ptref) != 4)
- return_error(gs_error_rangecheck);
- /* Stable memory due to current caching of color space */
- code = gs_cspace_build_CIEDEF(&pcs, NULL, mem->stable_memory);
- if (code < 0)
- return code;
- pcie = pcs->params.def;
- pcie->Table.n = 3;
- pcie->Table.m = 3;
+ if ((code = dict_find_string(CIEDict, "Table", &ptref)) <= 0) {
+ if (code == 0)
+ gs_note_error(cie_set_finish(i_ctx_p, pcs, &procs, edepth, gs_error_rangecheck));
+ else
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
+ }
+ check_read_type(*ptref, t_array);
+ if (r_size(ptref) != 4)
+ return_error(gs_error_rangecheck);
+ /* Stable memory due to current caching of color space */
+ code = gs_cspace_build_CIEDEF(&pcs, NULL, mem->stable_memory);
+ if (code < 0)
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
+ pcie = pcs->params.def;
+ pcie->Table.n = 3;
+ pcie->Table.m = 3;
code = cie_cache_push_finish(i_ctx_p, cie_def_finish, imem, pcie);
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
code = cie_def_param(i_ctx_p, imemory, CIEDict, pcie, &procs,
&has_abc_procs, &has_lmn_procs, &has_def_procs, ptref);
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
/* Add the color space to the profile cache */
gsicc_add_cs(igs, pcs,dictkey);
} else {
@@ -629,15 +637,15 @@ cieabcspace(i_ctx_t *i_ctx_p, ref *CIEDict, ulong dictkey)
/* Stable memory due to current caching of color space */
code = gs_cspace_build_CIEABC(&pcs, NULL, mem->stable_memory);
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
pcie = pcs->params.abc;
code = cie_cache_push_finish(i_ctx_p, cie_abc_finish, imem, pcie);
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
code = cie_abc_param(i_ctx_p, imemory, CIEDict, pcie, &procs,
&has_abc_procs, &has_lmn_procs);
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
/* Set the color space in the graphic state. The ICC profile
will be set later if we actually use the space. Procs will be
sampled now though. Also, the finish procedure is on the stack
@@ -692,16 +700,16 @@ cieaspace(i_ctx_t *i_ctx_p, ref *CIEdict, ulong dictkey)
/* Stable memory due to current caching of color space */
code = gs_cspace_build_CIEA(&pcs, NULL, mem->stable_memory);
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
pcie = pcs->params.a;
code = cie_a_param(imemory, CIEdict, pcie, &procs, &has_a_procs,
&has_lmn_procs);
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
/* Push finalize procedure on the execution stack */
code = cie_cache_push_finish(i_ctx_p, cie_a_finish, (gs_ref_memory_t *)imem, pcie);
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
if (!has_a_procs && !has_lmn_procs) {
pcie->common.caches.DecodeLMN->floats
.params.is_identity = true;
@@ -713,7 +721,7 @@ cieaspace(i_ctx_t *i_ctx_p, ref *CIEdict, ulong dictkey)
code = cie_prepare_iccproc(i_ctx_p, &pcie->RangeA,
&procs.Decode.A, &pcie->caches.DecodeA.floats, pcie, imem, "Decode.A");
if (code < 0)
- return code;
+ return cie_set_finish(i_ctx_p, pcs, &procs, edepth, code);
} else {
pcie->caches.DecodeA.floats.params.is_identity = true;
}
@@ -842,8 +850,10 @@ cie_cache_finish_store(i_ctx_t *i_ctx_p, bool replicate)
code = float_param(ref_stack_index(&o_stack,
(replicate ? 0 : gx_cie_cache_size - 1 - i)),
&pcache->values[i]);
- if (code < 0)
+ if (code < 0) {
+ esp -= 2; /* pop pointer to cache */
return code;
+ }
}
}
#ifdef DEBUG
diff --git a/psi/zcolor1.c b/psi/zcolor1.c
index 56dcf71..1a341ff 100644
--- a/psi/zcolor1.c
+++ b/psi/zcolor1.c
@@ -94,6 +94,7 @@ static int
zsetcolortransfer(i_ctx_t *i_ctx_p)
{
os_ptr op = osp;
+ os_ptr ep = esp;
int code;
check_proc(op[-3]);
@@ -130,8 +131,14 @@ zsetcolortransfer(i_ctx_t *i_ctx_p)
(code = zcolor_remap_one(i_ctx_p, &istate->transfer_procs.gray,
igs->set_transfer.gray, igs,
zcolor_remap_one_finish)) < 0
- )
+ )
+ {
+ /* Return the exec stack to the state it was in before we started the setup
+ * for the transfer function evaluation.
+ */
+ esp = ep;
return code;
+ }
return o_push_estack;
}
diff --git a/psi/zcontrol.c b/psi/zcontrol.c
index ebcd5e3..36da22c 100644
--- a/psi/zcontrol.c
+++ b/psi/zcontrol.c
@@ -239,8 +239,10 @@ end_runandhide(i_ctx_t *i_ctx_p)
{
int code;
- if ((code = runandhide_restore_hidden(i_ctx_p, esp, esp - 1)) < 0)
+ if ((code = runandhide_restore_hidden(i_ctx_p, esp, esp - 1)) < 0) {
+ esp -= 2;
return code;
+ }
esp -= 2; /* pop the hidden value and its atributes */
return o_pop_estack;
}
diff --git a/psi/zfile.c b/psi/zfile.c
index b307840..d6575ea 100644
--- a/psi/zfile.c
+++ b/psi/zfile.c
@@ -425,8 +425,10 @@ file_continue(i_ctx_t *i_ctx_p)
uint len = r_size(pscratch);
uint code;
- if (len < devlen)
+ if (len < devlen) {
+ esp -= 5; /* pop proc, pfen, devlen, iodev , mark */
return_error(gs_error_rangecheck); /* not even room for device len */
+ }
do {
memcpy((char *)pscratch->value.bytes, iodev->dname, devlen);
@@ -435,8 +437,10 @@ file_continue(i_ctx_t *i_ctx_p)
if (code == ~(uint) 0) { /* all done */
esp -= 5; /* pop proc, pfen, devlen, iodev , mark */
return o_pop_estack;
- } else if (code > len) /* overran string */
+ } else if (code > len) { /* overran string */
+ esp -= 5; /* pop proc, pfen, devlen, iodev , mark */
return_error(gs_error_rangecheck);
+ }
else if (iodev != iodev_default(imemory)
|| (check_file_permissions(i_ctx_p, (char *)pscratch->value.bytes, code + devlen, iodev, "PermitFileReading")) == 0) {
push(1);
diff --git a/psi/zht1.c b/psi/zht1.c
index 86f32e8..7265abb 100644
--- a/psi/zht1.c
+++ b/psi/zht1.c
@@ -115,8 +115,11 @@ setcolorscreen_finish(i_ctx_t *i_ctx_p)
pdht->order = pdht->components[0].corder;
code = gx_ht_install(igs, r_ptr(esp - 1, gs_halftone), pdht);
- if (code < 0)
+ if (code < 0) {
+ esp -= 7;
+ setcolorscreen_cleanup(i_ctx_p);
return code;
+ }
istate->screen_procs.red = esp[-5];
istate->screen_procs.green = esp[-4];
istate->screen_procs.blue = esp[-3];
diff --git a/psi/zht2.c b/psi/zht2.c
index 48d7dd3..60bd656 100644
--- a/psi/zht2.c
+++ b/psi/zht2.c
@@ -558,8 +558,11 @@ sethalftone_finish(i_ctx_t *i_ctx_p)
if (pdht->components)
pdht->order = pdht->components[0].corder;
code = gx_ht_install(igs, r_ptr(esp - 1, gs_halftone), pdht);
- if (code < 0)
+ if (code < 0) {
+ esp -= 4;
+ sethalftone_cleanup(i_ctx_p);
return code;
+ }
istate->halftone = esp[-2];
esp -= 4;
sethalftone_cleanup(i_ctx_p);
diff --git a/psi/zpath1.c b/psi/zpath1.c
index e9eca1c..b9fb2e5 100644
--- a/psi/zpath1.c
+++ b/psi/zpath1.c
@@ -232,6 +232,8 @@ path_continue(i_ctx_t *i_ctx_p)
path_cleanup(i_ctx_p);
return o_pop_estack;
default: /* error */
+ esp -= 6;
+ path_cleanup(i_ctx_p);
return code;
case gs_pe_moveto:
esp[2] = esp[-4]; /* moveto proc */
diff --git a/psi/zpcolor.c b/psi/zpcolor.c
index fb601a3..3c3e29e 100644
--- a/psi/zpcolor.c
+++ b/psi/zpcolor.c
@@ -351,8 +351,10 @@ pattern_paint_finish(i_ctx_t *i_ctx_p)
}
pinst = (gs_pattern1_instance_t *)gs_currentcolor(igs->saved)->pattern;
/* If pinst is NULL after all of that then we are not going to recover */
- if (pinst == NULL)
+ if (pinst == NULL) {
+ esp -= 5;
return_error(gs_error_unknownerror);
+ }
}
pgs = igs;
@@ -360,8 +362,10 @@ pattern_paint_finish(i_ctx_t *i_ctx_p)
if (pinst->is_clist) {
/* Send the compositor command to close the PDF14 device */
code = gs_pop_pdf14trans_device(pgs, true);
- if (code < 0)
+ if (code < 0) {
+ esp -= 5;
return code;
+ }
} else {
/* Not a clist, get PDF14 buffer information */
code = pdf14_get_buffer_information(pgs->device,
@@ -369,8 +373,10 @@ pattern_paint_finish(i_ctx_t *i_ctx_p)
true);
/* PDF14 device (and buffer) is destroyed when pattern cache
entry is removed */
- if (code < 0)
+ if (code < 0) {
+ esp -= 5;
return code;
+ }
}
}
code = gx_pattern_cache_add_entry(igs, pdev, &ctile);
--
2.9.1
