From 0eb19b4d4c5635a50ea24ae436d8882d7547e2df Mon Sep 17 00:00:00 2001
From: Jordan Russell <jr-github@jrsoftware.org>
Date: Wed, 27 Nov 2024 04:39:23 -0600
Subject: [PATCH] SevenZipDecoder: Add extra layer of filename validation.

Instead of relying on PathExpand (GetFullPathName) alone to catch directory escape attempts, add our own checks.

We also now strictly reject path components with trailing dots or spaces.
---
 Projects/Src/Compression.SevenZipDecoder.pas | 49 ++++++++++++++++++--
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/Projects/Src/Compression.SevenZipDecoder.pas b/Projects/Src/Compression.SevenZipDecoder.pas
index 65c51d67e..82720b15c 100644
--- a/Projects/Src/Compression.SevenZipDecoder.pas
+++ b/Projects/Src/Compression.SevenZipDecoder.pas
@@ -46,11 +46,50 @@   TSevenZipDecodeState = record
 
 function IS_7zDec(const fileName: PChar; const fullPaths: Bool): Integer; cdecl; external name '_IS_7zDec';
 
+function ValidateAndCombinePath(const ADestDir, AFilename: String;
+  out AResultingPath: String): Boolean;
+{ Filenames read from archives aren't validated at all by the SDK's 7zMain.c,
+  so we have to handle that ourself. Most importantly, we need to make sure a
+  malicious archive cannot create files outside of the destination directory.
+  Returns True if all security checks pass, with the combination of ADestDir
+  and AFilename in AResultingPath.
+  ADestDir is assumed to be normalized already and have a trailing backslash.
+  AFilename may be a file or directory name. }
+begin
+  { - Don't allow empty names
+    - Don't allow forward slashes or repeated slashes
+      (archives use '/' on disk but 7zMain.c changes them to '\')
+    - Don't allow rooted (non-relative to current directory) names
+    - Don't allow trailing slash
+    - Don't allow invalid characters/dots/spaces (this catches '..') }
+  Result := False;
+  if (AFilename <> '') and
+     (AFilename = PathNormalizeSlashes(AFilename)) and
+     not PathIsRooted(AFilename) and
+     not PathCharIsSlash(AFilename[High(AFilename)]) and
+     not PathHasInvalidCharacters(AFilename, False) then begin
+    { Our validity checks passed. Now pass the combined path to PathExpand
+      (GetFullPathName) to see if it thinks the path needs normalization.
+      If the returned path isn't exactly what was passed in, then consider
+      the name invalid.
+      One way that can happen is if the path ends in an MS-DOS device name:
+      PathExpand('c:\path\NUL') returns '\\.\NUL'. Obviously we don't want
+      devices being opened, so that must be rejected. }
+    var CombinedPath := ADestDir + AFilename;
+    var TestExpandedPath: String;
+    if PathExpand(CombinedPath, TestExpandedPath) and
+       (CombinedPath = TestExpandedPath) then begin
+      AResultingPath := CombinedPath;
+      Result := True;
+    end;
+  end;
+end;
+
 function __CreateDirectoryW(lpPathName: LPCWSTR;
   lpSecurityAttributes: PSecurityAttributes): BOOL; cdecl;
 begin
   var ExpandedDir: String;
-  if PathExpand(lpPathName, ExpandedDir) and PathStartsWith(ExpandedDir, State.ExpandedDestDir) then
+  if ValidateAndCombinePath(State.ExpandedDestDir, lpPathName, ExpandedDir) then
     Result := CreateDirectoryW(PChar(ExpandedDir), lpSecurityAttributes)
   else begin
     Result := False;
@@ -74,9 +113,11 @@ function __CreateFileW(lpFileName: LPCWSTR; dwDesiredAccess, dwShareMode: DWORD;
   hTemplateFile: THandle): THandle; cdecl;
 begin
   var ExpandedFileName: String;
-  if PathExpand(lpFileName, ExpandedFileName) and
-     (((dwDesiredAccess = GENERIC_READ) and (PathCompare(ExpandedFileName, State.ExpandedArchiveFileName) = 0)) or
-      ((dwDesiredAccess = GENERIC_WRITE) and PathStartsWith(ExpandedFileName, State.ExpandedDestDir))) then
+  if ((dwDesiredAccess = GENERIC_READ) and
+      PathExpand(lpFileName, ExpandedFileName) and
+      (PathCompare(ExpandedFileName, State.ExpandedArchiveFileName) = 0)) or
+     ((dwDesiredAccess = GENERIC_WRITE) and
+      ValidateAndCombinePath(State.ExpandedDestDir, lpFileName, ExpandedFileName)) then
     Result := CreateFileW(PChar(ExpandedFileName), dwDesiredAccess, dwShareMode, lpSecurityAttributes, dwCreationDisposition, dwFlagsAndAttributes, hTemplateFile)
   else begin
     Result := INVALID_HANDLE_VALUE;