Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Security hardening: Set secure cookie flag when using SSL #8474

Closed
wants to merge 1 commit into from

Conversation

Whissi
Copy link

@Whissi Whissi commented Sep 11, 2016

Even if a website is already accessible via HTTPS most setups provide a
fallback for HTTP users to redirect to HTTPS.

In this scenario Contao can leak sensible *_USER_AUTH cookies: Imagine a
user is in the same network like an attacker. If the attacker manages to
trick the user to access the Contao website via HTTP the attacker can
capture the user's session.

This commit mitigates the problem by activating the secure flag for cookies
when using SSL.

Tested against v3.5.16.

Even if a website is already accessible via HTTPS most setups provide a
fallback for HTTP users to redirect to HTTPS.

In this scenario Contao can leak sensible *_USER_AUTH cookies: Imagine a
user is in the same network like an attacker. If the attacker manages to
trick the user to access the Contao website via HTTP the attacker can
capture the user's session.

This commit mitigates the problem by activating the secure flag for cookies
when using SSL.
@leofeyer
Copy link
Member

@contao/developers Is this a bugfix or a new feature?

@ausi
Copy link
Member

ausi commented Sep 14, 2016

I think this would not be fully backwards compatible. If someone actually wants to share the cookie between HTTP and HTTPS it would not work anymore.

In favor of security it may be OK to break BC here.

But wasn’t there an issue which prevented the use of cookie secure? @Toflar has it on his list in contao/core-bundle#574.

@xchs
Copy link
Contributor

xchs commented Sep 14, 2016

Related issue: contao/core-bundle#443

@Whissi
Copy link
Author

Whissi commented Sep 14, 2016

Yes, it will break sites which serves HTTP and HTTPS at the same time.
Well, it doesn't really break something, it just doesn't allow you to downgrade.

I.e. when coming from HTTPS with HTTPS-only Cookie and downgrade to HTTP then the HTTPS-only Cookie is missing and the "session" looks like a fresh session (i.e. you would have to re-login and stuff like that).
If you are coming from HTTP and upgrade to HTTPS this won't be a problem.

I don't consider that as an issue. For me a site which allows HTTP-downgrade has an improper configuration (this site will for example also have a duplicated content problem because search engines like Google treats HTTP and HTTPS as different sites).

@leofeyer
Copy link
Member

@contao/developers Bug or feature?

@ausi
Copy link
Member

ausi commented Sep 16, 2016

IMO Feature

@aschempp
Copy link
Member

aschempp commented Sep 16, 2016 via email

@leofeyer leofeyer added this to the 4.3.0 milestone Sep 16, 2016
@discordier
Copy link
Contributor

Feature but I assume an obsolete one with the auth PR by @bytehead.

@Toflar
Copy link
Member

Toflar commented Sep 26, 2016

It is a feature but a very important one. That's why I already started working on it and unfortunately there are still show stoppers. See task (and prerequisite) number 5 in contao/core-bundle#574. If this blocker is out of the way we can even support both, page tree's using https and page tree's using http in the same installation.

@leofeyer
Copy link
Member

While porting the changes to Contao 4.3, I noticed another scenario that would break.

  1. Think of a multi-domain installation with one website being available via HTTPS and the others only via HTTP.
  2. The option "search protected pages" is enabled.
  3. The back end is available via HTTPS, too.
  4. In the maintenance module, the search index is rebuilt with a front end user logged in.

Since the back end is called via HTTPS, the cookies will be set with the secure flag. But since only one website is available via HTTPS, the front end user would not be logged in on the other website and the protected pages would not be indexed.

@leofeyer leofeyer removed this from the 4.3.0 milestone Oct 28, 2016
@aschempp
Copy link
Member

The cookie cannot be set for a foreign domain anyway...

@leofeyer
Copy link
Member

That is true.

@leofeyer leofeyer added this to the 4.3.0 milestone Oct 31, 2016
@leofeyer leofeyer self-assigned this Oct 31, 2016
leofeyer added a commit to contao/core-bundle that referenced this pull request Oct 31, 2016
@leofeyer
Copy link
Member

Implemented in contao/core-bundle@05e725a.

@leofeyer leofeyer closed this Oct 31, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants