From 61687fff0cb96aa7564e5de76f427f28dbec4a85 Mon Sep 17 00:00:00 2001 From: Jonathan Gray Date: Tue, 7 May 2013 16:39:08 +1000 Subject: [PATCH] CVE-2011-2764/CVE-2011-3012 check for dangerous file extensions CVE-2011-2764 The FS_CheckFilenameIsNotExecutable function in qcommon/files.c in the ioQuake3 engine 1.36 and earlier, as used in World of Padman, Smokin' Guns, OpenArena, Tremulous, and ioUrbanTerror, does not properly determine dangerous file extensions, which allows remote attackers to execute arbitrary code via a crafted third-party addon that creates a Trojan horse DLL file. CVE-2011-3012 The ioQuake3 engine, as used in World of Padman 1.2 and earlier, Tremulous 1.1.0, and ioUrbanTerror 2007-12-20, does not check for dangerous file extensions before writing to the quake3 directory, which allows remote attackers to execute arbitrary code via a crafted third-party addon that creates a Trojan horse DLL file, a different vulnerability than CVE-2011-2764. bugzilla #3695 from Tim Angus in ioquake3 svn 1405 git 2c0861c1cea44861c5ceba2dc39e601d6bc3f0af * (bug 3695) Not allowing to write file with lib extention (.dll/.so/...) (TsT ) from Tim Angus in ioquake3 svn 1499 git 48d8c8876b6ec035b0bb85f4d3c47c9210c3ca30 * s/FS_FilenameIsExecutable/FS_CheckFilenameIsNotExecutable/g * Fix potential buffer under run in FS_CheckFilenameIsNotExecutable from Thilo Schulz in ioquake3 svn 2098 git c4f739b8d03ca203435744c4a96e3561863ccdfe Fix extension name comparison for DLL files from Zack Middleton in ioquake3 git 6c88bf8aeee3c1e5449682f874f91e86cb393ef4 Rename FS_CheckFilenameIsNotExecutable to ..NotImmutable from Harley Laue in ioquake3 git 1b2a6abed996b43eb108486abbda449b3d16e019 Rename FS_CheckFilenameIsNotImmutable to ..IsMutable --- codemp/client/cl_parse.cpp | 2 +- codemp/game/q_shared.c | 40 +++++++++++++++++++++++++++++++++++++ codemp/game/q_shared.h | 2 ++ codemp/qcommon/files_pc.cpp | 34 ++++++++++++++++++++++++++++++- codemp/qcommon/qcommon.h | 2 +- 5 files changed, 77 insertions(+), 3 deletions(-) diff --git a/codemp/client/cl_parse.cpp b/codemp/client/cl_parse.cpp index d267eb0..2905556 100644 --- a/codemp/client/cl_parse.cpp +++ b/codemp/client/cl_parse.cpp @@ -773,7 +773,7 @@ void CL_ParseDownload ( msg_t *msg ) { clc.download = 0; // rename the file - FS_SV_Rename ( clc.downloadTempName, clc.downloadName ); + FS_SV_Rename ( clc.downloadTempName, clc.downloadName, qfalse ); } *clc.downloadTempName = *clc.downloadName = 0; Cvar_Set( "cl_downloadName", "" ); diff --git a/codemp/game/q_shared.c b/codemp/game/q_shared.c index 1356a1d..deb3e5f 100644 --- a/codemp/game/q_shared.c +++ b/codemp/game/q_shared.c @@ -91,6 +91,21 @@ char *COM_SkipPath (char *pathname) return last; } +/* +============ +COM_GetExtension +============ +*/ +const char *COM_GetExtension( const char *name ) +{ + const char *dot = strrchr(name, '.'), *slash; + if (dot && (!(slash = strrchr(name, '/')) || slash < dot)) + return dot + 1; + else + return ""; +} + + /* ============ COM_StripExtension @@ -105,6 +120,31 @@ void COM_StripExtension( const char *in, char *out, int destsize ) { Q_strncpyz(out, in, destsize); } +/* +============ +COM_CompareExtension + +string compare the end of the strings and return qtrue if strings match +============ +*/ +qboolean COM_CompareExtension(const char *in, const char *ext) +{ + int inlen, extlen; + + inlen = strlen(in); + extlen = strlen(ext); + + if(extlen <= inlen) + { + in += inlen - extlen; + + if(!Q_stricmp(in, ext)) + return qtrue; + } + + return qfalse; +} + /* ================== COM_DefaultExtension diff --git a/codemp/game/q_shared.h b/codemp/game/q_shared.h index a2e08f4..acc5988 100644 --- a/codemp/game/q_shared.h +++ b/codemp/game/q_shared.h @@ -1691,7 +1691,9 @@ int Com_Clampi( int min, int max, int value ); //rwwRMG - added float Com_Clamp( float min, float max, float value ); char *COM_SkipPath( char *pathname ); +const char *COM_GetExtension( const char *name ); void COM_StripExtension( const char *in, char *out, int destsize ); +qboolean COM_CompareExtension(const char *in, const char *ext); void COM_DefaultExtension( char *path, int maxSize, const char *extension ); void COM_BeginParseSession( const char *name ); diff --git a/codemp/qcommon/files_pc.cpp b/codemp/qcommon/files_pc.cpp index 68fdeb6..698aff4 100644 --- a/codemp/qcommon/files_pc.cpp +++ b/codemp/qcommon/files_pc.cpp @@ -156,6 +156,26 @@ qboolean FS_CreatePath (char *OSPath) { return qfalse; } +/* +================= +FS_CheckFilenameIsMutable + +ERR_FATAL if trying to maniuplate a file with the platform library, QVM, or pk3 extension +================= + */ +static void FS_CheckFilenameIsMutable( const char *filename, + const char *function ) +{ + // Check if the filename ends with the library, QVM, or pk3 extension + if( COM_CompareExtension( filename, DLL_EXT ) + || COM_CompareExtension( filename, ".qvm" ) + || COM_CompareExtension( filename, ".pk3" ) ) + { + Com_Error( ERR_FATAL, "%s: Not allowed to manipulate '%s' due " + "to %s extension", function, filename, COM_GetExtension( filename ) ); + } +} + /* ================= FS_CopyFile @@ -211,6 +231,8 @@ FS_Remove =========== */ void FS_Remove( const char *osPath ) { + FS_CheckFilenameIsMutable( osPath, __func__ ); + remove( osPath ); } @@ -287,6 +309,8 @@ fileHandle_t FS_SV_FOpenFileWrite( const char *filename ) { Com_Printf( "FS_SV_FOpenFileWrite: %s\n", ospath ); } + FS_CheckFilenameIsMutable( ospath, __func__ ); + if( FS_CreatePath( ospath ) ) { return 0; } @@ -392,7 +416,7 @@ FS_SV_Rename =========== */ -void FS_SV_Rename( const char *from, const char *to ) { +void FS_SV_Rename( const char *from, const char *to, qboolean safe ) { char *from_ospath, *to_ospath; if ( !fs_searchpaths ) { @@ -411,6 +435,10 @@ void FS_SV_Rename( const char *from, const char *to ) { Com_Printf( "FS_SV_Rename: %s --> %s\n", from_ospath, to_ospath ); } + if ( safe ) { + FS_CheckFilenameIsMutable( to_ospath, __func__ ); + } + if (rename( from_ospath, to_ospath )) { // Failed, try copying it and deleting the original FS_CopyFile ( from_ospath, to_ospath ); @@ -505,6 +533,8 @@ fileHandle_t FS_FOpenFileWrite( const char *filename ) { Com_Printf( "FS_FOpenFileWrite: %s\n", ospath ); } + FS_CheckFilenameIsMutable( ospath, __func__ ); + if( FS_CreatePath( ospath ) ) { return 0; } @@ -551,6 +581,8 @@ fileHandle_t FS_FOpenFileAppend( const char *filename ) { Com_Printf( "FS_FOpenFileAppend: %s\n", ospath ); } + FS_CheckFilenameIsMutable( ospath, __func__ ); + if( FS_CreatePath( ospath ) ) { return 0; } diff --git a/codemp/qcommon/qcommon.h b/codemp/qcommon/qcommon.h index 8c0adaa..aa2b30f 100644 --- a/codemp/qcommon/qcommon.h +++ b/codemp/qcommon/qcommon.h @@ -544,7 +544,7 @@ fileHandle_t FS_FOpenFileWrite( const char *qpath ); int FS_filelength( fileHandle_t f ); fileHandle_t FS_SV_FOpenFileWrite( const char *filename ); int FS_SV_FOpenFileRead( const char *filename, fileHandle_t *fp ); -void FS_SV_Rename( const char *from, const char *to ); +void FS_SV_Rename( const char *from, const char *to, qboolean safe ); int FS_FOpenFileRead( const char *qpath, fileHandle_t *file, qboolean uniqueFILE ); // if uniqueFILE is true, then a new FILE will be fopened even if the file // is found in an already open pak file. If uniqueFILE is false, you must call