| Summary: | Path traversal and command execution due to path reduction | ||
|---|---|---|---|
| Product: | Ghostscript | Reporter: | zhutyra |
| Component: | Security (public) | Assignee: | Chris Liddell (chrisl) <chris.liddell> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | akhaitov, carnil, cbuissar, dr, jsmeix, ken.sharp, marc.deslauriers, rlescak, robin.watts, sam, till.kamppeter |
| Priority: | P2 | ||
| Version: | unspecified | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Customer: | Word Size: | --- | |
| Attachments: | Patch | ||
The patch also include the patch for the bug 707686 (In reply to zhutyra from comment #0) > In the case of a path like "./../tmp/aa" it will first see that it has a cwd > prefix and then reduce it to "../tmp/aa". When validation fails, it tries > the variant without the cwd prefix by skipping the first two characters. But > this will skip the ".." and validate "/tmp/aa", which is an allowed path (on > Linux). I'm not clear why this is a security problem ? Certainly it's an unexpected result, but that's what you get for using confusing paths. We're permitted to write to the directory so you could just as easily have put (/tmp/aa) and got the same result. Nevertheless, the obvious solution (and I think this forms part of your patch, but it's hard to tell) is to check that the buffer does actually contain a CWD prefix, not just that it's large enough to do so, and I'm happy with doing that. (In reply to zhutyra from comment #1) > The patch also include the patch for the bug 707686 Which, unfortunately, makes it harder to understand this patch. I'm not (yet, at least) satisfied with the patch for 707686, I have a different solution which I like better (but I need to put it up to my colleagues for a consensus) and including that patch here makes it unfortunately much harder to follow the proposed patch for this problem. I'm also inclined to approach this problem in a simpler fashion and simply throw an error if there is a device specification anywhere except at the start of a specification. I believe the device specification should always be the first thing in the path and it makes no sense to have it in the middle, nor to have multiple device specifiers. (In reply to Ken Sharp from comment #2) > I'm not clear why this is a security problem ? The path "./foo" and "foo" refer to the same file. Ghostscript follows this convention and will try to find permissions for the path with and without the cwd prefix. "foo" -> "./foo", "./foo" -> "foo". So with the path "./../tmp" it will see the cwd prefix. *Then* it reduces the path to "../tmp" and when it does not find permission for this path, it removes the cwd prefix. Which is no longer there, and the path to validate becomes "/tmp", which succeeds. This is just a validation, the accessed path is still "./../tmp". If you are in "/home/user/something", it will allow access to "/home/user/tmp". The fix is to reduce the path right at the start. > Which, unfortunately, makes it harder to understand this patch. I'm sorry, I forgot I had a previous change in that file as well. It's just one line that says that "../foo" is not in the current directory, so it shouldn't add a cwd prefix and try the "./../foo" path, which would be allowed by "./*". > I'm also inclined to approach this problem in a simpler fashion and simply > throw an error if there is a device specification anywhere except at the > start of a specification. I believe the device specification should always > be the first thing in the path and it makes no sense to have it in the > middle, nor to have multiple device specifiers. I agree, but currently the paths are just strings with an implicit %os% device (and filesystem paths with percent signs are perfectly valid), so I just handled it as best I could. (In reply to zhutyra from comment #3) > This is just a validation, the accessed path is still "./../tmp". If you are > in "/home/user/something", it will allow access to "/home/user/tmp". Aha, right I see what you mean now. > The fix is to reduce the path right at the start. I'm going to have to take this to a Linux machine to test it out, the problem doesn't exist on Windows, at least in general, because the %TEMP% directory includes a drive letter, so the approach you've used on Linux fails. > I'm sorry, I forgot I had a previous change in that file as well. It's just > one line that says that "../foo" is not in the current directory, so it > shouldn't add a cwd prefix and try the "./../foo" path, which would be > allowed by "./*". Indeed, but there are other changes in the file, and because I chose to use a different method for that bug (not treating an initial '../' as a special case) it doesn't match my code, which makes it harder to read. > I agree, but currently the paths are just strings with an implicit %os% > device (and filesystem paths with percent signs are perfectly valid), so I > just handled it as best I could. I think it's neater to prescan the initial 'path' variable to find '|' or '%pipe' anywhere except at the start of the string, and fail if they are. Other PostScript devices can't (I don't think) be abused the way that pipes can, and we don't special case them anywhere else either. If a user messes about sticking PostScript device specifiers in the middle of a string it shouldn't work (and as far as I can tell, doesn't work) because, as you rightly say, the '%' character is just treated as part of the filesystem path. However, this isn't my area of the code (I'm only looking at this due to resource pressure) so I need to get input from everyone else, which is going to take some days. It's entirely possible they'll prefer your approach. It's been a busy few weeks, and it still hasn't settled down yet, so I'm commenting on all the open 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. I have two patches to address the problems here. The first addresses the problem of skipping over the expected './': diff --git a/base/gpmisc.c b/base/gpmisc.c index 48fb5278f..7d102d386 100644 --- a/base/gpmisc.c +++ b/base/gpmisc.c @@ -1165,8 +1165,8 @@ gp_validate_path_len(const gs_memory_t *mem, continue; } - else if (code < 0 && cdirstrl > 0 && prefix_len == 0 && buffer == bufferfull - && memcmp(buffer, cdirstr, cdirstrl) && !memcmp(buffer + cdirstrl, dirsepstr, dirsepstrl)) { + else if (code < 0 && cdirstrl > 0 && prefix_len == 0 && buffer == bufferfull) { + buffer = bufferfull + cdirstrl + dirsepstrl; continue; } break; The second addresses the more serious issue of %pipe% or '|' combined with the './' permission : diff --git a/base/gpmisc.c b/base/gpmisc.c index 7d102d386..ca8cb0a71 100644 --- a/base/gpmisc.c +++ b/base/gpmisc.c @@ -1091,6 +1091,27 @@ gp_validate_path_len(const gs_memory_t *mem, rlen = len; } else { + char *test = (char *)path, *test1; + uint tlen = len, slen; + + /* Look for any pipe (%pipe% or '|' specifications between path separators + * Reject any path spec which has a %pipe% or '|' anywhere except at the start. + */ + while (tlen > 0) { + if (test[0] == '|' || (tlen > 5 && memcmp(test, "%pipe", 5) == 0)) { + code = gs_note_error(gs_error_invalidfileaccess); + goto exit; + } + test1 = test; + slen = search_separator((const char **)&test, path + len, test1, 1); + if(slen == 0) + break; + test += slen; + tlen -= test - test1; + if (test >= path + len) + break; + } + rlen = len+1; bufferfull = (char *)gs_alloc_bytes(mem->thread_safe_memory, rlen + prefix_len, "gp_validate_path"); if (bufferfull == NULL) @@ -1165,8 +1186,8 @@ gp_validate_path_len(const gs_memory_t *mem, continue; } - else if (code < 0 && cdirstrl > 0 && prefix_len == 0 && buffer == bufferfull) { - buffer = bufferfull + cdirstrl + dirsepstrl; + else if (code < 0 && cdirstrl > 0 && prefix_len == 0 && buffer == bufferfull + && memcmp(buffer, cdirstr, cdirstrl) && !memcmp(buffer + cdirstrl, dirsepstr, dirsepstrl)) { continue; } break; Yes, I think this should fix it. CVE-2024-33869 |
Created attachment 25518 [details] Patch The function "gp_validate_path_len" first tests if the path has a cwd prefix and then reduces the path. In the case of a path like "./../tmp/aa" it will first see that it has a cwd prefix and then reduce it to "../tmp/aa". When validation fails, it tries the variant without the cwd prefix by skipping the first two characters. But this will skip the ".." and validate "/tmp/aa", which is an allowed path (on Linux). Testcase for Linux: ``` $ mkdir tmp test $ cd test $ gs -q -c '(./../tmp/test) (w) file dup (hello\n) writestring closefile quit' $ cat ../tmp/test hello ``` --- The reduction of paths itself has a shortcoming too. A path like "aa/../%dev%bb" will be reduced to "%dev%bb" which may have unexpected results. For example, when using the "tiffsep" device with the output file "aa/../%pipe%command#", the file name will be reduced to "%pipe%command#". It's still just a file. But then, when creating the separation file, the file "%pipe%command#(Cmyk).tif" is added to the permitted paths and will allow the use of %pipe. Testcase for Linux: The assumption is that "./" is writable and contains another directory. There may be other ways to abuse it, but this is the most obvious one. ``` $ mkdir travers $ gs -q --permit-file-write=./ -c '<< /OutputDevice /tiffsep /OutputFile (travers/../%%pipe%%id;#) >> setpagedevice showpage (%pipe%id;#(Cyan).tif) (w) file quit' uid=1000(user) gid=1000(user) groups=1000(user) ``` --- In the attached patch I modified mainly the "gp_file_name_reduce" function so that paths do not change type. Maybe it could be directly in "gp_file_name_combine", but I didn't want to change that one. So now it works like this: "%dev%aa/../bb" --> "%dev%bb" (previously "bb") "aa/../%dev%bb" --> "./%dev%bb" (previously "%dev%bb") It's not as many changes as it seems from the diff :)