View Issue Details

IDProjectCategoryView StatusLast Update
0034595FPCPackagespublic2018-12-13 15:47
ReporterMichal GawryckiAssigned ToMichael Van Canneyt 
PrioritynormalSeverityminorReproducibilityhave not tried
Status closedResolutionfixed 
Product Version3.3.1Product Build 
Target Version3.2.0Fixed in Version3.3.1 
Summary0034595: fphttpclient - improved redirections
DescriptionCurrently, fphttpclient does not support redirection to a relative path. Cookies also should not be cleaned when redirecting within the same domain.
Patch attached.
TagsNo tags attached.
Fixed in Revision40395
FPCOldBugId
FPCTarget
Attached Files
  • fphttpclient-redir.patch (1,460 bytes)
    Index: packages/fcl-web/src/base/fphttpclient.pp
    ===================================================================
    --- packages/fcl-web/src/base/fphttpclient.pp	(revision 39795)
    +++ packages/fcl-web/src/base/fphttpclient.pp	(working copy)
    @@ -1376,7 +1376,7 @@
       Stream: TStream; const AllowedResponseCodes: array of Integer);
     
     Var
    -  M,L,NL : String;
    +  M,L,NL,RNL : String;
       RC : Integer;
       RR : Boolean; // Repeat request ?
     
    @@ -1399,17 +1399,22 @@
             if (RC>MaxRedirects) then
               Raise EHTTPClient.CreateFmt(SErrMaxRedirectsReached,[RC]);
             NL:=GetHeader(FResponseHeaders,'Location');
    -        if Not Assigned(FOnRedirect) then
    -          L:=NL
    -        else
    +        if Assigned(FOnRedirect) then
               FOnRedirect(Self,L,NL);
    +        if (not IsAbsoluteURI(NL)) and ResolveRelativeURI(L,NL,RNL) then
    +          NL:=RNL;
             if (RedirectForcesGET(FResponseStatusCode)) then
               M:='GET';
    +        // Request has saved cookies in sentcookies.
    +        if ParseURI(L).Host=ParseURI(NL).Host then
    +          FreeAndNil(FSentCookies)
    +        else
    +          begin
    +          FreeAndNil(FCookies);
    +          FCookies:=FSentCookies;
    +          FSentCookies:=Nil;
    +          end;
             L:=NL;
    -        // Request has saved cookies in sentcookies.
    -        FreeAndNil(FCookies);
    -        FCookies:=FSentCookies;
    -        FSentCookies:=Nil;
             end;
           end;
         if (FResponseStatusCode=401) then
    
    fphttpclient-redir.patch (1,460 bytes)

Activities

Michal Gawrycki

2018-11-23 21:44

reporter  

fphttpclient-redir.patch (1,460 bytes)
Index: packages/fcl-web/src/base/fphttpclient.pp
===================================================================
--- packages/fcl-web/src/base/fphttpclient.pp	(revision 39795)
+++ packages/fcl-web/src/base/fphttpclient.pp	(working copy)
@@ -1376,7 +1376,7 @@
   Stream: TStream; const AllowedResponseCodes: array of Integer);
 
 Var
-  M,L,NL : String;
+  M,L,NL,RNL : String;
   RC : Integer;
   RR : Boolean; // Repeat request ?
 
@@ -1399,17 +1399,22 @@
         if (RC>MaxRedirects) then
           Raise EHTTPClient.CreateFmt(SErrMaxRedirectsReached,[RC]);
         NL:=GetHeader(FResponseHeaders,'Location');
-        if Not Assigned(FOnRedirect) then
-          L:=NL
-        else
+        if Assigned(FOnRedirect) then
           FOnRedirect(Self,L,NL);
+        if (not IsAbsoluteURI(NL)) and ResolveRelativeURI(L,NL,RNL) then
+          NL:=RNL;
         if (RedirectForcesGET(FResponseStatusCode)) then
           M:='GET';
+        // Request has saved cookies in sentcookies.
+        if ParseURI(L).Host=ParseURI(NL).Host then
+          FreeAndNil(FSentCookies)
+        else
+          begin
+          FreeAndNil(FCookies);
+          FCookies:=FSentCookies;
+          FSentCookies:=Nil;
+          end;
         L:=NL;
-        // Request has saved cookies in sentcookies.
-        FreeAndNil(FCookies);
-        FCookies:=FSentCookies;
-        FSentCookies:=Nil;
         end;
       end;
     if (FResponseStatusCode=401) then
fphttpclient-redir.patch (1,460 bytes)

Thaddy de Koning

2018-11-25 12:39

reporter   ~0112180

Last edited: 2018-11-25 12:51

View 3 revisions

"Cookies also should not be cleaned when redirecting within the same domain"
Yes they should. Always. Basic.
I am curious that you write a decent patch and introduce a vulnerability at the same time? Plz explain...
Introducing such code into public domain open source is a very (not!) inventive way to compromise any re-compiles.

You also made the point "and also" where I can only see it as the core of the patch.... Plz explain....
Such things should be properly evaluated on the mailing list.

Thaddy de Koning

2018-11-25 12:58

reporter   ~0112184

The core team should discuss this.

Michael Van Canneyt

2018-11-29 16:38

administrator   ~0112263

Checked and applied patch, thanks; this is a useful improvement !

Michal Gawrycki

2018-12-13 15:47

reporter   ~0112536

Thanks! Closed.

Issue History

Date Modified Username Field Change
2018-11-23 21:44 Michal Gawrycki New Issue
2018-11-23 21:44 Michal Gawrycki File Added: fphttpclient-redir.patch
2018-11-25 12:39 Thaddy de Koning Note Added: 0112180
2018-11-25 12:44 Thaddy de Koning Note Edited: 0112180 View Revisions
2018-11-25 12:51 Thaddy de Koning Note Edited: 0112180 View Revisions
2018-11-25 12:58 Thaddy de Koning Note Added: 0112184
2018-11-29 16:34 Michael Van Canneyt Assigned To => Michael Van Canneyt
2018-11-29 16:34 Michael Van Canneyt Status new => assigned
2018-11-29 16:38 Michael Van Canneyt Fixed in Revision => 40395
2018-11-29 16:38 Michael Van Canneyt Note Added: 0112263
2018-11-29 16:38 Michael Van Canneyt Status assigned => resolved
2018-11-29 16:38 Michael Van Canneyt Fixed in Version => 3.3.1
2018-11-29 16:38 Michael Van Canneyt Resolution open => fixed
2018-11-29 16:38 Michael Van Canneyt Target Version => 3.2.0
2018-12-13 15:47 Michal Gawrycki Note Added: 0112536
2018-12-13 15:47 Michal Gawrycki Status resolved => closed