Re: svn commit: r1832662 - /ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java

[taher]

I can't find the JIRA reference for this commit, can you place share a
JIRA if it exists? Also, I'm not sure why you're calling
session.invalidate()? Doesn't request.getSession() already assign a
new session depending on client settings?

On Fri, Jun 1, 2018 at 10:37 AM,  <[hidden email]> wrote:

> Author: jleroux
> Date: Fri Jun  1 07:37:27 2018
> New Revision: 1832662
>
> URL: http://svn.apache.org/viewvc?rev=1832662&view=rev
> Log:
> Fixes a session fixation security issue discovered by a client with the security
>  audit tool "IBM Security AppScan Enterprise , Version : 9.0.3.7"
>
> Prevents the session fixation by making Tomcat generate a new jsessionId
> (ultimately put in cookie).
>
> Only do when really signing in to avoid unnecessary calls
> Though if the client has disabled the use of cookies, then a session will be
> new on each request, not a good choice on client side!
>
>
>
> Modified:
>     ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
>
> Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java?rev=1832662&r1=1832661&r2=1832662&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java (original)
> +++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java Fri Jun  1 07:37:27 2018
> @@ -328,6 +328,14 @@ public class LoginWorker {
>       */
>      public static String login(HttpServletRequest request, HttpServletResponse response) {
>          HttpSession session = request.getSession();
> +
> +        // Prevent session fixation by making Tomcat generate a new jsessionId (ultimately put in cookie).
> +        if (!session.isNew()) {  // Only do when really signing in.
> +            session.invalidate(); // If the client has disabled the use of cookies, then a session will be new on each request, not a good choice on client side!
> +            session = request.getSession(true);
> +            UtilHttp.setInitialRequestInfo(request); // We need to put that in place again
> +        }
> +
>          Delegator delegator = (Delegator) request.getAttribute("delegator");
>          String username = request.getParameter("USERNAME");
>          String password = request.getParameter("PASSWORD");
>
>

[Jacques Le Roux]

Hi Taher,

Thanks for asking.

I created the Jira afterwards and then updated the commits comments. More a pattern to prevent security disclosures. I must say in this case not of
much use, anyway I prefer to keep this habit.

I called session.invalidate() because at this stage we already have a session. Actually an org.apache.catalina.session.StandardSessionFacade, Tomcat
handles it. So request.getSession() does not create one.

But I had a new look and I can do better. I was unaware of request.changeSessionId() which is better here since I only need to change the session id.

I'll change that

Jacques


Le 03/06/2018 à 00:33, Taher Alkhateeb a écrit :

> I can't find the JIRA reference for this commit, can you place share a
> JIRA if it exists? Also, I'm not sure why you're calling
> session.invalidate()? Doesn't request.getSession() already assign a
> new session depending on client settings?
>
> On Fri, Jun 1, 2018 at 10:37 AM,  <[hidden email]> wrote:
>> Author: jleroux
>> Date: Fri Jun  1 07:37:27 2018
>> New Revision: 1832662
>>
>> URL: http://svn.apache.org/viewvc?rev=1832662&view=rev
>> Log:
>> Fixes a session fixation security issue discovered by a client with the security
>>   audit tool "IBM Security AppScan Enterprise , Version : 9.0.3.7"
>>
>> Prevents the session fixation by making Tomcat generate a new jsessionId
>> (ultimately put in cookie).
>>
>> Only do when really signing in to avoid unnecessary calls
>> Though if the client has disabled the use of cookies, then a session will be
>> new on each request, not a good choice on client side!
>>
>>
>>
>> Modified:
>>      ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
>>
>> Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java?rev=1832662&r1=1832661&r2=1832662&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java (original)
>> +++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java Fri Jun  1 07:37:27 2018
>> @@ -328,6 +328,14 @@ public class LoginWorker {
>>        */
>>       public static String login(HttpServletRequest request, HttpServletResponse response) {
>>           HttpSession session = request.getSession();
>> +
>> +        // Prevent session fixation by making Tomcat generate a new jsessionId (ultimately put in cookie).
>> +        if (!session.isNew()) {  // Only do when really signing in.
>> +            session.invalidate(); // If the client has disabled the use of cookies, then a session will be new on each request, not a good choice on client side!
>> +            session = request.getSession(true);
>> +            UtilHttp.setInitialRequestInfo(request); // We need to put that in place again
>> +        }
>> +
>>           Delegator delegator = (Delegator) request.getAttribute("delegator");
>>           String username = request.getParameter("USERNAME");
>>           String password = request.getParameter("PASSWORD");
>>
>>

[Michael Brohl-3]

Hi Jacques,

I would *always* prefer a Jira, patch and discussion *first* before a
commit in these core areas of the application.

Regards,

Michael


Am 03.06.18 um 10:28 schrieb Jacques Le Roux:

> Hi Taher,
>
> Thanks for asking.
>
> I created the Jira afterwards and then updated the commits comments.
> More a pattern to prevent security disclosures. I must say in this
> case not of much use, anyway I prefer to keep this habit.
>
> I called session.invalidate() because at this stage we already have a
> session. Actually an
> org.apache.catalina.session.StandardSessionFacade, Tomcat handles it.
> So request.getSession() does not create one.
>
> But I had a new look and I can do better. I was unaware of
> request.changeSessionId() which is better here since I only need to
> change the session id.
>
> I'll change that
>
> Jacques
>
>
> Le 03/06/2018 à 00:33, Taher Alkhateeb a écrit :
>> I can't find the JIRA reference for this commit, can you place share a
>> JIRA if it exists? Also, I'm not sure why you're calling
>> session.invalidate()? Doesn't request.getSession() already assign a
>> new session depending on client settings?
>>
>> On Fri, Jun 1, 2018 at 10:37 AM,  <[hidden email]> wrote:
>>> Author: jleroux
>>> Date: Fri Jun  1 07:37:27 2018
>>> New Revision: 1832662
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1832662&view=rev
>>> Log:
>>> Fixes a session fixation security issue discovered by a client with
>>> the security
>>>   audit tool "IBM Security AppScan Enterprise , Version : 9.0.3.7"
>>>
>>> Prevents the session fixation by making Tomcat generate a new
>>> jsessionId
>>> (ultimately put in cookie).
>>>
>>> Only do when really signing in to avoid unnecessary calls
>>> Though if the client has disabled the use of cookies, then a session
>>> will be
>>> new on each request, not a good choice on client side!
>>>
>>>
>>>
>>> Modified:
>>> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
>>>
>>> Modified:
>>> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java?rev=1832662&r1=1832661&r2=1832662&view=diff
>>> ==============================================================================
>>>
>>> ---
>>> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
>>> (original)
>>> +++
>>> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
>>> Fri Jun  1 07:37:27 2018
>>> @@ -328,6 +328,14 @@ public class LoginWorker {
>>>        */
>>>       public static String login(HttpServletRequest request,
>>> HttpServletResponse response) {
>>>           HttpSession session = request.getSession();
>>> +
>>> +        // Prevent session fixation by making Tomcat generate a new
>>> jsessionId (ultimately put in cookie).
>>> +        if (!session.isNew()) {  // Only do when really signing in.
>>> +            session.invalidate(); // If the client has disabled the
>>> use of cookies, then a session will be new on each request, not a
>>> good choice on client side!
>>> +            session = request.getSession(true);
>>> +            UtilHttp.setInitialRequestInfo(request); // We need to
>>> put that in place again
>>> +        }
>>> +
>>>           Delegator delegator = (Delegator)
>>> request.getAttribute("delegator");
>>>           String username = request.getParameter("USERNAME");
>>>           String password = request.getParameter("PASSWORD");
>>>
>>>
>

[Jacques Le Roux]

Hi Michael,

Yes I thought about talking about in the security ML.

As, it was pretty simple and I was sure of the result I finally decided to go this simple way.

I'll remember your will next time, especially if it's a big thing.

BTW what do you think about https://issues.apache.org/jira/browse/OFBIZ-10427 "Add a mean to handle CSRF" ?

Jacques


Le 03/06/2018 à 11:27, Michael Brohl a écrit :

> Hi Jacques,
>
> I would *always* prefer a Jira, patch and discussion *first* before a commit in these core areas of the application.
>
> Regards,
>
> Michael
>
>
> Am 03.06.18 um 10:28 schrieb Jacques Le Roux:
>> Hi Taher,
>>
>> Thanks for asking.
>>
>> I created the Jira afterwards and then updated the commits comments. More a pattern to prevent security disclosures. I must say in this case not of
>> much use, anyway I prefer to keep this habit.
>>
>> I called session.invalidate() because at this stage we already have a session. Actually an org.apache.catalina.session.StandardSessionFacade,
>> Tomcat handles it. So request.getSession() does not create one.
>>
>> But I had a new look and I can do better. I was unaware of request.changeSessionId() which is better here since I only need to change the session id.
>>
>> I'll change that
>>
>> Jacques
>>
>>
>> Le 03/06/2018 à 00:33, Taher Alkhateeb a écrit :
>>> I can't find the JIRA reference for this commit, can you place share a
>>> JIRA if it exists? Also, I'm not sure why you're calling
>>> session.invalidate()? Doesn't request.getSession() already assign a
>>> new session depending on client settings?
>>>
>>> On Fri, Jun 1, 2018 at 10:37 AM,  <[hidden email]> wrote:
>>>> Author: jleroux
>>>> Date: Fri Jun  1 07:37:27 2018
>>>> New Revision: 1832662
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1832662&view=rev
>>>> Log:
>>>> Fixes a session fixation security issue discovered by a client with the security
>>>>   audit tool "IBM Security AppScan Enterprise , Version : 9.0.3.7"
>>>>
>>>> Prevents the session fixation by making Tomcat generate a new jsessionId
>>>> (ultimately put in cookie).
>>>>
>>>> Only do when really signing in to avoid unnecessary calls
>>>> Though if the client has disabled the use of cookies, then a session will be
>>>> new on each request, not a good choice on client side!
>>>>
>>>>
>>>>
>>>> Modified:
>>>> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
>>>>
>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java?rev=1832662&r1=1832661&r2=1832662&view=diff
>>>> ==============================================================================
>>>> --- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java (original)
>>>> +++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java Fri Jun  1 07:37:27 2018
>>>> @@ -328,6 +328,14 @@ public class LoginWorker {
>>>>        */
>>>>       public static String login(HttpServletRequest request, HttpServletResponse response) {
>>>>           HttpSession session = request.getSession();
>>>> +
>>>> +        // Prevent session fixation by making Tomcat generate a new jsessionId (ultimately put in cookie).
>>>> +        if (!session.isNew()) {  // Only do when really signing in.
>>>> +            session.invalidate(); // If the client has disabled the use of cookies, then a session will be new on each request, not a good
>>>> choice on client side!
>>>> +            session = request.getSession(true);
>>>> +            UtilHttp.setInitialRequestInfo(request); // We need to put that in place again
>>>> +        }
>>>> +
>>>>           Delegator delegator = (Delegator) request.getAttribute("delegator");
>>>>           String username = request.getParameter("USERNAME");
>>>>           String password = request.getParameter("PASSWORD");
>>>>
>>>>
>>
>
>