The session-hijacking fixes from https://www.drupal.org/SA-CORE-2014-006 look like they would be relevant to Drupal 8 too and need to be ported there.

See the session.inc changes from http://cgit.drupalcode.org/drupal/commit/?id=81586d9e9d04dcee487c50de426...

Patch credit (for the D6/D7 fix): klausi, David_Rothstein, pwolanin

Note that there are some very nice functional tests (written by klausi) but we didn't release them as part of the security fix because they represent too much of a roadmap that shows an attacker how to exploit this. The idea was to hold on to them for a little while and then make them public and commit them later on (once people have had plenty of time to update).

In the meantime, the patch could go into Drupal 8 without tests, or maybe with some lower-level tests that check the very basic fact of how the session-reading function behaves when it encounters an empty session ID.

CommentFileSizeAuthor
#4 2378699.patch721 bytesklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Related issues: +#2378703: Port denial of service fixes from SA-CORE-2014-006 to Drupal 8

Adding link to the related issue for the other half of the security advisory.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Postponed

Postponing due to #2342593: Remove mixed SSL support from core. If we decide not to do that we can re-open.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Postponed » Active

Since we decided to fix this in Drupal 6 also which has no mixed HTTPS mode support in core we should also do so for D8. Empty session IDs are never acceptable, so this is a legit flaw in the code base which should be fixed regardless of the outcome of #2342593: Remove mixed SSL support from core.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Active » Needs review
FileSize
2378699.patch721 bytes

klausi opened a new pull request for this issue.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

Patch is green and we don't want to post a test for this right now. RTBC anyone?

I will publish the test cases I developed after December 19th when upgrading to Drupal 7.34 is done on most sites.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Needs review » Reviewed & tested by the community

Yup, this is good to go. Equivalent patch to Drupal 7, and looking at the surrounding code I don't see any reason it should be different in Drupal 8.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +D8 upgrade path
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 040dc5f and pushed to 8.0.x. Thanks!

  • alexpott committed 040dc5f on 8.0.x 
    Issue #2378699 by klausi, David_Rothstein, pwolanin: Port session...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

D7 test cases will be added in #2399657: Add session hijacking test cases for SA-CORE-2014-006.