DWR

Add support for HttpOnly cookies

Details

  • Type: Improvement Improvement
  • Status: Resolved Resolved
  • Priority: Normal Normal
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: 3.0.RC2
  • Component/s: Security
  • Documentation Required:
    Yes
  • Description:
    If JSESSIONID cookies are marked HttpOnly then DWR will fail.
    In addition there is some risk of using the same nonce in a cookie as is used in a body request.
    So DWR should separate the 2 nonces, and store the body nonce on the server in the session.

Issue Links

Activity

Hide
David Marginian added a comment - 23/Jul/10 7:01 PM

httpOnly is becoming increasingly popular. We need to address this issue in 3.x.

Show
David Marginian added a comment - 23/Jul/10 7:01 PM httpOnly is becoming increasingly popular. We need to address this issue in 3.x.
Hide
David Marginian added a comment - 23/Jul/10 7:04 PM

Mike,
Per the mailing list thread included above - I have started to implement Joe's suggestion. I will keep you posted and would like you to code review any changes. Thanks.

Show
David Marginian added a comment - 23/Jul/10 7:04 PM Mike, Per the mailing list thread included above - I have started to implement Joe's suggestion. I will keep you posted and would like you to code review any changes. Thanks.
Hide
Mike Wilson added a comment - 05/Aug/10 12:49 PM

There are two other mailing list threads that relate to this feature:
https://dwr.dev.java.net/servlets/BrowseList?list=users&by=thread&from=1822043
and
https://dwr.dev.java.net/servlets/ReadMsg?listName=users&msgNo=17127 (linking to the post where the relevant discussion started in the middle of the thread)

Show
Mike Wilson added a comment - 05/Aug/10 12:49 PM There are two other mailing list threads that relate to this feature: https://dwr.dev.java.net/servlets/BrowseList?list=users&by=thread&from=1822043 and https://dwr.dev.java.net/servlets/ReadMsg?listName=users&msgNo=17127 (linking to the post where the relevant discussion started in the middle of the thread)
Hide
David Marginian added a comment - 24/Aug/10 7:42 PM

The JavaScript Security page (script-tag-protection.html) in the docs will need to be updated with this change.

Show
David Marginian added a comment - 24/Aug/10 7:42 PM The JavaScript Security page (script-tag-protection.html) in the docs will need to be updated with this change.
Hide
Mike Wilson added a comment - 15/Sep/10 11:36 AM

This feature has now been implemented and also resolves DWR-17, DWR-338, DWR-364, and DWR-433.

Following is a description of the implementation and the changes done, to be used for documentation and release notes:

The client layer (engine.js) is now in full control over the script session id and determines when it wants to assign this id. The id has been split up in two parts; one common for the whole browser process, named the DWR session, and one part unique for each loaded page in a browser window. The DWR session is stored as a DWRSESSIONID session cookie and normally only has to be assigned on the first request of a session. It has nothing to do with the HttpSession. If cookies are disabled this id will be set up for each page load. The client layer takes help from a new __System.generateId() call to obtain a secure and globally unique random string from the server. The page-local part of the id is generated locally on each page load.

The server-side id generator has been replaced with a more secure version and promoted to a plugin:able bean in the container, so there is now an interface in the extend package and the implementation has also moved package-wise. And there is a new entry in defaults.properties.

As the client layer determines the script session id there is no longer any id resync when a script session has timed out on the server side, it will just be recreated with the same id when there is a call from the same page.

Calls from the client will no longer attach the httpSessionId in the batch data as we often will not have access to it (httpOnly). For the CSRF scheme we are instead relying on the first part of the script session id, separated from the window specific part with a slash, which will mirror the DWRSESSIONID cookie value in the same request. Security comes from a CSRF attackers inability to assign this cookie.
Thus, httpSessionId and sessionCookieName handling have been removed from most parts of the code as these are not interesting any longer. The webapp's contextPath has been added though, to be able to set the correct path on the DWRSESSIONID cookie.

Call ordering has been fixed so that there is an internal property _internalOrdered which is used by the script session id logic before we have obtained the DWRSESSIONID. This way we don't need to set/reset any user setting for _ordered. It is also fixed wrt sync calls as it will now let sync calls execute irrespective of the ordered setting. This is our only way to deal with this, as we can't do programmatic blocking ourselves while holding back a sync call. Even though this results in two parallel generateId() calls (one async and one sync), we still protect ourselves from creating two script sessions as only the first of the two returned ids will be activated and used in the following calls, and it is not until this point the script session will be created on the server.

So, when doing the first call from a new browser process, the call logic will obtain the DWRSESSIONID by intercepting the call and inserting an additional call to __System.generateId() before the real call. If the real call is synchronous we will do a synchronous call to generateId() to be able to deliver a blocking experience all the way to the completed real call. When generateId() returns, and before executing the real call, DWRSESSIONID and the script session id will be set up. As this is done transparently together with the first call, the DWRSESSIONID and script session id will not be set up before the first call from a new browser (the first call could be poll, pageLoaded, etc though). If there have been previous calls done from other pages in the same process, the DWRSESSIONID cookie will be set up (if cookies enabled), and the script session id will be available immediately on page load without doing any remote calls.

Show
Mike Wilson added a comment - 15/Sep/10 11:36 AM This feature has now been implemented and also resolves DWR-17, DWR-338, DWR-364, and DWR-433. Following is a description of the implementation and the changes done, to be used for documentation and release notes: The client layer (engine.js) is now in full control over the script session id and determines when it wants to assign this id. The id has been split up in two parts; one common for the whole browser process, named the DWR session, and one part unique for each loaded page in a browser window. The DWR session is stored as a DWRSESSIONID session cookie and normally only has to be assigned on the first request of a session. It has nothing to do with the HttpSession. If cookies are disabled this id will be set up for each page load. The client layer takes help from a new __System.generateId() call to obtain a secure and globally unique random string from the server. The page-local part of the id is generated locally on each page load. The server-side id generator has been replaced with a more secure version and promoted to a plugin:able bean in the container, so there is now an interface in the extend package and the implementation has also moved package-wise. And there is a new entry in defaults.properties. As the client layer determines the script session id there is no longer any id resync when a script session has timed out on the server side, it will just be recreated with the same id when there is a call from the same page. Calls from the client will no longer attach the httpSessionId in the batch data as we often will not have access to it (httpOnly). For the CSRF scheme we are instead relying on the first part of the script session id, separated from the window specific part with a slash, which will mirror the DWRSESSIONID cookie value in the same request. Security comes from a CSRF attackers inability to assign this cookie. Thus, httpSessionId and sessionCookieName handling have been removed from most parts of the code as these are not interesting any longer. The webapp's contextPath has been added though, to be able to set the correct path on the DWRSESSIONID cookie. Call ordering has been fixed so that there is an internal property _internalOrdered which is used by the script session id logic before we have obtained the DWRSESSIONID. This way we don't need to set/reset any user setting for _ordered. It is also fixed wrt sync calls as it will now let sync calls execute irrespective of the ordered setting. This is our only way to deal with this, as we can't do programmatic blocking ourselves while holding back a sync call. Even though this results in two parallel generateId() calls (one async and one sync), we still protect ourselves from creating two script sessions as only the first of the two returned ids will be activated and used in the following calls, and it is not until this point the script session will be created on the server. So, when doing the first call from a new browser process, the call logic will obtain the DWRSESSIONID by intercepting the call and inserting an additional call to __System.generateId() before the real call. If the real call is synchronous we will do a synchronous call to generateId() to be able to deliver a blocking experience all the way to the completed real call. When generateId() returns, and before executing the real call, DWRSESSIONID and the script session id will be set up. As this is done transparently together with the first call, the DWRSESSIONID and script session id will not be set up before the first call from a new browser (the first call could be poll, pageLoaded, etc though). If there have been previous calls done from other pages in the same process, the DWRSESSIONID cookie will be set up (if cookies enabled), and the script session id will be available immediately on page load without doing any remote calls.
Hide
peter bryant added a comment - 23/Dec/10 1:07 PM

I appear to be ending up with multiple DWRSESSIONID cookies. Different paths. The 'wrong one' may be getting sent back to the server. and I'm getting CSRF error popups as a result. yucky. Any way I can control what path is set on the cookie, or do you have a better solution?

Name DWRSESSIONID
Value LktG1mV8GS7OZSRTkDGEpL4t7Qi
Host rimuhosting.com
Path /
Secure No
Expires At End Of Session

Name DWRSESSIONID
Value Wxh$O4tthZQ9GRCzLBRtrXrr7Qi
Host rimuhosting.com
Path /cp/
Secure No
Expires At End Of Session

Show
peter bryant added a comment - 23/Dec/10 1:07 PM I appear to be ending up with multiple DWRSESSIONID cookies. Different paths. The 'wrong one' may be getting sent back to the server. and I'm getting CSRF error popups as a result. yucky. Any way I can control what path is set on the cookie, or do you have a better solution? Name DWRSESSIONID Value LktG1mV8GS7OZSRTkDGEpL4t7Qi Host rimuhosting.com Path / Secure No Expires At End Of Session Name DWRSESSIONID Value Wxh$O4tthZQ9GRCzLBRtrXrr7Qi Host rimuhosting.com Path /cp/ Secure No Expires At End Of Session
Hide
David Marginian added a comment - 23/Dec/10 1:52 PM

Peter what build are you using?

Show
David Marginian added a comment - 23/Dec/10 1:52 PM Peter what build are you using?
Hide
peter added a comment - 23/Dec/10 2:53 PM

was trunk -ALL-103 from bamboo updating to latest to try that.

Show
peter added a comment - 23/Dec/10 2:53 PM was trunk -ALL-103 from bamboo updating to latest to try that.
Hide
David Marginian added a comment - 23/Dec/10 3:51 PM

Ok, let us know. This was new functionality recently added and there was a minor issue that Mike fixed.

Show
David Marginian added a comment - 23/Dec/10 3:51 PM Ok, let us know. This was new functionality recently added and there was a minor issue that Mike fixed.
Hide
peter added a comment - 23/Dec/10 5:01 PM

same issue on latest trunk artifacts. seeing multiple cookies. each with diff paths. are you setting paths on the cookie? how do we control that? PS I'm using dwr on various pages, paths, of course. e.g / /cp /cp/vps .

Show
peter added a comment - 23/Dec/10 5:01 PM same issue on latest trunk artifacts. seeing multiple cookies. each with diff paths. are you setting paths on the cookie? how do we control that? PS I'm using dwr on various pages, paths, of course. e.g / /cp /cp/vps .
Hide
David Marginian added a comment - 23/Dec/10 7:24 PM

Peter, we are going to have to wait for Mike, he knows a lot more about this and I don't want to give you incomplete or incorrect answers.

Show
David Marginian added a comment - 23/Dec/10 7:24 PM Peter, we are going to have to wait for Mike, he knows a lot more about this and I don't want to give you incomplete or incorrect answers.
Hide
Mike Wilson added a comment - 24/Dec/10 4:00 AM

Hi Peter,

Yes we are settings path on the cookie, see engine.js:
document.cookie = "DWRSESSIONID=" + dwrsess + "; path=" + dwr.engine._contextPath;
The _contextPath field is set from the server.

Are you possibly using several DWR instances, resulting in multiple context paths, or doing URL rewriting on a facing webserver?

Show
Mike Wilson added a comment - 24/Dec/10 4:00 AM Hi Peter, Yes we are settings path on the cookie, see engine.js: document.cookie = "DWRSESSIONID=" + dwrsess + "; path=" + dwr.engine._contextPath; The _contextPath field is set from the server. Are you possibly using several DWR instances, resulting in multiple context paths, or doing URL rewriting on a facing webserver?
Hide
peter added a comment - 24/Dec/10 11:29 AM

I'm running two dwr servlets. (different paths).

Show
peter added a comment - 24/Dec/10 11:29 AM I'm running two dwr servlets. (different paths).
Hide
Mike Wilson added a comment - 25/Dec/10 6:16 AM

Hm, that is indeed a case I haven't tested explicitly. Though, we are using the contextPath, and not servletPath, here so we shoudn't really be affected by two different servlet mappings. Could you debug your setup a bit and tell us your paths and where in the request sequence the unwanted cookie appears?

Show
Mike Wilson added a comment - 25/Dec/10 6:16 AM Hm, that is indeed a case I haven't tested explicitly. Though, we are using the contextPath, and not servletPath, here so we shoudn't really be affected by two different servlet mappings. Could you debug your setup a bit and tell us your paths and where in the request sequence the unwanted cookie appears?
Hide
Mike Wilson added a comment - 25/Dec/10 9:31 AM

And, are these two dwr servlets in the same, or different, webapps?

Show
Mike Wilson added a comment - 25/Dec/10 9:31 AM And, are these two dwr servlets in the same, or different, webapps?
Hide
peter added a comment - 28/Dec/10 1:41 PM

I see the cookie is set as:

document.cookie = "DWRSESSIONID=" + dwrsess + "; path=" + dwr.engine._contextPath;

I see that my context is "" (not "/" or not "/someotherpath"). Resulting in:

dwr.engine._contextPath = "";

And then my browser is just assigning whatever webapp directory I happen to be in on my request to the cookie. Resulting in multiple cookies being set with different paths.

A fix for webapps being deployed in the root context is then something like this:

diff -r /tmp/dwr-src 2/org/directwebremoting/servlet/EngineHandler.java ./org/directwebremoting/servlet/EngineHandler.java
86,87c86,91
< replace.put("${contextPath}", request.getContextPath());
< String contextServletPath = request.getContextPath() + request.getServletPath();

> String contextPath = request.getContextPath();
> if("".equals(contextPath)) { > contextPath ="/"; > }
> replace.put("${contextPath}", contextPath);
> String contextServletPath = contextPath + request.getServletPath();

Additionally consider in checkNotCsrfAttack checking all the cookies and only throwing an error if none of the cookies is valid (vs. throwing an error if we see an invalid one before a valid one).

Show
peter added a comment - 28/Dec/10 1:41 PM I see the cookie is set as: document.cookie = "DWRSESSIONID=" + dwrsess + "; path=" + dwr.engine._contextPath; I see that my context is "" (not "/" or not "/someotherpath"). Resulting in: dwr.engine._contextPath = ""; And then my browser is just assigning whatever webapp directory I happen to be in on my request to the cookie. Resulting in multiple cookies being set with different paths. A fix for webapps being deployed in the root context is then something like this: diff -r /tmp/dwr-src 2/org/directwebremoting/servlet/EngineHandler.java ./org/directwebremoting/servlet/EngineHandler.java 86,87c86,91 < replace.put("${contextPath}", request.getContextPath()); < String contextServletPath = request.getContextPath() + request.getServletPath(); — > String contextPath = request.getContextPath(); > if("".equals(contextPath)) { > contextPath ="/"; > } > replace.put("${contextPath}", contextPath); > String contextServletPath = contextPath + request.getServletPath(); Additionally consider in checkNotCsrfAttack checking all the cookies and only throwing an error if none of the cookies is valid (vs. throwing an error if we see an invalid one before a valid one).
Hide
Mike Wilson added a comment - 28/Dec/10 2:40 PM

Great Peter, that's really helpful! I'll run this through myself and validate, and commit a/the fix.

WRT checkNotCsrfAttack; I'd like to ensure that we don't create multiple DWRSESSIONID cookies. If there are multiple cookies I would rather issue an error for that, rather than trying each one in turn. What are your thoughts?

Show
Mike Wilson added a comment - 28/Dec/10 2:40 PM Great Peter, that's really helpful! I'll run this through myself and validate, and commit a/the fix. WRT checkNotCsrfAttack; I'd like to ensure that we don't create multiple DWRSESSIONID cookies. If there are multiple cookies I would rather issue an error for that, rather than trying each one in turn. What are your thoughts?
Hide
peter added a comment - 28/Dec/10 2:59 PM

I hit issues with my previous patch. (seeing paths in calls like //admin and my browser sending requests off to an 'admin' host). The following diff is probably better.
1347c1347
< document.cookie = "DWRSESSIONID=" + dwrsess + "; path=" + dwr.engine._contextPath;

> document.cookie = "DWRSESSIONID=" + dwrsess + "; path=" + ("" === dwr.engine._contextPath ? "/" : dwr.engine._contextPath);

WRT checkNotCsrfAttack I don't have a strong preference. It should be a non-issue, as you say. But if they have a valid cookie then the request should be fine. Doesn't matter to user if they also have an invalid one. Log as error on server side only and avoid it impacting on user wherever possible?

Show
peter added a comment - 28/Dec/10 2:59 PM I hit issues with my previous patch. (seeing paths in calls like //admin and my browser sending requests off to an 'admin' host). The following diff is probably better. 1347c1347 < document.cookie = "DWRSESSIONID=" + dwrsess + "; path=" + dwr.engine._contextPath; — > document.cookie = "DWRSESSIONID=" + dwrsess + "; path=" + ("" === dwr.engine._contextPath ? "/" : dwr.engine._contextPath); WRT checkNotCsrfAttack I don't have a strong preference. It should be a non-issue, as you say. But if they have a valid cookie then the request should be fine. Doesn't matter to user if they also have an invalid one. Log as error on server side only and avoid it impacting on user wherever possible?
Hide
Mike Wilson added a comment - 28/Dec/10 3:22 PM

Your new patch is so straight forward I can apply it straight away. Your reasoning about not impacting the user makes sense. I'll fix and check in tomorrow.

Show
Mike Wilson added a comment - 28/Dec/10 3:22 PM Your new patch is so straight forward I can apply it straight away. Your reasoning about not impacting the user makes sense. I'll fix and check in tomorrow.
Hide
Mike Wilson added a comment - 29/Dec/10 5:08 PM

I've commited fixes and it is available in
http://directwebremoting.org/bamboo/browse/DWRTRUNK-ALL-152
and onwards.
Thanks for your report and investigation!

Show
Mike Wilson added a comment - 29/Dec/10 5:08 PM I've commited fixes and it is available in http://directwebremoting.org/bamboo/browse/DWRTRUNK-ALL-152 and onwards. Thanks for your report and investigation!

People

Dates

  • Created:
    28/Mar/07 2:40 PM
    Updated:
    27/Apr/11 1:30 PM
    Resolved:
    15/Sep/10 11:36 AM