You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by jl...@apache.org on 2018/06/01 07:37:28 UTC
svn commit: r1832662 -
/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
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");
Re: svn commit: r1832662 -
/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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, <jl...@apache.org> 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");
>>>>
>>>>
>>
>
>
Re: svn commit: r1832662 -
/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
Posted by Michael Brohl <mi...@ecomify.de>.
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, <jl...@apache.org> 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");
>>>
>>>
>
Re: svn commit: r1832662 -
/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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, <jl...@apache.org> 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");
>>
>>
Re: svn commit: r1832662 - /ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/LoginWorker.java
Posted by Taher Alkhateeb <sl...@gmail.com>.
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, <jl...@apache.org> 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");
>
>