From: Chris Liddell Date: Tue, 21 Aug 2018 19:17:05 +0000 (+0100) Subject: Bug 699657: properly apply file permissions to .tempfile Bug 699657: properly apply file permissions to .tempfile https://git.ghostscript.com/?p=ghostpdl.git;a=commit;h=0d3901189f245232f0161addf215d7268c4d05a3 From: Chris Liddell Date: Tue, 21 Aug 2018 19:17:51 +0000 (+0100) Subject: Bug 699658: Fix handling of pre-SAFER opened files. Bug 699658: Fix handling of pre-SAFER opened files. Temp files opened for writing before SAFER is engaged are not subject to the SAFER restrictions - that is handled by recording in a dictionary, and checking that as part of the permissions checks. By adding a custom error handler for invalidaccess, that allowed the filename to be added to the dictionary (despite the attempted open throwing the error) thus meaning subsequent accesses were erroneously permitted. https://git.ghostscript.com/?p=ghostpdl.git;a=commit;h=a054156d425b4dbdaaa9fda4b5f1182b27598c2b --- diff -up a/psi/zfile.c.cve-2018-15908 b/psi/zfile.c --- a/psi/zfile.c.cve-2018-15908 2018-11-14 15:13:31.249625819 +0100 +++ b/psi/zfile.c 2018-11-14 15:14:16.933831779 +0100 @@ -121,7 +121,7 @@ make_invalid_file(i_ctx_t *i_ctx_p, ref /* strings of the permitgroup array. */ static int check_file_permissions_reduced(i_ctx_t *i_ctx_p, const char *fname, int len, - const char *permitgroup) + gx_io_device *iodev, const char *permitgroup) { long i; ref *permitlist = NULL; @@ -131,8 +131,14 @@ check_file_permissions_reduced(i_ctx_t * bool use_windows_pathsep = (gs_file_name_check_separator(win_sep2, 1, win_sep2) == 1); uint plen = gp_file_name_parents(fname, len); - /* Assuming a reduced file name. */ + /* we're protecting arbitrary file system accesses, not Postscript device accesses. + * Although, note that %pipe% is explicitly checked for and disallowed elsewhere + */ + if (iodev && iodev != iodev_default(imemory)) { + return 0; + } + /* Assuming a reduced file name. */ if (dict_find_string(&(i_ctx_p->userparams), permitgroup, &permitlist) <= 0) return 0; /* if Permissions not found, just allow access */ @@ -187,14 +193,14 @@ check_file_permissions_reduced(i_ctx_t * /* strings of the permitgroup array */ static int check_file_permissions(i_ctx_t *i_ctx_p, const char *fname, int len, - const char *permitgroup) + gx_io_device *iodev, const char *permitgroup) { char fname_reduced[gp_file_name_sizeof]; uint rlen = sizeof(fname_reduced); if (gp_file_name_reduce(fname, len, fname_reduced, &rlen) != gp_combine_success) return e_invalidaccess; /* fail if we couldn't reduce */ - return check_file_permissions_reduced(i_ctx_p, fname_reduced, rlen, permitgroup); + return check_file_permissions_reduced(i_ctx_p, fname_reduced, rlen, iodev, permitgroup); } /* file */ @@ -298,7 +304,7 @@ zdeletefile(i_ctx_t *i_ctx_p) return code; if (pname.iodev == iodev_default(imemory)) { if ((code = check_file_permissions(i_ctx_p, pname.fname, pname.len, - "PermitFileControl")) < 0 && + pname.iodev, "PermitFileControl")) < 0 && !file_is_tempfile(i_ctx_p, op->value.bytes, r_size(op))) { return code; } @@ -382,7 +388,7 @@ file_continue(i_ctx_t *i_ctx_p) } else if (code > len) /* overran string */ return_error(gs_error_rangecheck); else if (iodev != iodev_default(imemory) - || (check_file_permissions_reduced(i_ctx_p, (char *)pscratch->value.bytes, code + devlen, "PermitFileReading")) == 0) { + || (check_file_permissions_reduced(i_ctx_p, (char *)pscratch->value.bytes, code + devlen, NULL, "PermitFileReading")) == 0) { push(1); ref_assign(op, pscratch); r_set_size(op, code + devlen); @@ -432,12 +438,12 @@ zrenamefile(i_ctx_t *i_ctx_p) * and FileWriting permissions to the destination file/path. */ ((check_file_permissions(i_ctx_p, pname1.fname, pname1.len, - "PermitFileControl") < 0 && + pname1.iodev, "PermitFileControl") < 0 && !file_is_tempfile(i_ctx_p, op[-1].value.bytes, r_size(op - 1))) || (check_file_permissions(i_ctx_p, pname2.fname, pname2.len, - "PermitFileControl") < 0 || + pname2.iodev, "PermitFileControl") < 0 || check_file_permissions(i_ctx_p, pname2.fname, pname2.len, - "PermitFileWriting") < 0 )))) { + pname2.iodev, "PermitFileWriting") < 0 )))) { code = gs_note_error(e_invalidfileaccess); } else { code = (*pname1.iodev->procs.rename_file)(pname1.iodev, @@ -484,8 +490,11 @@ zstatus(i_ctx_t *i_ctx_p) code = gs_terminate_file_name(&pname, imemory, "status"); if (code < 0) return code; - code = (*pname.iodev->procs.file_status)(pname.iodev, + if ((code = check_file_permissions(i_ctx_p, pname.fname, pname.len, + pname.iodev, "PermitFileReading")) >= 0) { + code = (*pname.iodev->procs.file_status)(pname.iodev, pname.fname, &fstat); + } switch (code) { case 0: check_ostack(4); @@ -694,8 +703,24 @@ ztempfile(i_ctx_t *i_ctx_p) } if (gp_file_name_is_absolute(pstr, strlen(pstr))) { - if (check_file_permissions(i_ctx_p, pstr, strlen(pstr), - "PermitFileWriting") < 0) { + int plen = strlen(pstr); + const char *sep = gp_file_name_separator(); +#ifdef DEBUG + int seplen = strlen(sep); + if (seplen != 1) + return_error(gs_error_Fatal); +#endif + /* strip off the file name prefix, leave just the directory name + * so we can check if we are allowed to write to it + */ + for ( ; plen >=0; plen--) { + if (pstr[plen] == sep[0]) + break; + } + memcpy(fname, pstr, plen); + fname[plen] = '\0'; + if (check_file_permissions(i_ctx_p, fname, strlen(fname), + NULL, "PermitFileWriting") < 0) { return_error(e_invalidfileaccess); } } else if (!prefix_is_simple(pstr)) { @@ -837,6 +862,7 @@ zopen_file(i_ctx_t *i_ctx_p, const gs_pa const char *file_access, stream **ps, gs_memory_t *mem) { gx_io_device *const iodev = pfn->iodev; + int code = 0; if (pfn->fname == NULL) /* just a device */ return iodev->procs.open_device(iodev, file_access, ps, mem); @@ -847,7 +873,7 @@ zopen_file(i_ctx_t *i_ctx_p, const gs_pa open_file = iodev_os_open_file; /* Check OS files to make sure we allow the type of access */ if (open_file == iodev_os_open_file) { - int code = check_file_permissions(i_ctx_p, pfn->fname, pfn->len, + code = check_file_permissions(i_ctx_p, pfn->fname, pfn->len, pfn->iodev, file_access[0] == 'r' ? "PermitFileReading" : "PermitFileWriting"); if (code < 0 && !file_is_tempfile(i_ctx_p, @@ -894,7 +920,7 @@ check_file_permissions_aux(i_ctx_t *i_ct /* fname must be reduced. */ if (i_ctx_p == NULL) return 0; - if (check_file_permissions_reduced(i_ctx_p, fname, flen, "PermitFileReading") < 0) + if (check_file_permissions_reduced(i_ctx_p, fname, flen, NULL, "PermitFileReading") < 0) return_error(e_invalidfileaccess); return 0; } diff -up a/Resource/Init/gs_init.ps.cve-2018-15908 b/Resource/Init/gs_init.ps --- a/Resource/Init/gs_init.ps.cve-2018-15908 2018-11-14 16:34:23.268867657 +0100 +++ b/Resource/Init/gs_init.ps 2018-11-14 16:36:38.765552576 +0100 @@ -2015,6 +2015,19 @@ readonly def concatstrings concatstrings .generate_dir_list_templates } if ] + /PermitFileWriting [ + currentuserparams /PermitFileWriting get aload pop + (TMPDIR) getenv not + { + (TEMP) getenv not + { + (TMP) getenv not + { + (/temp) (/tmp) + } if + } if + } if + ] /LockFilePermissions //true >> setuserparams } @@ -2062,7 +2075,9 @@ readonly def % the file can be deleted later, even if SAFER is set. /.tempfile { .tempfile % filename file - //SAFETY /tempfiles get 2 .argindex //true .forceput + //SAFETY /safe get not { % only add the filename if we're not yet safe + //SAFETY /tempfiles get 2 .argindex //true .forceput + } if } .bind executeonly odef % If we are running in SAFER mode, lock things down