From 6f244ecef4a740b3b2dde15303b13a93a83706c1 Mon Sep 17 00:00:00 2001 From: Chris Liddell Date: Wed, 7 Jun 2023 10:23:06 +0100 Subject: [PATCH] Bug 706761: Don't "reduce" %pipe% file names for permission validation For regular file names, we try to simplfy relative paths before we use them. Because the %pipe% device can, effectively, accept command line calls, we shouldn't be simplifying that string, because the command line syntax can end up confusing the path simplifying code. That can result in permitting a pipe command which does not match what was originally permitted. Special case "%pipe" in the validation code so we always deal with the entire string. Bug 706778: 706761 revisit Two problems with the original commit. The first a silly typo inverting the logic of a test. The second was forgetting that we actually actually validate two candidate strings for pipe devices. One with the expected "%pipe%" prefix, the other using the pipe character prefix: "|". This addresses both those. --- CVE: CVE-2023-36664 Upstream-Status: Backport [see text] From git://git.ghostscript.com/ghostpdl commit 5e65eeae225c7d02d447de5abaf4a8e6d234fcea commit fb342fdb60391073a69147cb71af1ac416a81099 The second commit fixes errors in the first one, so we combine them. Signed-off-by: Joe Slater --- base/gpmisc.c | 31 +++++++++++++++++++-------- base/gslibctx.c | 56 ++++++++++++++++++++++++++++++++++++------------- 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/base/gpmisc.c b/base/gpmisc.c index 3d878ac..f9a9230 100644 --- a/base/gpmisc.c +++ b/base/gpmisc.c @@ -1076,16 +1076,29 @@ gp_validate_path_len(const gs_memory_t *mem, && !memcmp(path + cdirstrl, dirsepstr, dirsepstrl)) { prefix_len = 0; } - rlen = len+1; - bufferfull = (char *)gs_alloc_bytes(mem->thread_safe_memory, rlen + prefix_len, "gp_validate_path"); - if (bufferfull == NULL) - return gs_error_VMerror; - - buffer = bufferfull + prefix_len; - if (gp_file_name_reduce(path, (uint)len, buffer, &rlen) != gp_combine_success) - return gs_error_invalidfileaccess; - buffer[rlen] = 0; + /* "%pipe%" do not follow the normal rules for path definitions, so we + don't "reduce" them to avoid unexpected results + */ + if (path[0] == '|' || (len > 5 && memcmp(path, "%pipe", 5) == 0)) { + bufferfull = buffer = (char *)gs_alloc_bytes(mem->thread_safe_memory, len + 1, "gp_validate_path"); + if (buffer == NULL) + return gs_error_VMerror; + memcpy(buffer, path, len); + buffer[len] = 0; + rlen = len; + } + else { + rlen = len+1; + bufferfull = (char *)gs_alloc_bytes(mem->thread_safe_memory, rlen + prefix_len, "gp_validate_path"); + if (bufferfull == NULL) + return gs_error_VMerror; + + buffer = bufferfull + prefix_len; + if (gp_file_name_reduce(path, (uint)len, buffer, &rlen) != gp_combine_success) + return gs_error_invalidfileaccess; + buffer[rlen] = 0; + } while (1) { switch (mode[0]) { diff --git a/base/gslibctx.c b/base/gslibctx.c index 1862482..8bfe4bb 100644 --- a/base/gslibctx.c +++ b/base/gslibctx.c @@ -740,14 +740,28 @@ gs_add_control_path_len_flags(const gs_memory_t *mem, gs_path_control_t type, co return gs_error_rangecheck; } - rlen = len+1; - buffer = (char *)gs_alloc_bytes(core->memory, rlen, "gp_validate_path"); - if (buffer == NULL) - return gs_error_VMerror; + /* "%pipe%" do not follow the normal rules for path definitions, so we + don't "reduce" them to avoid unexpected results + */ + if (path[0] == '|' || (len > 5 && memcmp(path, "%pipe", 5) == 0)) { + buffer = (char *)gs_alloc_bytes(core->memory, len + 1, "gs_add_control_path_len"); + if (buffer == NULL) + return gs_error_VMerror; + memcpy(buffer, path, len); + buffer[len] = 0; + rlen = len; + } + else { + rlen = len + 1; - if (gp_file_name_reduce(path, (uint)len, buffer, &rlen) != gp_combine_success) - return gs_error_invalidfileaccess; - buffer[rlen] = 0; + buffer = (char *)gs_alloc_bytes(core->memory, rlen, "gs_add_control_path_len"); + if (buffer == NULL) + return gs_error_VMerror; + + if (gp_file_name_reduce(path, (uint)len, buffer, &rlen) != gp_combine_success) + return gs_error_invalidfileaccess; + buffer[rlen] = 0; + } n = control->num; for (i = 0; i < n; i++) @@ -833,14 +847,28 @@ gs_remove_control_path_len_flags(const gs_memory_t *mem, gs_path_control_t type, return gs_error_rangecheck; } - rlen = len+1; - buffer = (char *)gs_alloc_bytes(core->memory, rlen, "gp_validate_path"); - if (buffer == NULL) - return gs_error_VMerror; + /* "%pipe%" do not follow the normal rules for path definitions, so we + don't "reduce" them to avoid unexpected results + */ + if (path[0] == '|' || (len > 5 && memcmp(path, "%pipe", 5) == 0)) { + buffer = (char *)gs_alloc_bytes(core->memory, len + 1, "gs_remove_control_path_len"); + if (buffer == NULL) + return gs_error_VMerror; + memcpy(buffer, path, len); + buffer[len] = 0; + rlen = len; + } + else { + rlen = len+1; - if (gp_file_name_reduce(path, (uint)len, buffer, &rlen) != gp_combine_success) - return gs_error_invalidfileaccess; - buffer[rlen] = 0; + buffer = (char *)gs_alloc_bytes(core->memory, rlen, "gs_remove_control_path_len"); + if (buffer == NULL) + return gs_error_VMerror; + + if (gp_file_name_reduce(path, (uint)len, buffer, &rlen) != gp_combine_success) + return gs_error_invalidfileaccess; + buffer[rlen] = 0; + } n = control->num; for (i = 0; i < n; i++) { -- 2.35.5