Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

httpOnly flag in CookieCsrfTokenRepository configuration #6151

Closed
wangf1122 opened this issue Feb 5, 2022 · 6 comments
Closed

httpOnly flag in CookieCsrfTokenRepository configuration #6151

wangf1122 opened this issue Feb 5, 2022 · 6 comments

Comments

@wangf1122
Copy link
Collaborator

The CookieCsrfTokenRepository was developed in this commit

8ed2a12

The httpOnly flag was set to false which is less restrictive. Here is the configuration

<bean class="org.fao.geonet.security.web.csrf.CookieCsrfTokenRepository"
id="csrfTokenRepository">
<property name="cookieHttpOnly" value="false"/>
</bean>

Just wonder what is the logic behind why it's been set to false.

@ianwallen
Copy link
Contributor

According to the following

https://security.stackexchange.com/questions/175536/does-a-csrf-cookie-need-to-be-httponly

The httpOnly option is not required on CSRF cookies. I'm guessing that it why it was set to false.

@ianwallen
Copy link
Contributor

My understanding from the documents I have read is that the httpOnly cookie is not required for the CSRF cookie but it does not indicate that it is not required for the other cookies. So I suspect that it should be enabled for other cookies.

Based on the following, I can see that the httpOnly flag is not set to true for the serverTime and SessionExpiry time and I think it should be set for those cookies.

image

This test was done looking at the main search page from https://vanilla.geocat.net/geonetwork/srv/eng/catalog.search#/home (3.12.2.snapshot)
And I see the same result on https://apps.titellus.net/geonetwork/srv/eng/catalog.search#/home (4.1.0.snapshot?)

@josegar74
Copy link
Member

@ianwallen, in web.xml the http-only parameter is set to true. In your screenshot I see the cookies without HttpOnly set to true have a path /, maybe that is related, but I need to check where are that cookies defined:

<session-config>
<tracking-mode>COOKIE</tracking-mode>
<session-timeout>${sessionTimeout}</session-timeout>
<cookie-config>
<http-only>true</http-only>
<secure>${cookieSecureFlag}</secure>
</cookie-config>
</session-config>


Related to CSRF cookies, probably is less restrictive due to old Jeeves services still in use. I guess Spring MVC services should manage it automatically, but Jeeves services probably require to inject the CSRF token in the javascript code, but to check.

@josegar74
Copy link
Member

The cookies with path / are defined in:

Cookie cookie = new Cookie("serverTime", "" + currTime);
cookie.setPath("/");

if (userSession != null && StringUtils.isNotEmpty(userSession.getName())) {
long expiryTime = currTime + session.getMaxInactiveInterval() * 1000;
cookie = new Cookie("sessionExpiry", "" + expiryTime);
} else {
cookie = new Cookie("sessionExpiry", "" + currTime);
}
cookie.setPath("/");

I'll check if setting the application path solves the issue.

@juanluisrp
Copy link
Contributor

@josegar74 @ianwallen there are other places where cookies are set without the secure flag. Should they be changed to secure too? It also has the setPath("/"):

  • // Set the language of the request as the preferred language in a cookie
    final Cookie langCookie = new Cookie(Jeeves.LANG_COOKIE, srvReq.getLanguage());
    langCookie.setMaxAge((int) TimeUnit.DAYS.toSeconds(7));
    langCookie.setComment("Keeps the last language chosen to be the preferred language");
    langCookie.setVersion(1);
    langCookie.setPath("/");
    res.addCookie(langCookie);

@josegar74
Copy link
Member

@juanluisrp I didn't see that cookie in the browser, but sure, should be updated.

Please check to make a PR with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants