71475dc7c4a4a603ed7334eb4d27fbda8abdbdec braney Tue Sep 8 17:54:23 2026 -0700 cart: leave the trash directory out of the symlink-resolving path check Code review of 67700f0c574 found that resolving trashDir() does more than widen a check. trashDir() is the relative "../trash", so its resolved spelling is a different string, and sessionDataPathFromTrash() substitutes exactly the relative spelling. A path accepted as trash in its resolved spelling therefore rewrites to itself, and the callers treat the two as two files: saveTrackFile() opens the old one for reading and truncates the new one for writing, then unlinks the old name and points it at itself, and sessionDataSaveTrashFile() unlinks the file and then aborts when link() fails. Since ctfile_<db>, customComposite-<db> and hubQuickLift-<db> reach saveTrackFile() from a cart value, a session save could have destroyed another session's file. Resolving a relative directory also resolves it against the working directory of the process, so the answer moved with the caller: on the command line trashDir() is $JKTRASH or ".", which made anything under $TMPDIR or under cgi-bin count as trash. Put isTrashPath() back on the plain string compare and keep the resolving version for sessionDataDir, sessionDataDirOld and myVariantsDataDir, which are the configured absolute directories the #38303 sessions are stored under. Only resolve an absolute directory, so the result no longer depends on the working directory. All 297,487 saved sessions in the September dumps of the RR, hgwbeta and hgwdev centrals were checked: none holds a resolved trash path, so nothing loses the widening. refs #38303, refs #38304, refs #37623 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> diff --git src/hg/lib/trashDir.c src/hg/lib/trashDir.c index 8b1d9967939..2d99ad1084b 100644 --- src/hg/lib/trashDir.c +++ src/hg/lib/trashDir.c @@ -1,213 +1,223 @@ /* trashDir.c - temporary file creation and directory creation in /trash */ /* Copyright (C) 2014 The Regents of the University of California * See kent/LICENSE or http://genome.ucsc.edu/license/ for licensing information. */ #include <limits.h> #include "common.h" #include "hash.h" #include "hgConfig.h" #include "portable.h" #include "trashDir.h" static boolean hasDotDotComponent(char *path) /* Return TRUE if any '/'-separated component of path is exactly "..", which is the only * way a path can climb back above a directory it appears to be inside of. */ { char *s = path; while (s != NULL && s[0] != '\0') { if (s[0] == '.' && s[1] == '.' && (s[2] == '/' || s[2] == '\0')) return TRUE; s = strchr(s, '/'); if (s != NULL) s += 1; } return FALSE; } boolean pathIsUnderDir(char *dir, char *path) /* Return TRUE if path names something underneath dir. A '/' is required at the directory * boundary, so a sibling directory whose name merely starts the same way (trashBackup next * to trash) does not match. ".." below the boundary is refused. */ { if (isEmpty(dir) || isEmpty(path)) return FALSE; int dirLen = strlen(dir); while (dirLen > 0 && dir[dirLen-1] == '/') dirLen -= 1; if (dirLen == 0) return FALSE; if (strncmp(path, dir, dirLen) != 0 || path[dirLen] != '/' || path[dirLen+1] == '\0') return FALSE; return !hasDotDotComponent(path + dirLen + 1); } static boolean pathIsUnderDirOrItsTarget(char *dir, char *path) /* pathIsUnderDir(), but also accept a path under the directory that dir resolves to. A * configured directory is often reached through a symlink, and sessionData.c stores the * resolved spelling of a path whose file is already a symlink, so both spellings turn up in - * saved sessions. Only dir is resolved. Resolving path would defeat the check, because a - * trash file is often a symlink into session storage on purpose. */ + * saved sessions. + * + * Only dir is resolved. Resolving path would defeat the check, because a trash file is + * often a symlink into session storage on purpose. Only an absolute dir is resolved, so + * the answer cannot depend on the working directory of the process. + * + * Only this one direction is covered: a dir configured as the already-resolved spelling + * does not accept a path written through the symlink. + * + * Do not use this on trashDir(). It is a relative path, "../trash", and + * sessionDataPathFromTrash() substitutes exactly that spelling, so accepting the resolved + * spelling here would hand that function a path it cannot rewrite. */ { if (pathIsUnderDir(dir, path)) return TRUE; -if (isEmpty(dir)) +if (isEmpty(dir) || dir[0] != '/') return FALSE; char resolved[PATH_MAX]; if (realpath(dir, resolved) == NULL) return FALSE; return pathIsUnderDir(resolved, path); } boolean isTrashPath(char *path) /* Return TRUE if path names a file inside the trash directory. */ { -return pathIsUnderDirOrItsTarget(trashDir(), path); +return pathIsUnderDir(trashDir(), path); } boolean isTrashOrSessionDataPath(char *path) /* Return TRUE if path is under the trash directory, or under one of the durable session-data * directories that trash files are moved to when a session is saved. */ { return isTrashPath(path) || pathIsUnderDirOrItsTarget(cfgOption("sessionDataDir"), path) || pathIsUnderDirOrItsTarget(cfgOption("sessionDataDirOld"), path); } boolean isServerUserFilePath(char *path) /* Return TRUE if path is under one of the directories where the server keeps files it made * for a user: the trash directory, the session-data directories, or a per-feature data * directory such as myVariantsDataDir. */ { return isTrashOrSessionDataPath(path) || pathIsUnderDirOrItsTarget(cfgOption("myVariantsDataDir"), path); } boolean isRemoteUrl(char *path) /* Return TRUE if path is a URL fetched over the network rather than a file name. Only the * three protocols the tree actually fetches count; hasProtocol() in net.c is a test for * "://" anywhere in the string, which is too loose to decide anything on. */ { return startsWith("http://", path) || startsWith("https://", path) || startsWith("ftp://", path); } boolean isServerUserFileOrUrl(char *path) /* Return TRUE if path is either a remote URL or a file the server made for a user. * * A few cart variables legitimately hold either one: the user gives hgTracks a URL for the * multi-region BED or pastes the BED itself, and hgSession loads settings from a URL. The * code then decides which it has by looking for a protocol, and treats anything else as a * local file name, so "no protocol" has to mean "one of ours" or the local-file branch reads * whatever the cart says. */ { return isRemoteUrl(path) || isServerUserFilePath(path); } static void trashDirFileExt(struct tempName *tn, char *dirName, char *base, char *suffix, boolean addDate) /* obtain a trash file name trash/dirName/base*.suffix */ { static struct hash *dirHash = NULL; char prefix[128]; char buffer[4096]; if (! dirHash) dirHash = newHash(0); /* already created this directory ? */ if (! hashLookup(dirHash,dirName)) { hashAddInt(dirHash, dirName, 1); /* remember, been here, done that */ mkdirTrashDirectory(dirName); } if (addDate) { safef(buffer, sizeof buffer, "%s/%03d", dirName, dayOfYear()); dirName = buffer; if (! hashLookup(dirHash,dirName)) { hashAddInt(dirHash, dirName, 1); /* remember, been here, done that */ mkdirTrashDirectory(dirName); } } /* no need to duplicate the _ at the end of base, makeTempName is going * to add _ to the given base, some CGIs pass "base_" */ if (endsWith(base,"_")) { char *t = cloneString(base); int len = strlen(t); t[len-1] = '\0'; /* remove ending _ */ safef(prefix, sizeof(prefix), "%s/%s", dirName,t); freeMem(t); } else safef(prefix, sizeof(prefix), "%s/%s", dirName,base); makeTempName(tn, prefix, suffix); } void trashDirFile(struct tempName *tn, char *dirName, char *base, char *suffix) /* obtain a trash file name trash/dirName/base*.suffix */ { trashDirFileExt(tn, dirName, base, suffix, FALSE); } void trashDirDateFile(struct tempName *tn, char *dirName, char *base, char *suffix) /* obtain a trash file name trash/dirName.dayOfYear/base*.suffix */ { trashDirFileExt(tn, dirName, base, suffix, TRUE); } boolean trashDirReusableFile(struct tempName *tn, char *dirName, char *base, char *suffix) /* obtain a resusable trash file name as trash/dirName/base.suffix * returns TRUE if already exists. */ { trashDirFile(tn,dirName,base,suffix); // Don't really want the randomized name. char *cgiName = rStringIn("/",tn->forCgi ); char *htmlName = rStringIn("/",tn->forHtml); if (cgiName == NULL) cgiName = rStringIn("\\",tn->forCgi); assert(cgiName != NULL && htmlName != NULL); cgiName += 1; htmlName += 1; boolean addDot = (*suffix != '.'); safef(cgiName, strlen(cgiName), "%s%s%s", base, (addDot?".":""),suffix);// There is room, since safef(htmlName,strlen(htmlName),"%s%s%s", base, (addDot?".":""),suffix);// tempName: base_*.suffix // exists? return fileExists(tn->forCgi); } void copyFileToTrash(char **pFileName, char *dirName, char *base, char *suffix) /* If *pFileName is not NULL and exists, then create a new file in the * given dirName of trash/ with the given base and suffix, copy *pFileName's * contents to it, and set *pFileName to the new filename. */ { if (pFileName != NULL && *pFileName != NULL) { if (fileExists(*pFileName)) { FILE *fIn = mustOpen(*pFileName, "r"); struct tempName tn; trashDirFile(&tn, dirName, base, suffix); char *newFileName = tn.forCgi; FILE *fOut = mustOpen(newFileName, "w"); unsigned char buf[16 * 1024]; size_t sz; while ((sz = fread(buf, sizeof(buf[0]), ArraySize(buf), fIn)) > 0) fwrite(buf, sizeof(buf[0]), sz, fOut); fclose(fOut); fclose(fIn); *pFileName = cloneString(newFileName); } } }