From d2f3c303aa0a7a94396bfa5b7a47b8eff36b4c1e Mon Sep 17 00:00:00 2001 From: Lactozilla Date: Tue, 28 Jan 2025 23:14:46 -0300 Subject: [PATCH] Make validation more robust --- src/r_picformats.c | 2 +- src/r_picformats.h | 3 ++- src/w_wad.c | 23 +++++++++++++++++------ 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/r_picformats.c b/src/r_picformats.c index d58d5abbc..56db44975 100644 --- a/src/r_picformats.c +++ b/src/r_picformats.c @@ -909,7 +909,7 @@ boolean Picture_IsLumpPNG(const UINT8 *d, size_t s) // Check for PNG file signature using memcmp // As it may be faster on CPUs with slow unaligned memory access // Ref: http://www.libpng.org/pub/png/spec/1.2/PNG-Rationale.html#R.PNG-file-signature - return (memcmp(&d[0], "\x89\x50\x4e\x47\x0d\x0a\x1a\x0a", 8) == 0); + return (memcmp(&d[0], "\x89\x50\x4e\x47\x0d\x0a\x1a\x0a", PNG_HEADER_SIZE) == 0); } #ifndef NO_PNG_LUMPS diff --git a/src/r_picformats.h b/src/r_picformats.h index 409859136..ac6fc2992 100644 --- a/src/r_picformats.h +++ b/src/r_picformats.h @@ -56,7 +56,8 @@ enum }; // Minimum length of a valid Doom patch -#define PATCH_MIN_SIZE 13 +// This is the size of a 1x1 patch. +#define PATCH_MIN_SIZE ((sizeof(INT16) * 4) + (sizeof(INT32)) + 1) // Minimum size of a PNG file. // See: https://web.archive.org/web/20230524232139/http://garethrees.org/2007/11/14/pngcrush/ diff --git a/src/w_wad.c b/src/w_wad.c index 87e13ed0f..456b01a1a 100644 --- a/src/w_wad.c +++ b/src/w_wad.c @@ -1700,7 +1700,7 @@ lumpnum_t W_GetNumForLongName(const char *name) // in its entirety. static boolean W_IsProbablyValidPatch(UINT16 wadnum, UINT16 lumpnum) { - UINT8 header[sizeof(INT16) * 4]; + UINT8 header[PATCH_MIN_SIZE]; I_StaticAssert(sizeof(header) >= PNG_HEADER_SIZE); @@ -1708,7 +1708,7 @@ static boolean W_IsProbablyValidPatch(UINT16 wadnum, UINT16 lumpnum) size_t lumplen = W_LumpLengthPwad(wadnum, lumpnum); // Cannot be a valid Doom patch - if (lumplen < PATCH_MIN_SIZE) + if (lumplen < sizeof(header)) return false; // Check if it's probably a valid PNG @@ -1726,11 +1726,14 @@ static boolean W_IsProbablyValidPatch(UINT16 wadnum, UINT16 lumpnum) // Otherwise, we read it as a patch } - // Read the first 8 bytes + // Read the first 12 bytes, plus one W_ReadLumpHeaderPwad(wadnum, lumpnum, header, sizeof(header), 0); - INT16 width = ((UINT16 *)header)[0]; - INT16 height = ((UINT16 *)header)[1]; + softwarepatch_t patch; + memcpy(&patch, header, sizeof(header)); + + INT16 width = SHORT(patch.width); + INT16 height = SHORT(patch.height); // Lump size makes no sense given the width if (!VALID_PATCH_LUMP_SIZE(lumplen, width)) @@ -1739,7 +1742,15 @@ static boolean W_IsProbablyValidPatch(UINT16 wadnum, UINT16 lumpnum) // Check the dimensions. if (width > 0 && height > 0 && width <= MAX_PATCH_DIMENSIONS && height <= MAX_PATCH_DIMENSIONS) { - // Dimensions seem to make sense. Patch might be valid + // Dimensions seem to make sense... But check at least the first column. + UINT32 ofs = LONG(patch.columnofs[0]); + + // Need one byte for an empty column (but there's patches that don't know that!) + if (ofs < ((sizeof(INT16) * 4) + (width * sizeof(INT32))) || ofs >= (UINT32)lumplen) + { + return false; + } + return true; }