From: Simon McVittie Date: Fri, 18 Nov 2011 18:44:44 +0000 Subject: CVE-2006-2236 - add bounds-checking to COM_StripExtension This fixes the "remapShader" exploit by backporting ioquake3 r765, with a further change to avoid strncpy'ing a string into itself. Original patch by Thilo Schulz. Origin: backport Bug-Debian: http://bugs.debian.org/660827 Bug-CVE: http://security-tracker.debian.org/tracker/CVE-2006-2236 Bug-Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=455458 --- src/cgame/cg_weapons.c | 6 +++--- src/client/cl_main.c | 2 +- src/qcommon/files.c | 2 +- src/qcommon/q_shared.c | 6 ++++-- src/qcommon/q_shared.h | 2 +- src/qcommon/vm.c | 2 +- src/renderer/tr_bsp.c | 2 +- src/renderer/tr_shader.c | 6 +++--- src/ui/ui_main.c | 2 +- src/ui/ui_players.c | 4 ++-- 10 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/cgame/cg_weapons.c b/src/cgame/cg_weapons.c index ecb2499..87a9b4c 100644 --- src/cgame/cg_weapons.c +++ src/cgame/cg_weapons.c @@ -503,17 +503,17 @@ static qboolean CG_ParseWeaponFile( const char *filename, weaponInfo_t *wi ) CG_Printf( S_COLOR_RED "ERROR: weapon model not found %s\n", token ); strcpy( path, token ); - COM_StripExtension( path, path ); + COM_StripExtension( path, path, sizeof(path) ); strcat( path, "_flash.md3" ); wi->flashModel = trap_R_RegisterModel( path ); strcpy( path, token ); - COM_StripExtension( path, path ); + COM_StripExtension( path, path, sizeof(path) ); strcat( path, "_barrel.md3" ); wi->barrelModel = trap_R_RegisterModel( path ); strcpy( path, token ); - COM_StripExtension( path, path ); + COM_StripExtension( path, path, sizeof(path) ); strcat( path, "_hand.md3" ); wi->handsModel = trap_R_RegisterModel( path ); diff --git a/src/client/cl_main.c b/src/client/cl_main.c index 3db1d6d..7b562bd 100644 --- src/client/cl_main.c +++ src/client/cl_main.c @@ -2014,7 +2014,7 @@ void CL_Frame ( int msec ) { } Q_strncpyz( mapName, COM_SkipPath( cl.mapname ), sizeof( cl.mapname ) ); - COM_StripExtension( mapName, mapName ); + COM_StripExtension(mapName, mapName, sizeof(mapName)); Cbuf_ExecuteText( EXEC_NOW, va( "record %s-%s-%s", nowString, serverName, mapName ) ); diff --git a/src/qcommon/files.c b/src/qcommon/files.c index 84db5b1..67f375c 100644 --- src/qcommon/files.c +++ src/qcommon/files.c @@ -3377,7 +3377,7 @@ void FS_FilenameCompletion( const char *dir, const char *ext, Q_strncpyz( filename, filenames[ i ], MAX_STRING_CHARS ); if( stripExt ) { - COM_StripExtension( filename, filename ); + COM_StripExtension(filename, filename, sizeof(filename)); } callback( filename ); diff --git a/src/qcommon/q_shared.c b/src/qcommon/q_shared.c index 9f152d6..44085cc 100644 --- src/qcommon/q_shared.c +++ src/qcommon/q_shared.c @@ -59,10 +59,12 @@ char *COM_SkipPath (char *pathname) COM_StripExtension ============ */ -void COM_StripExtension( const char *in, char *out ) { +void COM_StripExtension( const char *in, char *out, int destsize ) { int length; - strcpy( out, in ); + if (in != out) { + Q_strncpyz(out, in, destsize); + } length = strlen(out)-1; while (length > 0 && out[length] != '.') diff --git a/src/qcommon/q_shared.h b/src/qcommon/q_shared.h index 0059cdf..03b2e03 100644 --- src/qcommon/q_shared.h +++ src/qcommon/q_shared.h @@ -627,7 +627,7 @@ vec_t DistanceBetweenLineSegments( float Com_Clamp( float min, float max, float value ); char *COM_SkipPath( char *pathname ); -void COM_StripExtension( const char *in, char *out ); +void COM_StripExtension(const char *in, char *out, int destsize); void COM_DefaultExtension( char *path, int maxSize, const char *extension ); void COM_BeginParseSession( const char *name ); diff --git a/src/qcommon/vm.c b/src/qcommon/vm.c index 0235ffe..8fb588d 100644 --- src/qcommon/vm.c +++ src/qcommon/vm.c @@ -231,7 +231,7 @@ void VM_LoadSymbols( vm_t *vm ) { return; } - COM_StripExtension( vm->name, name ); + COM_StripExtension(vm->name, name, sizeof(name)); Com_sprintf( symbols, sizeof( symbols ), "vm/%s.map", name ); len = FS_ReadFile( symbols, (void **)&mapfile ); if ( !mapfile ) { diff --git a/src/renderer/tr_bsp.c b/src/renderer/tr_bsp.c index eb57da8..2eff834 100644 --- src/renderer/tr_bsp.c +++ src/renderer/tr_bsp.c @@ -1824,7 +1824,7 @@ void RE_LoadWorldMap( const char *name ) { Q_strncpyz( s_worldData.name, name, sizeof( s_worldData.name ) ); Q_strncpyz( s_worldData.baseName, COM_SkipPath( s_worldData.name ), sizeof( s_worldData.name ) ); - COM_StripExtension( s_worldData.baseName, s_worldData.baseName ); + COM_StripExtension(s_worldData.baseName, s_worldData.baseName, sizeof(s_worldData.baseName)); startMarker = ri.Hunk_Alloc(0, h_low); c_gridVerts = 0; diff --git a/src/renderer/tr_shader.c b/src/renderer/tr_shader.c index 380f9bf..c349b15 100644 --- src/renderer/tr_shader.c +++ src/renderer/tr_shader.c @@ -96,7 +96,7 @@ void R_RemapShader(const char *shaderName, const char *newShaderName, const char // remap all the shaders with the given name // even tho they might have different lightmaps - COM_StripExtension( shaderName, strippedName ); + COM_StripExtension(shaderName, strippedName, sizeof(strippedName)); hash = generateHashValue(strippedName, FILE_HASH_SIZE); for (sh = hashTable[hash]; sh; sh = sh->next) { if (Q_stricmp(sh->name, strippedName) == 0) { @@ -2366,7 +2366,7 @@ shader_t *R_FindShaderByName( const char *name ) { return tr.defaultShader; } - COM_StripExtension( name, strippedName ); + COM_StripExtension(name, strippedName, sizeof(strippedName)); hash = generateHashValue(strippedName, FILE_HASH_SIZE); @@ -2434,7 +2434,7 @@ shader_t *R_FindShader( const char *name, int lightmapIndex, qboolean mipRawImag lightmapIndex = LIGHTMAP_BY_VERTEX; } - COM_StripExtension( name, strippedName ); + COM_StripExtension(name, strippedName, sizeof(strippedName)); hash = generateHashValue(strippedName, FILE_HASH_SIZE); diff --git a/src/ui/ui_main.c b/src/ui/ui_main.c index 7509d9b..604e709 100644 --- src/ui/ui_main.c +++ src/ui/ui_main.c @@ -5376,7 +5376,7 @@ static void UI_BuildQ3Model_List( void ) { filelen = strlen(fileptr); - COM_StripExtension(fileptr,skinname); + COM_StripExtension(fileptr, skinname, sizeof(skinname)); // look for icon_???? if (Q_stricmpn(skinname, "icon_", 5) == 0 && !(Q_stricmp(skinname,"icon_blue") == 0 || Q_stricmp(skinname,"icon_red") == 0)) diff --git a/src/ui/ui_players.c b/src/ui/ui_players.c index 5dbfdd3..f7baf31 100644 --- src/ui/ui_players.c +++ src/ui/ui_players.c @@ -84,13 +84,13 @@ tryagain: if ( weaponNum == WP_MACHINEGUN ) { strcpy( path, item->world_model[0] ); - COM_StripExtension( path, path ); + COM_StripExtension( path, path, sizeof(path) ); strcat( path, "_barrel.md3" ); pi->barrelModel = trap_R_RegisterModel( path ); } strcpy( path, item->world_model[0] ); - COM_StripExtension( path, path ); + COM_StripExtension( path, path, sizeof(path) ); strcat( path, "_flash.md3" ); pi->flashModel = trap_R_RegisterModel( path );