FIXED a Vulnerability bug which affects some deployment configurations.

danieleteti · Feb 13, 2020 · 92dcbd8 · 92dcbd8
1 parent 8b46dfc
commit 92dcbd8
Show file tree

Hide file tree

Showing 10 changed files with 60 additions and 13 deletions.
diff --git a/README.md b/README.md
@@ -264,6 +264,8 @@ end;
 
 - **Breaking Change!** `TDataSetHolder` doesn't renders dataset in a property called `items` but in a property named `data` (to be more standard).
 
+- Fixed! Has been patched a serious security bug affecting deployment configurations which uses internal WebServer to serve static files (do not affect all Apache, IIS or proxied deployments).  Thanks to **Stephan Munz** to have discovered it. *Update to dmvcframework-3.2-RC5+ is required for all such kind of deployments.*
+
 - New Installation procedure! 
 
     - Open the project group (select the correct one from the following table)

diff --git a/sources/MVCFramework.pas b/sources/MVCFramework.pas
@@ -2155,8 +2155,11 @@ function TMVCEngine.ExecuteAction(const ASender: TObject; const ARequest: TWebRe
                   if not Config[TMVCConfigKey.FallbackResource].IsEmpty then
                   begin
                     if (LContext.Request.PathInfo = '/') or (LContext.Request.PathInfo = '') then // useful for SPA
-                      Result := SendStaticFileIfPresent(LContext, TPath.Combine(Config[TMVCConfigKey.DocumentRoot],
+                    begin
+                      lFileName := TPath.GetFullPath(TPath.Combine(Config[TMVCConfigKey.DocumentRoot],
                         Config[TMVCConfigKey.FallbackResource]));
+                      Result := SendStaticFileIfPresent(LContext, lFileName);
+                    end;
                   end;
                   if (not Result) and (IsStaticFileRequest(ARequest, LFileName)) then
                   begin
@@ -2761,13 +2764,22 @@ class function TMVCStaticContents.IsScriptableFile(const AStaticFileName: string
 class function TMVCStaticContents.IsStaticFile(const AViewPath, AWebRequestPath: string;
 out ARealFileName: string): Boolean;
 var
-  FileName: string;
+  lFileName, lWebRoot: string;
 begin
   if TDirectory.Exists(AViewPath) then
-    FileName := AViewPath + AWebRequestPath.Replace('/', TPath.DirectorySeparatorChar)
+  begin
+    lWebRoot := TPath.GetFullPath(AViewPath);
+  end
   else
-    FileName := GetApplicationFileNamePath + AViewPath + AWebRequestPath.Replace('/', TPath.DirectorySeparatorChar);
-  ARealFileName := FileName;
+  begin
+    lWebRoot := TPath.GetFullPath(GetApplicationFileNamePath + AViewPath);
+  end;
+  lFileName := TPath.GetFullPath(lWebRoot + AWebRequestPath.Replace('/', TPath.DirectorySeparatorChar));
+  if not lFileName.StartsWith(lWebRoot) then
+  begin
+    Exit(False);
+  end;
+  ARealFileName := lFileName;
   Result := TFile.Exists(ARealFileName);
 end;
 

diff --git a/sources/dmvcframeworkbuildconsts.inc b/sources/dmvcframeworkbuildconsts.inc
@@ -1,2 +1,2 @@
 const
-  DMVCFRAMEWORK_VERSION = '3.2.0 (boron) RC4';
+  DMVCFRAMEWORK_VERSION = '3.2.0 (boron) RC5';
diff --git a/unittests/general/Several/LiveServerTestU.pas b/unittests/general/Several/LiveServerTestU.pas
@@ -205,6 +205,10 @@   TServerTest = class(TBaseServerTest)
     procedure TestResponseNoContent;
     [Test]
     procedure TestResponseAccepted;
+
+    // test web server
+    [Test]
+    procedure TestDirectoryTraversal1;
   end;
 
   [TestFixture]
@@ -1313,6 +1317,33 @@ procedure TServerTest.TestDeserializeNullablesWithValue;
   end;
 end;
 
+procedure TServerTest.TestDirectoryTraversal1;
+var
+  lRes: IRESTResponse;
+  I: Integer;
+  lUrl: String;
+begin
+  lRes := RESTClient
+    .Accept(TMVCMediaType.TEXT_HTML)
+    .doGET('/index.html', []);
+  Assert.areEqual(200, lRes.ResponseCode);
+
+  lRes := RESTClient
+    .Accept(TMVCMediaType.TEXT_HTML)
+    .doGET('/..\donotdeleteme.txt', []);
+  Assert.areEqual(404, lRes.ResponseCode);
+
+  lUrl := 'Windows\win.ini';
+  for I := 1 to 20 do
+  begin
+    lUrl := '..\' + lUrl;
+    lRes := RESTClient
+      .Accept(TMVCMediaType.TEXT_HTML)
+      .doGET('/' + lUrl, []);
+    Assert.areEqual(404, lRes.ResponseCode, 'Fail with: ' + '/' + lUrl);
+  end;
+end;
+
 procedure TServerTest.TestSerializeAndDeserializeNullables;
 var
   lRes: IRESTResponse;

diff --git a/unittests/general/TestServer/TestServer.dpr b/unittests/general/TestServer/TestServer.dpr
@@ -13,7 +13,7 @@ uses
   Web.WebBroker,
   MVCFramework.Commons,
   MVCFramework.Console,
-  WebModuleUnit in 'WebModuleUnit.pas' {bas: TWebModule},
+  WebModuleUnit in 'WebModuleUnit.pas' {MainWebModule: TWebModule},
   TestServerControllerU in 'TestServerControllerU.pas',
   TestServerControllerExceptionU in 'TestServerControllerExceptionU.pas',
   SpeedMiddlewareU in 'SpeedMiddlewareU.pas',

diff --git a/unittests/general/TestServer/TestServer.dproj b/unittests/general/TestServer/TestServer.dproj
@@ -145,13 +145,14 @@
         <VerInfo_Keys>CompanyName=;FileDescription=$(MSBuildProjectName);FileVersion=1.0.0.0;InternalName=;LegalCopyright=;LegalTrademarks=;OriginalFilename=;ProductName=$(MSBuildProjectName);ProductVersion=1.0.0.0;Comments=;ProgramID=com.embarcadero.$(MSBuildProjectName)</VerInfo_Keys>
         <Icon_MainIcon>TestServer_Icon.ico</Icon_MainIcon>
         <Manifest_File>(None)</Manifest_File>
+        <DCC_DcuOutput>.\dcus\$(Platform)\$(Config)</DCC_DcuOutput>
     </PropertyGroup>
     <ItemGroup>
         <DelphiCompile Include="$(MainSource)">
             <MainSource>MainSource</MainSource>
         </DelphiCompile>
         <DCCReference Include="WebModuleUnit.pas">
-            <Form>bas</Form>
+            <Form>MainWebModule</Form>
             <DesignClass>TWebModule</DesignClass>
         </DCCReference>
         <DCCReference Include="TestServerControllerU.pas"/>

diff --git a/unittests/general/TestServer/TestServerControllerU.pas b/unittests/general/TestServer/TestServerControllerU.pas
@@ -221,7 +221,6 @@   TTestServerController = class(TMVCController)
     [MVCHTTPMethod([httpGET])]
     [MVCPath('/responses/nocontent')]
     procedure TestResponseNoContent;
-
   end;
 
   [MVCPath('/private')]

diff --git a/unittests/general/TestServer/WebModuleUnit.dfm b/unittests/general/TestServer/WebModuleUnit.dfm
@@ -1,4 +1,4 @@
-object bas: Tbas
+object MainWebModule: TMainWebModule
   OldCreateOrder = False
   OnCreate = WebModuleCreate
   Actions = <>

diff --git a/unittests/general/TestServer/WebModuleUnit.pas b/unittests/general/TestServer/WebModuleUnit.pas
@@ -38,14 +38,14 @@ interface
     ;
 
 type
-  Tbas = class(TWebModule)
+  TMainWebModule = class(TWebModule)
     procedure WebModuleCreate(Sender: TObject);
   private
     MVCEngine: TMVCEngine;
   end;
 
 var
-  WebModuleClass: TComponentClass = Tbas;
+  WebModuleClass: TComponentClass = TMainWebModule;
 
 implementation
 
@@ -64,14 +64,15 @@ implementation
   TestServerControllerJSONRPCU,
   MVCFramework.Middleware.Compression;
 
-procedure Tbas.WebModuleCreate(Sender: TObject);
+procedure TMainWebModule.WebModuleCreate(Sender: TObject);
 begin
   MVCEngine := TMVCEngine.Create(self,
     procedure(Config: TMVCConfig)
     begin
       // no config here
       Config[TMVCConfigKey.SessionTimeout] := '0'; // setting cookie
       Config[TMVCConfigKey.PathPrefix] := '';
+      Config[TMVCConfigKey.DocumentRoot] := '..\..\www';
     end, nil);
   MVCEngine.AddController(TTestServerController)
     .AddController(TTestPrivateServerController)

diff --git a/unittests/general/TestServer/donotdeleteme.txt b/unittests/general/TestServer/donotdeleteme.txt
@@ -0,0 +1 @@
+This file must not be readable from an HTTP request
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		This file must not be readable from an HTTP request