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

[dev.icinga.com #11187] Session cookie: Path too broad and unset secure flag on HTTPS #2307

Closed
icinga-migration opened this issue Feb 18, 2016 · 7 comments

Comments

@icinga-migration
Copy link
Member

@icinga-migration icinga-migration commented Feb 18, 2016

This issue has been migrated from Redmine: https://dev.icinga.com/issues/11187

Created by elabedzki on 2016-02-18 16:03:18 +00:00

Assignee: elippmann
Status: Resolved (closed on 2016-02-27 21:50:05 +00:00)
Target Version: 2.2.0
Last Update: 2016-02-27 21:50:05 +00:00 (in Redmine)


Hello,
 
Requirement of corporate security for Web applications is that in HTTPS connections for at least the session cookie that attribute "secure" must be set:
 
If the session is secured using TLS, the Web application in the session cookie must set the attribute "secure".
 
Motivation: The "secure" attribute prevents the browser cookies sent unencrypted. This happens for example when the unencrypted contents of a web application. But this can also be done through an active attack in which an attacker unencrypted links or references injected or presented. Because of this the injected contents of the browser is the session ID send unencrypted if the attribute "secure" is not set.
 
The Web application must set so restrictive in the session cookie attribute "path" that the cookie will not be sent to other Web applications on the same host.
Web applications can have the same host name, but are in different directories. It must therefore be ensured that different web applications will not get on the same host, the cookies of the other applications. If the attribute "path" specified when a cookie is set, it is only valid in this directory and all subdirectories. The "path" attribute must therefore be set so that no other Web application receives the session cookie.
 
For example, a web application under telekom.net/meineAnwendung/index.jsp sets a cookie with the path "; path = /". The cookie is then sent with all requests to the domain telekom.net, possibly at other, less trusted applications that have been placed in the root or in any other directories.
 
If the path, however, to "; path = / myApp /", the cookie is sent only with requests to domain.org/myApp/ (and also by all of the underlying sub-directories, but not overlying directories). The final slash character must not be omitted because the cookie is otherwise sent to other directories with matching names, z. B. to telekom.net/meineAnwendung-exploited.
 
If no path is specified, the browser uses as default the path of the current HTTP request, based on which the cookie was set.
Motivation: A restrictive use of the "path" attribute prevents the session cookie is sent to other Web applications.
 
You sit here "/" as the path, not "/ icingaweb2 /"

Protected should be yes because if the path "/ icingaweb2 /" or whatever is called, is yes certainly "/ foobar /", which presumably. will collide.

Best regards

Changesets

2016-02-26 13:26:10 +00:00 by aklimov f46f10d

Web::bootstrap(): set up the request before setting up the session

refs #11187

2016-02-26 14:49:05 +00:00 by aklimov 5b86493

PhpSession: implement isSecure()

refs #11187

2016-02-26 15:22:18 +00:00 by aklimov 8132d95

PhpSession: implement getCookiePath()

refs #11187

2016-02-26 17:03:02 +00:00 by aklimov a790b4d

PhpSession: implement getDomain()

refs #11187

2016-02-26 17:05:59 +00:00 by aklimov 05ef689

Override the following parameters of a session cookie: path, domain, secure, httponly

refs #11187

2016-02-27 21:17:01 +00:00 by aklimov 923e902

Web::bootstrap(): set up the request before setting up the session

refs #11187

2016-02-27 21:19:37 +00:00 by elippmann 5f64287

Respect cookie domain config in Cookie.php

refs #11187

2016-02-27 21:24:01 +00:00 by elippmann 5f43ac8

Fix path, secure flag and domain of session cookies

refs #11187

2016-02-27 21:42:32 +00:00 by elippmann 03d7f3a

Ensure trailing slash if cookie path is detected automatically

Seems like IE (8, 9, ?) has problems w/o the trailing slash and additional directories on the server that start w/ the path.

refs #11187

2016-02-27 21:47:20 +00:00 by elippmann 25f5969

Merge branch 'bugfix/session-cookie-11187'

fixes #11187
@icinga-migration

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Feb 18, 2016

Updated by elabedzki on 2016-02-18 16:04:33 +00:00

[netways #457131]: Icingaweb2 / session cookies

@icinga-migration

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Feb 18, 2016

Updated by tgelf on 2016-02-18 17:45:07 +00:00

  • Priority changed from Normal to High
  • Target Version set to 2.2.0

I can confirm this, the current behaviour is not acceptable. We tried to do the right thing and badly failed :p

secure flag

Icinga\Web\Cookie takes care about this, in theory. It uses secure for HTTPS as a default and allows config to override this (sometimes important for installation behind proxies). However, this method has a bug and returns the wrong value. Even worse, the corresponding code is nonetheless not used for our cookies as of the following reasons:

  • the session cookie does not use this class as of technical reasons, but has no corresponding logic implemented <- this is the only severe issue here and needs to be addressed immediately, it's a security issue
  • the application-state cookie (not security-relevant) does not use Web\Cookie but fetches the session cookie settings. And as we learned, those are wrong
  • the tzo cookie is not that important and set on client side. Should also set secure for HTTPS, just to make it look better. This one can of course not be httpOnly
  • the worst one is the cookie-check cookie. Absolutely not security-relevant, however it doesn't even try to do the right thing

Cookie path

It's incorrect and always used to be. Should be fixed, a config setting should allow to override the auto-detected path.

Regression tests

Please implement a test that greps our whole codebase for setcookie. It should not be allowed anywhere but in library/Icinga/Web/Response.php and in library/Icinga/Web/Session/PhpSession.php.

Thanks,
Thomas

@icinga-migration

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Feb 18, 2016

Updated by tgelf on 2016-02-18 17:45:21 +00:00

  • Category changed from Accessibility to Framework
@icinga-migration

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Feb 26, 2016

Updated by aklimov on 2016-02-26 14:03:29 +00:00

  • Status changed from New to Assigned
  • Assigned to set to aklimov
@icinga-migration

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Feb 26, 2016

Updated by aklimov on 2016-02-26 17:13:17 +00:00

  • Status changed from Assigned to Feedback
  • Assigned to changed from aklimov to elippmann
@icinga-migration

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Feb 27, 2016

Updated by elippmann on 2016-02-27 21:08:05 +00:00

  • Subject changed from Icinga Web2 session cookies and path not well formated and not HTTPS compliant to Session cookie: Path too broad and unset secure flag on HTTPS
@icinga-migration

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Feb 27, 2016

Updated by elippmann on 2016-02-27 21:50:05 +00:00

  • Status changed from Feedback to Resolved
  • Done % changed from 0 to 100

Applied in changeset 25f5969.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.