From 1d709e5e27089e4e899419899214a8b3183a86d9 Mon Sep 17 00:00:00 2001 From: Yamagi Burmeister Date: Tue, 27 Oct 2015 17:38:28 +0100 Subject: [PATCH] Move file name check to prevent spurious "refusing to download messages Moving the check under the "do we have the file localy" check prevents spurious "Refusing to download a path with .." messages with some game data. The tank commander skin is one example. This change has no security impact since FS_LoadFile() just opens and closes the file. While at it tighten the condition to prevent pathes with colons (this condition is added at the server side, too) and pathes starting with slashes and dots. --- src/client/cl_download.c | 14 ++++++-------- src/server/sv_user.c | 2 +- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/client/cl_download.c b/src/client/cl_download.c index fbd3e9ed..ab43a19f 100644 --- a/src/client/cl_download.c +++ b/src/client/cl_download.c @@ -445,14 +445,6 @@ CL_CheckOrDownloadFile(char *filename) char name[MAX_OSPATH]; char *ptr; - // FIXME: we should probably also forbid paths starting with '/' or '\\' or "C:\" - // (or any other drive name) because in the end FS_LoadFile() will fallback to fopen()! - if (strstr(filename, "..")) - { - Com_Printf("Refusing to download a path with ..: %s\n", filename); - return true; - } - /* fix backslashes - this is mostly für UNIX comaptiblity */ while ((ptr = strchr(filename, '\\'))) { @@ -465,6 +457,12 @@ CL_CheckOrDownloadFile(char *filename) return true; } + if (strstr(filename, "..") || strstr(name, ":") || (*name == '.') || (*name == '/')) + { + Com_Printf("Refusing to download a path with ..: %s\n", filename); + return true; + } + strcpy(cls.downloadname, filename); /* download to a temp name, and only rename diff --git a/src/server/sv_user.c b/src/server/sv_user.c index 12336c14..791ad3d3 100644 --- a/src/server/sv_user.c +++ b/src/server/sv_user.c @@ -308,7 +308,7 @@ SV_BeginDownload_f(void) /* hacked by zoid to allow more conrol over download first off, no .. or global allow check */ - if (strstr(name, "..") || strstr(name, "\\") || !allow_download->value + if (strstr(name, "..") || strstr(name, "\\") || strstr(name, ":") || !allow_download->value /* leading dot is no good */ || (*name == '.') /* leading slash bad as well, must be in subdir */