This issue was closed duplicate of #2245003: Use a random seed instead of the session_id for CSRF token generation by @catch in #293.

This is a follow-up to #1577: Mapping privilege separation on non-SSL/SSL.

Postponed on #2245003: Use a random seed instead of the session_id for CSRF token generation

Steps to reproduce:

  • Install HEAD
  • Apply the following patch, which sets the #https key on the comment reply form (obviously users will use hook_form_alter; hacking comment.module is for demonstration purposes)
    --- a/modules/comment/comment.module
    +++ b/modules/comment/comment.module
    @@ -1836,6 +1836,7 @@ function comment_form($form, &$form_state, $comment) {
       // comments on nodes.
       if (empty($comment->cid) && empty($comment->pid)) {
         $form['#action'] = url('comment/reply/' . $comment->nid);
    +    $form['#https'] = TRUE;
       }
    
       if (isset($form_state['comment_preview'])) {
    
  • add $conf['https'] = TRUE; to settings.php
  • log as admin via https://example.com/user (SSL)
  • create an article node
  • navigate to http://example.com/node/1 (PLAIN HTTP)
  • Submit the comment form.

Expected result: The comment is saved.

Actual result: "This form is outdated. Reload the page and try again. Contact the site administrator if the problem persists."

For a more thorough demonstration, you can grab the DRUPAL-7--1 branch of securepages and run the tests.

I think it's caused by the dual session cookies, which causes session_id() to vary between requests, which means form tokens don't validate. I assume (though haven't tested) this would also cause anything that relied on drupal_valid_token() (such as /comment/42/approve?token=bczrJ7m...) to fail if redirected to HTTPS.

In D6, the securepages_prevent_hijack module implements similar dual-cookie functionality. However the secondary secure cookie isn't an actual PHP session cookie, it simply a hash from drupal_get_token() that authenticates the primary session. In this way session_name(), session_id(), and drupal_valid_token() work normally.

One last note - I realize an insecure form with an HTTPS action is not secure at all - there's no way to know the form itself hasn't been tampered with. An attacker could rename all the form elements, and substitute them with hidden fields of his own choosing. Or install a javascript keylogger. Or steal the form token and use it for an XSRF. Similarly, redirecting an unencrypted ?token=bczrJ7m link is asking for XSRF.
Nonetheless, we have the #https FAPI key - which appears to have been designed to support this use case.

CommentFileSizeAuthor
#287 interdiff-285-287.txt582 bytesmartin107
#287 https-session-cookies-961508_287.patch9.88 KBmartin107
#285 https-session-cookies-961508_285.patch9.74 KBmartin107
#285 interdiff-281-285.txt4.21 KBmartin107
#283 interdiff-279-281.txt895 bytesmartin107
#281 interdiff-279-281.txt0 bytesmartin107
#281 https-session-cookies-961508_281.patch9.47 KBmartin107
#279 https-session-cookies-961508_279.patch9.49 KBmartin107
#276 interdiff-273-276.txt2.7 KBmartin107
#276 https-session-cookies-961508_276.patch9.5 KBmartin107
#273 https-session-cookies-961508_273.patch10.26 KBmartin107
#266 interdiff-260-266.txt881 bytesmartin107
#266 https-session-cookies-961508_266.patch10.24 KBmartin107
#260 https-session-cookies-961508_260.patch10.24 KBmgifford
#252 interdiff-961508_1-251.patch993 bytesmgifford
#251 https-session-cookies-961508_251.patch10.18 KBmgifford
#241 961508.patch10.18 KBSalah Messaoud
#232 961508-232.patch1.35 KBraphaelhuefner
#227 interdiff-961508-227.txt5.45 KBdamiankloip
#227 961508-227.patch10.18 KBdamiankloip
#224 961508-224.patch1.25 KBnerdcore
#216 drupal_https_sessions-961508-215_TESTS_ONLY.patch5.37 KBheddn
#216 drupal_https_sessions-961508-215.patch6.54 KBheddn
#214 drupal_https-961508-214-TESTS-ONLY.patch4.26 KBheddn
#214 drupal_https_sessions-961508-214.patch6.01 KBheddn
#214 interdiff.txt2.64 KBheddn
#211 drupal_https-961508-211-TESTS-ONLY.patch4.32 KBheddn
#211 drupal_https_sessions-961508-211.patch6.07 KBheddn
#165 drupal_https_sessions-961508-165-TESTS-ONLY.patch3.74 KBDavid_Rothstein
#165 drupal_https_sessions-961508-165-WRONG-FIX.patch5.26 KBDavid_Rothstein
#165 drupal_https_sessions-961508-165.patch5.31 KBDavid_Rothstein
#164 drupal_https_sessions-961508-164-TESTS-ONLY.patch3.72 KBDavid_Rothstein
#164 drupal_https_sessions-961508-164-WRONG-FIX.patch5.24 KBDavid_Rothstein
#164 drupal_https_sessions-961508-164.patch5.29 KBDavid_Rothstein
#159 961508-159.patch1.53 KBmducharme
#153 961508-153.patch1.54 KBmducharme
#141 961508-141.patch1.69 KBjazzdrive3
#134 drupal_https_sessions-961508-134-TESTS-ONLY.patch3.79 KBDavid_Rothstein
#134 drupal_https_sessions-961508-134.patch5.2 KBDavid_Rothstein
#131 drupal_https_sessions-961508-129.patch7.23 KBmbrett5062
#129 drupal_https_sessions-961508-129.patch7.23 KBmbrett5062
#127 drupal-https_session-961508-127.patch7.23 KBmbrett5062
#124 drupal-https_session-961508-124.patch7.2 KBmbrett5062
#110 drupal-https_session_cookies-961508-110.patch7.76 KBmbrett5062
#106 drupal-https_session_cookies-961508-106.patch7.29 KBmbrett5062
#103 drupal-https_session_cookies-961508-103.patch7.61 KBmbrett5062
#101 drupal-https_session_cookies-961508_101.patch6.99 KBmbrett5062
#98 drupal-https_session_cookies-961508-98.patch6.67 KBmbrett5062
#96 drupal-https_session_cookies-961508-96.patch6.67 KBmbrett5062
#92 drupal-n961508-92-d8.patch6.58 KBmgifford
#90 drupal-n961508-90-d8.patch6.55 KBmgifford
#85 drupal-n961508-85-d8.patch8.33 KBDamienMcKenna
#82 drupal-n961508-82-d8.patch6.92 KBDamienMcKenna
#75 961508-75-https-dual-tokens.patch6.92 KBmr.baileys
#71 drupal-https-only-961508-23-32.patch6.21 KBkrlucas
#32 961508-23-32-D7.patch1.53 KBEverett Zufelt
#23 961508-23.patch1.57 KBxjm
#21 961508-20.patch1.53 KBxjm
#9 961508.patch1.5 KBgrendzy
#5 961508_TEST_ONLY.patch4.93 KBgrendzy
#3 961508.patch840 bytesgrendzy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Add a $sid parameter to drupal_get_token, defaulting to session_sid(). Form API passes in ssid if $form['#https'] is set. If the user doesn't have a ssid then just throw an error.

mfb’s picture

subscribe just for fun (I'm not a fan of $conf['https'] but curious to follow the #action)

grendzy’s picture

Status: Active » Needs review
FileSize
840 bytes

Here's one simple approach that allows the tokens to be shared. Module authors that *want* tokens to be restricted to one protocol can still do so, they'd just need to check $_SERVER['https'] in the validation logic.
(I feel like using $_COOKIE in this patch is a tad clumsy - it could be refined by tweaking session.inc to give us the sid. )

Regarding comment #1, I'd prefer a solution that's broader in scope. Fixing tokens in FAPI only would leave URL tokens unresolved, potentially forcing us to deal with #755584: Built-in support for csrf tokens in links and menu router in D7.

mfb’s picture

Module authors that *want* tokens to be restricted to one protocol can still do so, they'd just need to check $_SERVER['https'] in the validation logic.

My reading of the patch is that the token is received from the server via HTTP, then could be passed back to the server along with the insecure session cookie via either HTTP or HTTPS as a valid token.

It depends how the token is used (I need to dig into the fapi when I have time), but theoretically what chx talks about in #1 would be more secure: you could create a token that would only be valid if passed back with the secure session cookie via HTTPS.

grendzy’s picture

Status: Needs review » Needs work
FileSize
4.93 KB

This patch isn't a fix; it only contains tests for the issue.

BTW mfb: you're right, if we follow the approach in my first patch, drupal_validate_form() should require an HTTPS connection when the #https key is set.

mfb’s picture

to clarify my comment in #4, I was just wondering if there's any chance of token validation alone being used for some sort of privilege escalation (accessing a cached form or something) if the token is passed in via HTTPS by an attacker (with insecure session key but not the secure session key needed for user authentication). I haven't yet had a chance to dig in to how tokens are used (by core or contrib)

grendzy’s picture

Issue tags: +SecurePages
tutumlum’s picture

subscribing

grendzy’s picture

Status: Needs work » Needs review
FileSize
1.5 KB

mfb: I'm pretty sure the risk you ask about isn't an issue, because tokens can never be validated without the correct session_id. Here's a better patch. The tests will go into #1050746: HTTPS sessions not working in all cases

mfb’s picture

@grendzy: The issue is that this patch allows a token to be validated with the insecure session id (which could be intercepted by an attacker via packet sniffing) on the secure site (which otherwise requires the secure session id for user authentication).

This isn't necessarily a problem if both token validation and user authentication are always required by the code that relies on tokens, since an attacker shouldn't be able to authenticate on the secure site with just the insecure session id. But I did want to point out the risk.

grendzy’s picture

Ah, yes. (Too little sleep at drupalcon). As I see it using mixed-mode in the first place means being willing to accept some compromises and also taking responsibility for configuring your modules wisely. I do share your general skepticism about mixed-mode: with modern CPUs, it's a compromise that's becoming less necessary, and with SSL-stripping one that's less effective. Nonetheless this is a use-case that has been possible in the past and still has a place in the ecosystem.

If we can get this in I'll commit to documenting this compromise at http://drupal.org/https-information and in the securepages documentation.

sun’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » task
Issue tags: +Needs security review, +Needs backport to D7

AFAICS, Drupal never supported this so far, so this is a good and possibly backportable improvement, but not a bug.

grendzy’s picture

Category: task » bug

sun, it's a regression bug, because the straightforward use cases (submitting a form with action=https, or clicking a tokenized link) work fine in D5 and D6. There is no known workaround (except possibly forking session.inc, which is pluggable). As it stands both use cases are simply impossible in 7.x.

I'm not really sure what to do next. I've invested probably 30+ hours in this and #1050746 together, but it's really hard to find reviewers. Can you help? I've already asked my colleagues on the security team, on IRC, and friends in the local user group. As always I'm happy to trade reviews or other tasks, but so far everyone has balked when they see the complexity of session.inc.

Akaoni’s picture

You might have better luck if you do some of the thinking for the reviewers.

For example, in #1050746 you either have to install Secure Pages and run the tests or code-read the patch just to know what session issues you've fixed. Unknowns are often written off as too much work. By stating what the patch fixes in words people can get an idea of what's involved before they start. It might even encourage people to review just a portion of the issue rather than the whole (as I did).

Just my $0.02. ;)

xjm’s picture

Tagging issues not yet using summary template.

grendzy’s picture

BenK’s picture

Subscribing

greggles’s picture

I think this tag is appropriate.

grendzy’s picture

The last submitted patch, 961508.patch, failed testing.

xjm’s picture

FileSize
1.53 KB

Needs a reroll?

xjm’s picture

Status: Needs work » Needs review
xjm’s picture

FileSize
1.57 KB

Rerolled for core/.

rickmanelius’s picture

In an effort to move these along by providing feedback, here is a quick overview.

rickmanelius’s picture

Status: Needs review » Reviewed & tested by the community
chx’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for fixing the shoddy work I did with the HTTP/HTTPS mechanism. However, this needs a test. Thanks again.

rickmanelius’s picture

Could anyone elaborate on just what one would test? I'm not quite there in my skill yet to simply know how to proceed with that and googling didn't really turn up anything useful beyond SimpleTest tutorials in general.

Akaoni’s picture

Issue tags: +session

Adding session tag.

JmsCrk’s picture

This is still a problem for me too when trying to use the secure_pages module. If I understand right these patches are not for the secure_pages module but for Drupal core?

hanoii’s picture

For what it's worth, #3 (for D7) and #23 (for D8) are almost identical and #23 applies fine on D7.10 as well.

webchick’s picture

I think a test could fake $GLOBALS['is_http'] and verify that a form with this property submits fine with it set to true, and fails when set to false.

Everett Zufelt’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

Patch from #23 re-rolled against 7.x for drush make file.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Still needs a test I think. Thanks!

Everett Zufelt’s picture

If someone can suggest where (which file) the tests should go, and give a high level summary of what to test I have some funded time to get this done.

grendzy’s picture

Everett, that's great! Here's a quick outline for a test.
Add a new menu item in form_test.module, with the path /form_test/https_only. This page should render a simple form with $form['#https'] = TRUE. Post a mock HTTPS request to that form, via $this->httpsUrl. Assert a successful form submission. Then, repeat the form submission with $this->httpUrl (a mock non-HTTPS request), and assert the form error is present.

Everett Zufelt’s picture

@grendzy

Thanks for the help. I think I can do this.

I created the menu item and form in form_test.module, and I created a new test case in form.test. I notice that httpUrl() and httpsUrl() are not part of the web test case, but are in session.test. What is the preferred way to load the required class?

Also, it looks like what I need to do is get the proper absolute URLs from httpsUrl and httpUrl() and then to do a $this->drupalPost(), is that correct?

grendzy’s picture

Oh, I didn't realize httpsUrl() was in a different class. You might have to move those two functions to DrupalWebTestCase.

Everett Zufelt’s picture

Does anyone disagree with @grendzy's recommendation to move httpUrl() and httpsUrl() to DrupalWebTestCase?

Everett Zufelt’s picture

I moved httpUrl and httpsUrl to DrupalWebTestCase, that is working.

In my test I do:

$this->drupalPost($this->httpsUrl('form_test/https_only'), array('textfield' => '123'), t('Submit'));

This gets the page, but then does a GET back to index.php, instead of simpletest/tests/https.php, since the form returns without the action set accordingly.

1. I understand why drupalPost() needs to do a GET before it does a POST, but I don't understand why the second request is a GET.

2. What is the best way for me to dynamically set the action for this form to the correct absolute URL?

grendzy’s picture

I'm not sure about 1. For 2, it looks like session.test alters the action this way:

    $this->drupalGet('user');
    $form = $this->xpath('//form[@id="user-login"]');
    $form[0]['action'] = $this->httpsUrl('user');
Everett Zufelt’s picture

Okay, using

    $this->xpath('//form[@id="-form-test-https-only-form"]');

Is giving a PHP unexpected t_variable error. Can anyone see the error? This is used in a test function within a class that extends DrupalWebTestCase.

xjm’s picture

Perhaps the hyphen at the beginning of -form-test-https-only-form? I'm not sure though. I've found the xpath error messages not the greatest for debugging.

chaskype’s picture

How do I apply this patch? Does not work the command git (git apply -v 'http://drupal.org/node/707484') -- I'm using 7.10 Dupal

grendzy’s picture

chaskype, the URL in your comment ( http://drupal.org/node/707484 ) is a link to the handbook page documenting how to apply patches, not part of the apply command. #21 will apply to Drupal 7, and #23 is for Drupal 8.

chaskype’s picture

Thank you @grendzy - That was my problem then (I was applying the patch to D7 D8)

Thank you...

anavarre’s picture

anavarre’s picture

Something went wrong with the other tags it seems. Re-adding them...

zipymonkey’s picture

The last submitted patch, 961508-23-32-D7.patch, failed testing.

rickmanelius’s picture

@Everett Zufelt
Did you ever get your test working? Do you have a patch for what you've completed thus far? It seems from a code perspective, no one has any complaints... it's just a test that's holding this up.

Additionally, I'm not sure if anyone gave feedback on #39. Anyone have any complaints with this approach?

Everett Zufelt’s picture

@rickmanelius

Time is my current blocker, I'll add this to my todo list for May 4 if I don't find time to get to it before that.

njcheng’s picture

I really appreciate everyone's work on this, but is there any update? I must admit using the combination of the other modules I've cobbled together to redirect is tough to maintain and not nearly as clean as Secure Pages on Drupal 6.x. Wish I knew enough to contribute.

Robin Monks’s picture

The last submitted patch, 961508-23-32-D7.patch, failed testing.

rickmanelius’s picture

Hey Robin (#54). It seems we're waiting on Everett in #52 because he's already made progress on the testing portion.

coolestdude1’s picture

Can we get an update on this? May 4th was expected to be worked on 2 months later no progress? Preventing secure pages module full release, probably even preventing people from upgrading to drupal 7.

sbandyopadhyay’s picture

@Everett Zufelt
Can you please post what you have so far of the test? It would be helpful to use that as a starting point to build from and get this patch committed. It looks like @xjm's patch from #21/23 would essentially be RTBC, but for this test.

TheBarnacle’s picture

Hi,

Thanks for all the time everyone's put into this. I'm a little confused in relation to Drupal 7 having found this thread on the SecurePages project page under fixes for D7.

Does anybody know if the patch has been put into D7 core (e.g. the latest 7.15 release)? Or is even on the agenda for D7 core development?

Perignon’s picture

Version: 8.x-dev » 7.15

Can we please get this included in core? D7 is touted as a highly optimized platform for eCommerce with Drupal Commerce but yet the basic tentant of doing such business - SSL - is tharwalted by problems in core.

Perignon’s picture

Priority: Major » Critical

Bumping this to critical. Without SSL switching, how can you build an eCommerce site.

hass’s picture

+1 for critical, 7.16 release blocker

alexp999’s picture

Could we see the same priority for this as well?

http://drupal.org/node/471970

Then Secure Pages can be released properly. I have built an e-commerce site around D7, but we can't go live with online sales until these issues are fixed.

David_Rothstein’s picture

Version: 7.15 » 8.x-dev

The patch needs to go into Drupal 8 first (http://drupal.org/node/767608).

But this issue already has the "needs backport to D7" tag and can certainly be backported after that.

frankcarey’s picture

@alexp999 - It's completely valid to "patch" (not hack) core. Just go ahead and apply the patch yourself until this makes it in. Just be careful that you don't loose the patch the next time you update though.

hass’s picture

Patching core is the same like hacking core. We have no idea if it may ever get committed. Missing propper SSL support can be safely seen as security issue with the critical severity. Unbelivable that core got named final with such an issue.

rickmanelius’s picture

Agreed with #66. I've had to reapply at least 2 of the patches with each new Drupal 7.x branch release... and I understand the need to properly place in D8 and backport with tests, etc... but we've had a terrible time getting the momentum to push this through despite the fact that it works, etc.

See my early comments, reviews, etc. I've been running 2 commerce sites with said patches and have also marked this RBTC one at least one occasion. But we're stuck on #52 to provide tests in order to get this fully committed, etc.

If the Drupal community truly takes security seriously, then it's kinda sad that this module STILL doesn't have a full release despite all the help in this thread!

Sorry, didn't mean to rant... you guys all rock. But this patch seems to be in perpetual purgatory...

kingdee40’s picture

We'll see if anyone agrees with me but after reading some of the drupal documentation, this is what I'd like to see.

http://drupal.org/node/767608
Did anyone read the Backport policy? This issue seems like a special case:

Sometimes, the ideal way to fix the bug in Drupal 8 may involve far-reaching changes or refactoring that would not be appropriate to backport. In those cases, it is best to create a separate Drupal 8 issue for the refactoring and cross-link it to the original issue. This allows the minimal fix to be committed to Drupal 8 and backported as quickly as possible, while work on the more far-reaching fix for Drupal 8 can continue at its own pace.

I don't think we need a test, not for d7, the far reaching change for d8 would be to write some tests. I think we should mark this issue as a 7.x issue and create a separate Drupal 8 issue for the refactoring and cross-link it to the original issue. We can submit this minimal fix to d8 and it's already backported for d7.

Then we need to get two experienced people to review the patch in #21 on 7.15 then mark it Reviewed and tested by the community.

http://drupal.org/patch/review

Patches usually need a positive review by at least two people other than the original author before being declared Reviewed and tested by the community. In the normal course of development, patches will go through several revisions, often with changes by multiple authors, before being committed to the official code repository. Although this process isn't foolproof, it helps ensure that code has been peer reviewed for performance, readability, and usability (good), as well as regressions and bugs (bad).

webchick’s picture

No, tests are generally needed for all bug fixes, regardless of version, because that's how we make sure we don't break things ever again. There are once in a great while exceptions to this, but given how critical this functionality is, it seems even more prudent to ensure there are tests for it.

Step 1 is for one of the 78 people following this issue to analyze the work Everett's done and try and get it working. If it's not possible, explain where you got stuck. From there we can figure out if it's worth committing the fix in advance of the test, but hopefully the more likely scenario is just pinging someone with specific knowledge to move it forward once we understand what the blocker is.

krlucas’s picture

I've contact form'd @Everett Zufelt to see if he can provide his work so far.

But I need this patch and need to learn tests so I will take a crack at it.

krlucas’s picture

Here's a patch based on #32 with a test added as suggested by @grendzy in #35. It also moves the httpUrl and httpsUrl methods to DrupalWebTestCase from SessionTestCase.

This doesn't include tests for the changes to drupal_get_token, just the #https form property.

Should I include a test based on #5 (the TEST ONLY patch)?

krlucas’s picture

Status: Needs work » Needs review

Oh, right, the form submission should implicitly test the drupal_get_token changes.

Changing to needs review.

I'm guessing the patch will fail because the issue version is set to 8. I don't know the protocol for changing versions since previous comments have suggested a d8 patch needs to be committed first. My attempt to write a d8 patch this evening has been thwarted by install wizard issues.

Status: Needs review » Needs work

The last submitted patch, drupal-https-only-961508-23-32.patch, failed testing.

maen’s picture

Sorry for disturbing you all, but I'm not sure what to do. I'd like to test this module, but atm it seems that patching is not possible (using drupal 7.14). So what is to do for a beginner with this module or patch atm???
Thanx in advance,

maen

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
6.92 KB

@maen: you can patch D7 by using the patch in #32 -- usual warnings concerning patching Drupal core apply.

I have re-factored the patch from #71 for D8:

  • There was a syntax error in one of the asserts (t()-function not correctly closed).
  • Moved SessionHttpsTest::httpUrl() and SessionHttpsTest::httpsUrl() to WebTestBase since they are now shared between multiple test cases.
  • Removed an excess space from the form_set_error message.
mr.baileys’s picture

Rick G’s picture

How can we get this done?

Maybe if someone creates a new issue with the version set to D7, so a D7 patch will actually pass testing?

DamienMcKenna’s picture

FileSize
6.92 KB

This patch fixes the paths to the https.php/http.php files in WebTestBase.php, but still has problems submitting the form.

DamienMcKenna’s picture

Status: Needs work » Needs review

Lets test it anyway.

Status: Needs review » Needs work

The last submitted patch, drupal-n961508-82-d8.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
8.33 KB

After help from chx on IRC I was able to fix the "Form HTTPS only" test, but the others need work.

Lloyd’s picture

jazzdrive3’s picture

What do we need to do to get this backported to Drupal 7? This is one of the critical patches that is need for serious commerce, and continue to keep Drupal easy to maintain. Can someone in the know list exactly what needs to happen, so we know where we are? I will try to help where I can.

webchick’s picture

In order for the patch to be backported to D7, it first has to be completed, reviewed, and committed to D8. See http://drupal.org/node/767608.

The latest D8 patch is in #85 and is currently failing tests. If you want to push this forward, you should check out a copy of Drupal 8 from Git, apply the patch, and attempt to fix the remaining failing tests. #drupal-contribute on Freenode is a good place to ask for help on this, and there are dedicated core mentoring hours if you are new to core development, to get any basic questions answered.

Thanks for offering to help, rather than just asking "Why is this not done yet?" It's really appreciated.

mgifford’s picture

FileSize
6.55 KB

The patch in #85 failed:

quickstart@qs10:/home/dm8$ git apply drupal-n961508-85-d8.patch
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php:46
error: core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php: patch does not apply

So just re-rolling it.

From the test details, I don't know why we are getting "HTTP response expected 200, actual 403" in this unless it isn't being sent via https. This should work:
$this->assertResponse(200);

The errors in Drupal\system\Tests\Session\SessionTest all look like they should be green. I don't understand what the bot is looking for.

Status: Needs review » Needs work

The last submitted patch, drupal-n961508-90-d8.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
6.58 KB

Nixed an extra }

Status: Needs review » Needs work

The last submitted patch, drupal-n961508-92-d8.patch, failed testing.

esbon’s picture

Subscribing, it is hard to know which patch to use.

mbrett5062’s picture

Assigned: Unassigned » mbrett5062

Taking this over for a little while. Re-rolling against latest head and cleaning some doc blocks.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
6.67 KB

And here is new patch for test bot.

Status: Needs review » Needs work

The last submitted patch, drupal-https_session_cookies-961508-96.patch, failed testing.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
6.67 KB

Missed some forward slashes. Try again.

Status: Needs review » Needs work

The last submitted patch, drupal-https_session_cookies-961508-98.patch, failed testing.

mbrett5062’s picture

Working on fixing tests. Hope to post new patch tomorrow.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
6.99 KB

OK think I have fixed some of the tests. But will probably still fail some. Will continue working on this tomorrow.

Status: Needs review » Needs work

The last submitted patch, drupal-https_session_cookies-961508_101.patch, failed testing.

mbrett5062’s picture

Seem to have cleared some more test errors. The last patch changed form.inc line #1153, but this did not fix the errors, just shifted from SessionTest (which we had not modified) to SessionHttpsTest.

if (!empty($form['#https']) && !$GLOBALS['is_https'])

was changed to

if (!empty($form['#https']) && $GLOBALS['is_https'])

So after further investigation. I saw that in session_test.module we were altering the login form, but unconditionaly.

function session_test_form_user_login_form_alter(&$form) {
  $form['#https'] = TRUE;
}

This gave errors in SessionTest which required #http form.

So changed above to the following.

function session_test_form_user_login_form_alter(&$form) {
  global $is_https;
  if ($is_https) {
    $form['#https'] = TRUE;
  }
}

This now cuts down the errors significantly on my local tests.

Lets see what test bot has to say. I hope this is going in the right direction.

If someone with more experience in tests and PHP then myself would like to chime in, please feel free. I am only a novice.

mbrett5062’s picture

Status: Needs work » Needs review

ooppps and send to bot.

Status: Needs review » Needs work

The last submitted patch, drupal-https_session_cookies-961508-103.patch, failed testing.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
7.29 KB

OK cleared up an incorrect assertion. Now left with only one fail I believe.

Can not figure out why it's failing.

Need the assistance of someone with more experience. Would also like my work checked, as I am not sure I am making changes to the test's correctly. It seems logical to me but could be wrong.

Status: Needs review » Needs work

The last submitted patch, drupal-https_session_cookies-961508-106.patch, failed testing.

pounard’s picture

I might be saying this a bit late, but this patch may be wiped out if #335411: Switch to Symfony2-based session handling gets in.

mbrett5062’s picture

OK, let me get it green anyway. Just one more test. Just in case.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
7.76 KB

OK fixed the final test. Not sure if I am correct in my assumption, but I believe the login page should also be non-secure.

At least if this comes back green and #335411: Switch to Symfony2-based session handling does not get in, we are good to go.

mbrett5062’s picture

Also this work may be of use for backport to D7

pounard’s picture

Yes, it would be great for D7 (and D8 if Symfony session handling does not get in).

jazzdrive3’s picture

Yes, regardless, this needs a backport to Drupal 7, regardless of what ends up happening with 8.

rickmanelius’s picture

This patch for D7 has been ready since #21 (granted, it needs the test coverage added). The issue is simply getting it committed to D8 first so that the backport can be accepted, which would be awesome so that secure pages for D7 can finally get to a 1.0 release without requiring these patches.

krlucas’s picture

#71 added test coverage to the D7 patch. It still needs to be cleaned up but it's almost there:

  • syntax error in one of the asserts (t()-function not correctly closed).
  • excess space from the form_set_error message.
jrviorato’s picture

#21: 961508-20.patch queued for re-testing.

salvis’s picture

This should not be blocked for D7 just because we don't know where D8 goes.

xlyz’s picture

This should not be blocked for D7 just because we don't know where D8 goes.

+1

mbrett5062’s picture

Assigned: mbrett5062 » Unassigned

Un-assigning myself, as my work here is done for now.

David_Rothstein’s picture

Status: Needs review » Needs work

I reviewed the patch and read through the entire issue. Comments:

  1. The drupal_get_token() changes look pretty solid to me. My only question is whether this hurts security in the case of a site that is "mostly" HTTPS (meaning, HTTPS-enough that it does not suffer from this bug in practice) but happens to put $conf['https'] = TRUE in its settings.php file anyway? Because then all tokens on the site will start being derived from the insecure session for no particular reason. I don't see this as a blocker because it could be dealt with via documentation (at http://drupal.org/https-information, and in release notes when this gets backported to Drupal 7), but we do really need to understand what the consequences are for various sites on the entire "all-HTTPS-to-no-HTTPS spectrum" so this can be documented.
  2. This part of the patch I don't understand:
    +  // Ensure the correct protocol when #https is set.
    +  if (!empty($form['#https']) && !$GLOBALS['is_https']) {
    +    form_set_error('', t('This form requires HTTPS. Contact the site administrator if the problem persists.'));
    +  }
    

    First, it's totally changing the meaning of $form['#https'] from "use HTTPS for this form if the server supports it" to "the server must support HTTPS for the form to be submitted". So for example, on my non-HTTPS server, this completely breaks installing modules via the UI (using authorize.php).

    Is the intention to check variable_get('https', FALSE) here too? Then, I think it makes sense.

    However, I'm still not clear on how it relates to the rest of this issue. I read comments #4 and #5 but I'm still not seeing the scenario it protects against. (And in any case, if it's doing something to compensate for a less-secure token, then only doing it in the form API seems problematic since tokens are used outside the form API too.)

    Finally (less important), the message looks outdated... I believe at some point Drupal core moved away from telling people to spam the site administrator :) I think just "This form must be submitted over a secure connection" would work.

  3. As for the tests...
    • Er, aren't the tests themselves missing??? The patch in #85 seems to be the last version that has the new test file :)
    • +    $this->assertText(t('Log in'), 'Login page loaded for first session.');
      

      I can't figure out why these are being added and what they accomplish?

    •      // Check that user login form action is secure.
           $this->drupalGet('user');
           $form = $this->xpath('//form[@id="user-login-form"]');
      -    $this->assertEqual(substr($form[0]['action'], 0, 6), 'https:', 'Login form action is secure');
      +    $this->assertNotEqual(substr($form[0]['action'], 0, 6), 'https:', 'Login form action is not secure');
      

      Well, at a minimum the code comment would need to be changed too, but it seems like a problem to me that this expectation is changing. Isn't it a fundamental part of the test?

    Note that some of the above may be fixed if it turns out we can remove the $form['#https'] changes from this patch (since then we could just remove the corresponding test changes also).

  4. For what it's worth, what ever happened to the tests @grendzy wrote way back in #5? I'm not sure they actually got moved anywhere else, and they look pretty relevant to the drupal_get_token() changes in this issue.
plach’s picture

mbrett5062’s picture

Assigned: Unassigned » mbrett5062

Working on fixing the issues from #120. Thanks for the review @David_Rothstein.

mbrett5062’s picture

OK have addressed some of the issues raised by @David_Rothstein.

1) Not really sure what is required here, maybe just documentation? I too am not sure of the consequences across the entire "all-HTTPS-to-no-HTTPS spectrum". I feel this needs more discussion.

2) I believe what is trying to be achieved is exactly what you quote "the server must support HTTPS for the form to be submitted".
Surely this would only break something if the form is set as HTTPS only. How would that stop you from installing modules on a non-HTTPS server? Maybe I am not understanding your concern correctly.
I would have thought that a server that supports HTTPS could still have insecure URL's, this is exactly what the issue is trying to resolve. Not sending secure forms submissions to insecure URL's, but allowing the form to be submitted to a secure URL on mixed sites.
I have however added variable_get('https', FALSE as you suggested.
Also I believe, again, this needs more investigation/discussion, as you point out tokens are used elsewhere also.
To be honest this whole concept of mixed sessions is confusing, and INMHO fraught with issues. Unless the site admin is particularly careful, there will be mistakes made. Do we really want to go to this much trouble to support such a use case?

3) I missed the actual HTTPS form test on the patch. That is now added.
I too do not understand why those assertions were added, just seemed like noise. I have deleted from the latest patch.
The last point in 3 I agree with you, it was however the only way I could get the tests failure to green. However, that was wrong. I have reverted that in this patch.
4) The tests from #5 where broken and would never pass. In particular the 'token' tests, they were calling drupal_get_token each time, and then testing to see that each time it sent the same token. However, it could not, as the function contains drupal_get_private_key() which returns a unique key each time called.
The other tests were just repeating the functionality of the new FormHttpsOnlyTest.

So finally, this patch will more then likely not pass, and somewhere there is an issue with sessions, and forms, that I have been chasing around in circles, to no avail. I think this needs a more thorough investigation and I personally do not have what it takes. Someone with more in depth knowledge of sessions and session handling needs to unravel this.

mbrett5062’s picture

Assigned: mbrett5062 » Unassigned
Status: Needs work » Needs review

ooppps here bot.

Status: Needs review » Needs work

The last submitted patch, drupal-https_session-961508-124.patch, failed testing.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
7.23 KB

OK addressing the token issue, as drupal_private_key is set as a global variable, have changed the drupal_get_token to use the variable instead of calling drupal_get_private_key.

This should help.

Status: Needs review » Needs work

The last submitted patch, drupal-https_session-961508-127.patch, failed testing.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
7.23 KB

OK lets try that again.

Status: Needs review » Needs work

The last submitted patch, drupal_https_sessions-961508-129.patch, failed testing.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
7.23 KB

Ok missed a ';', lets try that again.

Status: Needs review » Needs work

The last submitted patch, drupal_https_sessions-961508-129.patch, failed testing.

David_Rothstein’s picture

I believe what is trying to be achieved is exactly what you quote "the server must support HTTPS for the form to be submitted".
Surely this would only break something if the form is set as HTTPS only. How would that stop you from installing modules on a non-HTTPS server?

When you install modules via the UI (at admin/modules/install) it often redirects you to the FTP credentials form. This form has $form['#https'] set to TRUE and thus the earlier patches broke that form on non-HTTPS servers. However, I confirmed that the addition of variable_get('https', FALSE) in the newer patch fixes this and restores the behavior to normal.

There's a form in the existing core tests which has $form['#https'] set to TRUE as well, and I'm guessing this may have been the cause of some earlier test failures?

***

The remaining test failure looks to me like we need a variable_set('https', TRUE) to trigger the HTTPS behavior... That combined with the above makes me also wonder if we can't just merge this with the existing tests, which already have a code path that does this and already come very close to testing the same thing. I'll look into it...

I don't think I understand the idea of making $drupal_private_key a global variable? drupal_get_private_key() should work OK in tests as it is (at least I'd think).

David_Rothstein’s picture

This patch contains the code from #124, with the tests for HTTPs forms merged into the existing tests that were already there, as well as a version of the drupal_get_token() tests from #5.

If all goes well, the first patch will have failures in both sets of new tests, and the second patch won't have any :)

I'm still not sure why the $form['#https'] changes are part of this issue, though (even though I think they make sense on their own). Maybe if @mfb or @grendzy are still following this issue they will have an explanation...

David_Rothstein’s picture

Status: Needs work » Needs review
mbrett5062’s picture

David, thanks very much for your help here. I have been pulling my hair out over this.

I see that your improved 'token' tests now only call 'drupal_get_token' once and compare it on HTTPS and HTTP submissions, so yes, it will return the same token, that was exactly the problem with previous version of the test. Calling it twice will never return the same token. The reason I made the change to not calling the function, but retrieving the already stored token was to get over that shortcoming, however your solution seems better IMO.

I think the reason for my issue with the form tests, is that it seemed they were trying for a fail of HTTPS form over HTTP. Exactly what you say we do not want. So yes the new test's pass with variable_get('https', FALSE), but we need it confirmed that this is the expected behavior previously sought.

Anyway, thank you very much for your support here. I am a novice and have been learning a lot as I worked through this.

YesCT’s picture

Status: Needs review » Needs work

sounds like #136 Identified some things that need to be addressed, so marking needs work.

hass’s picture

Status: Needs work » Needs review

It sounds more like needs review.

YesCT’s picture

grendzy’s picture

David_Rothstein:

First, it's totally changing the meaning of $form['#https'] from "use HTTPS for this form if the server supports it" to "the server must support HTTPS for the form to be submitted". So for example, on my non-HTTPS server, this completely breaks installing modules via the UI (using authorize.php).

Is the intention to check variable_get('https', FALSE) here too? Then, I think it makes sense.

Looking at form.inc, I see that the #https key is indeed only considered when $conf['https'] is enabled. So yes I agree the form_set_error() should only fire when in this mode (the latest patch does include this check, so I think we're good).
As to why we set ''This form requires HTTPS" in the first place... it was based on feedback from mfb early in the issue. At the moment I'm having trouble stating a specific threat model that form_set_error() protects against. It's more of a sanity check: something has gone wrong if 1. $form['#https'] is set, 2. $conf['https'] is set, and 3. we're not actually on an SSL page.

I admit I was surprised by the bad effect on authorize.php. I'm not sure, there may be a misunderstanding about (the horribly named) variable $conf['https'] itself. It does not mean merely "my server supports HTTPS" (though I suppose that's implied). It actually means "Enable mixed-mode session handling".

My only question is whether this hurts security in the case of a site that is "mostly" HTTPS (meaning, HTTPS-enough that it does not suffer from this bug in practice) but happens to put $conf['https'] = TRUE in its settings.php file anyway?

Enabling mixed-mode radically changes the behavior of sessions, and if someone "just happens" to enable this out of ignorance, I'm not sure the code can protect them (though documentation could help).

jazzdrive3’s picture

FileSize
1.69 KB

I have re-rolled the required patch against Drupal 7.19 for those who need it, while we are waiting to see what happens.

Thanks.

The last submitted patch, 961508-141.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
AntiThesis’s picture

DanZ’s picture

This issue is 29 months old. Is there anything that can be done to get a fix committed for 7.x? Any tests I could run?

This fix is required for Secure Pages, which is security-critical for many sites. So, core has to be re-patched with every update. It's completely unclear which patch should be applied to the 7.x (currently 7.22), as the minitest results are showing some patches red, some green....

RobLoach’s picture

Direct $_COOKIE usage seems rather hack-ish. drupal_get_token() is just meant to generate a URL-safe token that's used for validation. Manipulating the session ID outside of session_id() seems wrong. Should this logic be in that function instead?

Beanjammin’s picture

Rather than patching core on every release, I think the better approach is to redirect all traffic to https. That's the approach I've taken on all D7 sites that require SSL.

Leeteq’s picture

#147: a mature CMS like Drupal 7+ probably needs to offer a solid mixed-mode handling if it is to retain its leading position now that security is on the rise "everywhere".

grendzy’s picture

RobLoach: I agree that accessing $_COOKIE here isn't ideal, however I believe it's the only solution that doesn't involve a non-backwards-compatible API change in D7.

DanZ: The patches deep-linked from the securepages project page are the ones I recommend.

mpdonadio’s picture

Ditto to what @grendzy said about which patches to use. I haven't run into a case where they need to be re-rolled for applying; the normal patch fuzz factors take into account the line number changes.

sonicthoughts’s picture

Tests seem ok, but this seems more like an issue of approach.
Will this ever get released to core so we can move secure pages to 1.0? Looks like a logjam - another solutions perhaps?

mgifford’s picture

I agree with @DanZ. Acquia's Insight tool recommends that everyone use the Secure Pages module. This really should have been fixed a while back.

mducharme’s picture

FileSize
1.54 KB

Here's a rewrite of 961508-20.patch for 7.22

Status: Needs review » Needs work

The last submitted patch, 961508-153.patch, failed testing.

DanZ’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review

Thanks, mducharme. Let's set it to 7.x-dev long enough to run the auto-test on #153. Set it back to 8.x when done.

DanZ’s picture

#153: 961508-153.patch queued for re-testing.

hass’s picture

There is trailing white space and all tests are missing.

klonos’s picture

Version: 7.x-dev » 8.x-dev

...that's great! Now back to 8.x ;)

mducharme’s picture

Version: 8.x-dev » 7.x-dev
FileSize
1.53 KB

Fixed whitespace issue. @hass: there are no tests in the original patch this is updating either. Feel free to add tests and re-up.

mducharme’s picture

Version: 7.x-dev » 8.x-dev

NOW back to 8.x

David_Rothstein’s picture

Assigned: Unassigned » David_Rothstein

The most recent patch will break non-HTTPS sites, as discussed above. We need to build off the patch in #134 (which also has tests).

I'll try to work on this (hopefully in the next day or so) and see if we can bring it home. It looks like there's only a little feedback left to address?

This is definitely an important issue to fix. However:

Acquia's Insight tool recommends that everyone use the Secure Pages module.

I do not know if the actual recommendation is more nuanced than that, but if it's not, it's incorrect. Secure Pages (if configured carefully and correctly) is more secure than a site running HTTP only, that is true. However, the most secure way to run a site is to force all requests to be over HTTPS, which does not require either the Secure Pages module or the patch in this issue...

David_Rothstein’s picture

Eh, it also occurs to me that if the patch in #159 can break non-HTTPS sites but still passes tests, we're probably missing some important test coverage... I'll see if I can add something for that too.

hass’s picture

This is OT here, but recommending Secure Pages and running a SSL cert is a valid recommendation. This is something that every site should run. Login forms or pages with personal data without SSL are not acceptable in production. d.o should also enable this, too!

David_Rothstein’s picture

Here's a new patch. I believe this covers everything in the last few months of comments (as well as other fixes required by recent changes in Drupal 8). Some responses:

  1. David, thanks very much for your help here. I have been pulling my hair out over this.

    I see that your improved 'token' tests now only call 'drupal_get_token' once and compare it on HTTPS and HTTP submissions, so yes, it will return the same token, that was exactly the problem with previous version of the test. Calling it twice will never return the same token.

    You're welcome! Interesting, I would think call it twice should return the same token (unless the user is logged out and not maintaining a session across requests), but in any case, it seems to work now.

  2. As to why we set ''This form requires HTTPS" in the first place... it was based on feedback from mfb early in the issue. At the moment I'm having trouble stating a specific threat model that form_set_error() protects against. It's more of a sanity check: something has gone wrong if 1. $form['#https'] is set, 2. $conf['https'] is set, and 3. we're not actually on an SSL page.

    Yeah, that makes sense to me; I just don't see how it's related to the rest of this issue. However, if no one else does either I guess that gives some confidence we aren't missing anything important, and fixing two bugs at once is fine by me :)

  3. Direct $_COOKIE usage seems rather hack-ish. drupal_get_token() is just meant to generate a URL-safe token that's used for validation.

    We probably should put the code that reads from $_COOKIE in a helper function instead (there is similar code throughout session.inc that could be consolidated) but since it's duplicated in a couple places already, even before this patch, I think we can leave consolidating it to a followup issue (in the interest of not making this patch bigger and harder to review).

  4. Manipulating the session ID outside of session_id() seems wrong. Should this logic be in that function instead?

    We're not manipulating the session ID itself; the idea is that we need to build the token off of something that isn't the session ID. However, the fact that previous patches used $session_id as the variable name definitely made that unclear... That's fixed in the attached patch.

  5. Eh, it also occurs to me that if the patch in #159 can break non-HTTPS sites but still passes tests, we're probably missing some important test coverage... I'll see if I can add something for that too.

    This actually seems like it might be covered already (at least in Drupal 8). I'm uploading a version that contains the incorrect fix in form.inc to test that it already will fail without having to add any more tests than what we already added.

David_Rothstein’s picture

Those test failures/passees look good to me. I did notice one small issue with the patch:

+    // Test that tokens can be shared in mixed mode.
+    $this->drupalGet('session-test/drupal-token');

This should probably use $this->drupalGet($this->httpUrl('session-test/drupal-token')) instead to be consistent with other code in the tests. (I think the difference matters only if the tests themselves are run over an HTTPS connection.)

Fixed in the attached.

krlucas’s picture

So this patch grants the last unencrypted session token access to all the data of the last encrypted session? So totally Firesheepable. I suppose that backwards compatibility needs to be respected... but the securepages model of "some pages are secure, some are not" is utterly wrong-headed. Sessions, not pages, should be encrypted.

mfb’s picture

The way I would describe it is, with this patch, if mixed-mode sessions are enabled, an eavesdropper can grab a token from a user's HTTP request, and then cause the user's browser to send it to the HTTPS site via cross-site request forgery as an authenticated and token-validated request. Without the patch, the tokens used on the HTTPS site are always different from those used on the HTTP site, so an eavesdropped token cannot be successfully submitted to the HTTPS site. The patch doesn't change anything for HTTPS sites not using mixed-mode sessions.

I'd consider the patch to be problematic from a security perspective, but then again, I don't use mixed-mode sessions at all (for reasons of Firesheep, hostile networks, user privacy, etc.), so I don't really understand what the threat model and security model are supposed to be in mixed-mode land :)

Lukas von Blarer’s picture

@David_Rothstein What do you think about #167?

alexp999’s picture

I don't personally think #167 is an issue. This is just what site admins must accept with mixed mode environments. Perhaps there can be a warning in the settings for the value to enable mixed sessions (if there isn't already).

The main idea with Secure pages is generally to protect the transmission of protected data. i.e. if a customer is submitting card or login details to the server, or if the server is sending address or order details back to the user.

It doesn't really matter in my opinion if a hacker can intercept a HTTP cookie and use it to access the HTTPS site, that isn't really the problem. What users don't want is to be entering private details without seeing the little padlock, secure pages allows us as admins to enable https on pages which might show or ask for visitors private details, thus preventing the interception of said info.

krlucas’s picture

It doesn't really matter in my opinion if a hacker can intercept a HTTP cookie and use it to access the HTTPS site, that isn't really the problem.

@alexp999 which is to say, you as a site admin, don't mind that once a user has entered their username and password with the little padlock, that someone sharing their coffeeshop wifi can then cruise into their account and change their password and update their profile?

krlucas’s picture

I should say that what we do with Drupal 7 (without this patch) is that we allow anonymous users to tool around most of the site with HTTP. But once they try to do anything interesting--log in, add something to their cart, submit a form--they are redirected to the HTTPS site (using .htaccess usually). And we never link them back to HTTP after that. Or if we do (because a content creator maybe entered an absolute link (and there is a module for that)) they lose their session.

hass’s picture

The main feature behind mixed mode is that you may be able to hijack a non-ssl cookie, but you cannot use this session data to get access to SSL secured sections of your site. Normally you don't transfer personal data on http, but when you switch to SSL (on a newsletter subscription page as example) you get a new session and you will enter your data there. Once you leave the subscription page you will be switched back to the non-ssl session and your SSL session is still secure.

This is all important for proxies, tablets, mobile phones and internet café. Someone may be able to jump back in browser history or multistep shopping cart's and gather your personal data in the used forms if you have not invalidated your session. This is all about privacy. It's also recommended to expire the SSL session on browser window close as I know. The SSL session should also calculated based on IP address to prevent session hijacks with other IPs. It's not allowed to grant access to this shopping cart with the non-ssl session id.

alexp999’s picture

The main reason I use HTTPS on sites is to secure the submission of data to stop packet sniffing of user credentials.

I don't profess to know as much as a lot of your guys about Drupal and cookies/https etc, my point was simply that as an admin, the above is what I use secure pages for.

If we can do what hass in #172 has suggested even better. I have been following this thread for some time now and while I agree we don't want to introduce a security hole, I was just putting out there what I as a site admin use it for, so we could perhaps find a good compromise, without trying to do the impossible. :)

krlucas’s picture

@alexp999 sorry I didn't mean to call you out. I'm also following this issue as a past user of securepages, someone who thought it was necessary but have started using a workaround (see my previous comment) that actually seems more secure with no obvious downside.

That said, I'm definitely entering a world of FUD so I don't want to hijack this thread with untested paranoia.

As @hass says it's all about privacy. A shopping cart block typically follows a user all around a site and may reveal all sorts of personal stuff about that user. It's not up to the site admin what should be "really" private. Securepages, with its emphasis on URLs (pages), ignores that.

Lukas von Blarer’s picture

So the data sent through these calls will not be compromised by hijacking a session?

Is there testing needed? If yes, what should be tested?

hass’s picture

The only known 100% secure way is encrypting *all* requests. All other variants may theoretically lead to troubles and hijacking. This is the reason why Google, Facebook and some others enabled SSL by default. So if you have an SSL accelerator in front of your box, it could be seen as a recommendation to enable SSL on all pages, not only a few pages like a mixed mode environment. If you have low budget servers you may not have the horse power to run all URLs via SSL encryption. SSL slow down page loading... all OT here.

alexp999’s picture

I think the only 100% secure way of doing it while still keeping some requests plain HTTP is to use something like the Secure Login module for Drupal. Then Anonymous users browse over HTTP, but all authenticated users browse via HTTPS.

That is obviously not what this issue was started for though, Drupal has the basis for mixed HTTP/HTTPS sessions although it is not working fully.

This was the point I was trying to make earlier, do we just need to forget trying to make it 100% secure, as it is just not going to be possible? Then have an explanation about the risks of dual sessions in the setting file?

alexp999’s picture

Not quite sure why my post came up with all those tags, I didn't set or change any..... :S

Lukas von Blarer’s picture

Lukas von Blarer’s picture

I guess I crossposted earlier...

So basically we could agree that there may be use cases where you want to use mixed mode just to provide a minimal level of security to protect the data being transferred as an alternative to not use SSL at all. There is no reason for Drupal not to support this, right? This does not prevent anyone to enforce SSL globally.

jazzdrive3’s picture

When using Commerce, however, this is still an issue, even if you don't want to go back to a non-secure HTTPS session.

Here is our main use case:

- Anonymous user visits site, begins adding things to cart.
- Anonymous want to begin checkout.
- Checkout sends them to HTTPS, which then loses their cart.
- OR, anonymous user goes to login screen, which sends them to HTTPS, and so they lose their cart.

We never send them back to HTTP once they go to HTTPS. But there must be a way to go to HTTPS for any type of normal, expected Commerce operation.

This patch fixes this issue for us, and is very important.

torgosPizza’s picture

Just to chime in, my use case is the exact same as @jazzdrive3 in #181 above. Anonymous users never hit https (for performance sake) but of course as a Commerce site we require it for handling sensitive customer data.

Lukas von Blarer’s picture

I have the same issue as in #181 as well.

But again my question:

So basically we could agree that there may be use cases where you want to use mixed mode just to provide a minimal level of security to protect the data being transferred as an alternative to not use SSL at all. There is no reason for Drupal not to support this, right? This does not prevent anyone to enforce SSL globally.

mfb’s picture

The use case of data from the anonymous insecure session being migrated to the secure session actually should already work at present. So I'm not sure why it doesn't work w/ Commerce.

There is logic in _drupal_session_read() which, on the HTTPS site, if no secure session is found, checks the sessions table for an anonymous session with the non-HTTPS-only cookie. Any session data found there (e.g. shopping cart) is read into the session - and will be included in the new secure session which is saved.

If you put this in your settings.php and visit the HTTP site followed by the HTTPS site, you should see that the secure session ends up with both the data from the insecure session and the secure session (you have to look in the sessions db table to see the session data):

if (isset($_SERVER['HTTPS']) && strtolower($_SERVER['HTTPS']) == 'on') {
  $_SESSION['secure'] = 'foo';
}
else {
  $_SESSION['insecure'] = 'bar';
}
jhrizz’s picture

To add to the chorus- Our use case is the same as @jazzdrive3 in #181 and @torgosPizza in #182- and we agree with #183- tens of thousands of nodes that can (and should) be browsed without SSL, then when it comes to putting money down or viewing their personal profile, secure the session from then on.

This may be a web commerce specific need, but if one looks at eBay, Amazon, etc, all sessions are mixed mode- even authenticated users are only SSL when paying for something or viewing their account profiles, then switches back to non-SSL (while maintaining the session) when browsing and "shopping". This switch is done quickly and seamlessly.

Granted, they have more resources than many Drupal site admins, but the concept is there and working on the largest commerce sites in the world. Perhaps this is a counter point to the social networks that make all authenticated session SSL.

If I were king for a day, I would want D7 / D8 to support "either, both or neither" and then support each use case with a best practices guide and allow admins a choice and the tools to do it right.

Will continue to do the hacks / fixes for now.

Cheers!

grendzy’s picture

krlucas: I think your analysis is overlooking the fact that mixed mode is intended to be combined with forced HTTPS redirects to "important" pages (where important is defined by the site builder). An attacker can hijack the insecure half of the mixed session, but not the secure half.

repeating my comment from #11:

As I see it using mixed-mode in the first place means being willing to accept some compromises and also taking responsibility for configuring your modules wisely. I do share your general skepticism about mixed-mode: with modern CPUs, it's a compromise that's becoming less necessary, and with SSL-stripping one that's less effective. Nonetheless this is a use-case that has been possible in the past and still has a place in the ecosystem.

sonicthoughts’s picture

We are using civicrm 4.33 which shares drupal 7.22 sessions.
Issue: Login with drupal HTTP works for civicrm HTTP over port 80. Civicrm switches to HTTPS and authentication breaks.
Is there a proposed fix/patch for 7.22?

jaredsmith’s picture

This issue could use an updated issue summary, based on the templates at http://drupal.org/issue-summaries.

marioangulo’s picture

synth3tk’s picture

#23: 961508-23.patch queued for re-testing.

rbrownell’s picture

heddn’s picture

cleaning up tags

rbrownell’s picture

According to the Secure Pages Project Page this is still an issue in Drupal 7... How do we flag this issue for D7 development?

This issue has been a problem for a long time with D7 and it is preventing useful modules like Secure Pages from producing a stable release.

EDIT: Did not intend to add the 'stable release blocker' tag... It somehow was automatically added back when I posted this reply...

star-szr’s picture

@rbrownell - The 'needs backport to D7' tag means that as soon as this is fixed in 8.x, this issue will go back to 7.x for a backport (to fix the issue in D7 as well).

So the best way to get this into D7 is to help with fixing it in D8 first :)

rbrownell’s picture

Thanks for the reply Cottser

According to the history of this, this was originally a problem with D7 that seemed to become overlooked when it was upgraded to be worked on in Drupal 8.

It seems that Drupal 7's development for contributed modules is becoming fragmented because of the frequency of which Drupal releases come out and the amount of work it takes to upgrade modules and sites... It almost reminds me a bit of the problem Google is experiencing with the Android ecosystem. Don't get me wrong though. I love Drupal and everything it stands for... It would just be nice to have more time between releases so that developers create their modules as opposed to falling into the "I'll work on it for the next version only" mindset because frankly Drupal 6 still looks better than Drupal 7 for contributed modules and harmony in the ecosystem.

Drupal 7 is 'advertised' as being eCommerce ready and the best version of drupal thusfar but issues that prevent one of the most popular security modules from releasing a full stable version makes it exactly the opposite.

Anyway... Enough of my rant... How do we fix this?

There appears to be fully functional patches for D7 that aren't being implemented into core because of the 8.x-dev version selection for this issue. How can we get these patches in D7's core? Or is it because D7 and D8 are very similar in terms of coding? Is the testing of such patches for D7 failing because of the issue being marked as D8?

star-szr’s picture

Because of the backport policy. We fix bugs in Drupal 8 first so that we don't have any regressions.

rbrownell’s picture

@Cottser. Thank you for pointing that out. Not sure I agree with the policy 100% but things make more sense now.

EDIT: Reading this there is an exception for Security patches... Though this is not a 'patched worked on in private' one wouldn't this still fall into that category because it is dealing with https?

catch’s picture

@rbrownell no that only applies to reported security vulnerabilities which are worked on in a separate issue tracker by a limited set of people.

You asked how to fix it - there's a patch in #165 that needs review.

Anonymous’s picture

If I understand #177 correctly, does this mean I could adapt my workflows to use the Secure Login module and just drop support for securepages? Skip this entire thread and the others like it? If so, thanks @alexp999 for the recommendation, I'm going to test this and see if it will work for us.

I have 5 years of Drupal experience without patching core in production, I don't want to start now.

Serious sad face that this issue is not closed yet.

Anonymous’s picture

Re: #197 although I also generally support the backport policy, I definitely buy in to the argument that this is a security issue. If people don't apply their patches correctly... trouble.

sonicthoughts’s picture

+1 for #197 Don't let the perfect get in the way of the good ...

alexp999’s picture

@Ryan Weal, this is what we have done, just stopped using secure pages and use secure login instead.

Much more secure, means all Anon users are http, all authenticated users over https.

No worry about security between sessions as if a user tries to visit a non http page then they just appear logged out. As soon as they try and do anything meaningful they are redirected to https and find themselves logged in again.

It may not work for everyone's situation but it is ideal for our e-commerce site where we only require users to login when they actually want to complete checkout.

sonicthoughts’s picture

@alexp999 does not work for us. We have webforms and registration pages that need SSL for anon.

Note securepages has over 100,000 downloads and 21,000 active users - most on D7. It's one of the most popular modules. I reiterate #197 that requiring this patch because of the backport policy is not very pragmatic. If you look at the number of https / http issues in google there are many issues that secure pages could solve if properly supported.

Anonymous’s picture

Installed Secure Login... what a dream!

@sonicthoughts have you tried using Secure Login? It is actually more of a "form encryption" thing than a "login" module. I think it would work in your case. You install the module, then by default it redirects any page with the login form (and some other forms) to the https version of the same page. You can add additional form IDs to the configuration page, so it will then target all of your other forms.

The only case where I could see Secure Login not working is if you have anon users adding to the cart... HOWEVER, you can simply put the "add to cart" form name in the list. Most add-to-cart forms I've seen append an ID to the end, so there may need to be a patch if the module doesn't support wildcards (I haven't tried that yet - my workflows are all directed to logged-in users). Maybe it already works?

Edit: that would already work per #184, thanks @mfb!

mfb’s picture

As I mentioned in #184, anonymous users adding items to their cart in HTTP and then switching to HTTPS actually should work. That's because _drupal_session_read() has logic to look for an insecure session if no secure session is found. It's possible that this might be broken somehow, but it's intended to work...

sonicthoughts’s picture

Yes, but we ALSO want many admin pages which do not necessarily have a form to go to SSL. I do see it as a compromise and appreciate the suggestion (may go that route temporarily) but I just think it is yet another band-aid.

Do a search in google or drupal.org on these related topics and there are endless issues that could be solved if securepages were released and not require a dev version and patches. I'm just saying ...

Anonymous’s picture

@sonicthoughts I think you should give it a try. By virtue of the login form being SSL it means that all your admin pages are too because that's the session you're using. Good luck. I consider myself done with this issue. :)

mgifford’s picture

The last submitted patch, drupal_https_sessions-961508-165.patch, failed testing.

Perignon’s picture

I second that. This problem is a security issues - black-eye on D7 - a backport from D8 shouldn't be required. I know we, as a community, are trying to push D8 development but policies like this harms Drupal as a whole - they don't help! D7 is (by Commerce Guys work) been pushed as the premier eCommerce solution and it works really well but without the ability properly handle SSL - it's absolutely useless. I am very shocked that over a year later this has not been fixed in D7.

Can we have an exception to policy and get this fixed NOW in D7 that is being used on production websites. This is harming people that are trying to conduct business (eCommerce) with the most usable version of Drupal which is Drupal 7. Please, let us use a little common sense here. Support the thousands of websites our there conducting business with Drupal 7 then port to D8.

heddn’s picture

Status: Needs work » Needs review
FileSize
6.07 KB
4.32 KB

So we can get this back-ported, here's a re-roll of #165. Tests ran fine locally. Let's see how we fair with the testbot. No interdiff since this is simply a re-roll.

heddn’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php
@@ -149,13 +149,23 @@ protected function testHttpsSession() {
+    $this->assertText(t('This form must be submitted over a secure connection.'), 'Form submission failed over HTTP.');

Hmm, tests didn't fail. Probably need to throw in $this->settingsSet('mixed_mode_sessions', TRUE); somewhere prior to this call. I'll see if I have time later today.

+++ b/core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php
@@ -211,6 +221,20 @@ protected function testHttpsSession() {
+    $this->assertEqual($token_plain, $token_secure, 'Tokens are shared in mixed HTTPS sessions.');

I'll have to dig into why this didn't fail. It isn't obvious.

Perignon’s picture

@Ryan Weal

Thanks for the suggestion about Secure Login.

heddn’s picture

Here's another attempt. Cleaned up the tests a little and altered the logic inside CsrfTokenGenerator->get()

heddn’s picture

Status: Needs review » Postponed
heddn’s picture

Status: Postponed » Needs review
FileSize
6.54 KB
5.37 KB

#2122761: SessionHttpsTest only runs half the tests seemed to fair well. While not committed yet, I'd like to see how tests run here.

The last submitted patch, drupal_https_sessions-961508-215.patch, failed testing.

mantisae’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 71: drupal-https-only-961508-23-32.patch, failed testing.

sonicthoughts’s picture

219 comments later ... and still not working for D7 :(

klonos’s picture

mgifford’s picture

Would love if we had a better D7 solution in Core, particularly with yesterday's security update.

heddn’s picture

Agreed. What's holding this up right now is D8's SimpleTests were all messed up. I haven't had time to go through and fix all the broken tests. But once we can fix those, this should be able to slip into D8 nicely and the backport for D7 is fairly simple.

nerdcore’s picture

Version: 8.x-dev » 7.24
FileSize
1.25 KB

This is a reroll of the patch in #159 which applies on Drupal 7.24.

nerdcore’s picture

Version: 7.24 » 8.x-dev

Setting Issue back to 8.x-dev

sonicthoughts’s picture

Hi - sorry but will #224 be committed or does it have to wait for 8.x to be marked fixed? There are a lot of dependencies on this single issue.... thanks for the hard work.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
10.18 KB
5.45 KB

Here is a reroll, but also we need to make some changes. The CSRF token generator doesn't set the request anymore. We were having issues that the generator was being used outside the request scope, so this may or may not work :/ We could get away with this easier before as get() did not need the request, then we could use the current_user service. We will see...

Left an @todo in the CsrfTokenGeneratorTest, as we need to add more assertions to check any new logic.

The last submitted patch, 227: 961508-227.patch, failed testing.

sonicthoughts’s picture

Hi @nerdcore - why are you setting back to 8.dev if this is might be contributing to major security issues can we create another issue for D7? There is understandable reluctance to apply patches to production systems (especially one's that are so pervasive as this.) Furthermore patches often get lost in upgrades ... the bottom line is that over 25,000 sites are using https://drupal.org/project/securepages and this may affect many more D7 sites. I understand the backport policy that @Cottser referenced, but for security issues this could clearly be seen as an exception.

David_Rothstein’s picture

This is not a security issue. If you are aware of a security issue, you should report it privately to the security team (https://drupal.org/node/101494) and in that case it would be fixed in Drupal 7 first.

In fact, this issue may be the opposite since #167 raised the concern that the current approach in the patch could actually weaken security. I think that has been adequately addressed by the subsequent comments (basically, the suggestion is that (a) in the case of a form on an HTTP page submitting to an HTTPS URL, it's as secure as it can be, and (b) it won't hurt security in other cases?) but I'm not sure.

FrancescoUK’s picture

Not sure the patch in #224 is totally correct because in common.inc it should probably use $session_id and not session_id() when calling drupal_hmac_base64 on the return line:

+  // For mixed HTTP(S) sessions, use a constant identifier so that tokens can be shared between protocols.
+  if (variable_get('https', FALSE) && $GLOBALS['is_https'] && isset($_COOKIE[substr(session_name(), 1)])) {
+    $session_id = $_COOKIE[substr(session_name(), 1)];
+  }
+  else {
+    $session_id = session_id();
+  }
   return drupal_hmac_base64($value, session_id() . drupal_get_private_key() . drupal_get_hash_salt());
raphaelhuefner’s picture

FileSize
1.35 KB

This is a reroll of the patch in #159 which applies on Drupal 7.25.

I agree with #231 that the patch in #224 was missing a line. Why set $session_id when you won't use it after?

Status: Needs review » Needs work

The last submitted patch, 232: 961508-232.patch, failed testing.

mikeytown2’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review

Moving issue to 7.x so patch will get tested.

mikeytown2’s picture

232: 961508-232.patch queued for re-testing.

mikeytown2’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

#232 passes. Moving issue back to 8.x

star-szr’s picture

Issue tags: +Needs reroll

Tagging for reroll.

Salah Messaoud’s picture

Assigned: Unassigned » Salah Messaoud
Issue tags: +TUNIS_2014_JANUARY, +#SprintWeekend2014

Started work

Salah Messaoud’s picture

227: 961508-227.patch queued for re-testing.

The last submitted patch, 227: 961508-227.patch, failed testing.

Salah Messaoud’s picture

FileSize
10.18 KB

I tried patch to reroll patch from #227 it doenst apply for HEAD, I applied it to the commit e75ac46431444c5feacbd744ce38ef350049b468 it worked but after trying to merge with 8.x I didnt get any conflicts, the patch attached apply cleanly

Salah Messaoud’s picture

Status: Needs work » Needs review
Salah Messaoud’s picture

Assigned: Salah Messaoud » Unassigned
rickmanelius’s picture

Nice work Salah! And if we get [#4196144] tackled as well, we can finally get a stable release of secure pages! https://drupal.org/project/securepages

Status: Needs review » Needs work

The last submitted patch, 241: 961508.patch, failed testing.

star-szr’s picture

+++ b/core/core.services.yml
@@ -427,9 +427,10 @@ services:
+      - [setRequest, ['@?request']

I think this line needs another ], based on the following error seen after applying the patch and running `drush cr`:

Fatal error: Uncaught exception 'Symfony\Component\Yaml\Exception\ParseException' with message 'Malformed inline YAML string [setRequest, ['@?request'] at l
ine 434 (near "- [setRequest, ['@?request']")' in /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Inline.php:312
Stack trace:
#0 /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Inline.php(59): Symfony\Component\Yaml\Inline::parseSequence('[setRequest, ['..
.', 26)
#1 /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php(409): Symfony\Component\Yaml\Inline::parse('[setRequest, ['...', fal
se, false)
#2 /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php(114): Symfony\Component\Yaml\Parser->parseValue('[setRequest, ['...'
, false, false)
#3 /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php(186): Symfony\Component\Yaml\Parser->parse('- [setCurrentUs...', fal
se, false)
#4 /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfo in /Users/Scott/Sites/d8.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Inline.php on line 
312

Fatal error: Call to a member function get() on a non-object in /Users/Scott/Sites/d8.dev/core/lib/Drupal.php on line 530

…and searching through core.services.yml to see how other setRequest items were formatted.

Paul Rowell’s picture

The last submitted patch, 71: drupal-https-only-961508-23-32.patch, failed testing.

mgifford’s picture

I got this error here when applying that patch /core/install.php?langcode=en&profile=standard

Symfony\Component\Yaml\Exception\ParseException: Malformed inline YAML string [setRequest, ['@?request'] at line 446 (near "- [setRequest, ['@?request']") in Symfony\Component\Yaml\Inline::parseSequence() (line 312 of core/vendor/symfony/yaml/Symfony/Component/Yaml/Inline.php).

star-szr’s picture

Issue tags: -Needs reroll +Novice

Adding Novice tag to make the small update mentioned in #246 and provide an interdiff.

mgifford’s picture

Status: Needs work » Needs review
FileSize
10.18 KB

I've been annoyed by this issue for too long to wait.

$ grep -ir setRequest core/core.services.yml

Now the setRequests are almost consistent:

core/core.services.yml: - [setRequest, ['@?request']]
core/core.services.yml: - [setRequest, ['@request']]
core/core.services.yml: - [setRequest, ['@?request']]
core/core.services.yml: - [setRequest, ['@?request']]
core/core.services.yml: - [setRequest, ['@?request']]
core/core.services.yml: - [setRequest, ['@?request']]

Not sure if the 2nd one should include the "?" but it's at least fixed in this issue.

mgifford’s picture

FileSize
993 bytes

forgot the interdiff.

Status: Needs review » Needs work

The last submitted patch, 252: interdiff-961508_1-251.patch, failed testing.

The last submitted patch, 251: https-session-cookies-961508_251.patch, failed testing.

amacias’s picture

The last submitted patch, 71: drupal-https-only-961508-23-32.patch, failed testing.

mgifford’s picture

The last submitted patch, 251: https-session-cookies-961508_251.patch, failed testing.

The last submitted patch, 251: https-session-cookies-961508_251.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
10.24 KB

This is just a re-roll. The patch broke couldn't apply for some reason.

Status: Needs review » Needs work

The last submitted patch, 260: https-session-cookies-961508_260.patch, failed testing.

webservant316’s picture

Which is the preferred patch for Drupal 7.26 for the proper operation of securepages?

MrPeanut’s picture

@webservant316 — The patch from #232.

webservant316’s picture

Wow - surprising that Drupal 7 is not functioning smoothly with an HTTPS module for mixed mode sessions. I've been fighting with this for a day! How can others even be using this on a commerce site? I see that the Drupal site itself appears to always be on HTTPS. Maybe that is the solution, do not use mixed sessions?

I followed the directions at https://drupal.org/project/securepages. Since I have Drupal 7.26 installed I could not use #13, https://drupal.org/comment/4196144#comment-4196144, so I used #100 instead, https://drupal.org/comment/8241501#comment-8241501. I also could not use #71, https://drupal.org/comment/6391460#comment-6391460, so I used #232, https://drupal.org/comment/8361131#comment-8361131. After setting $conf['https'] = TRUE; in settings.php is was not better off, but continued to permanently lose my session on HTTPS pages. Then I tried https://drupal.org/project/securelogin but with further problems.

Current I have a configuration with securepages that only loses the session when on HTTPS pages, but regains it when on HTTP pages. Fortunately, I can limp along with this for now.

Any idea when this will get straightened out?

webservant316’s picture

ok this got secure pages working for me - https://drupal.org/comment/8524769#comment-8524769

martin107’s picture

Identified a single problem :-

Removed confusion between drupalPost() and drupalPostForm()

martin107’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 266: https-session-cookies-961508_266.patch, failed testing.

martin107’s picture

Ok .. I can't quite see what is wrong, so I will provide notes and ask for help

Looking at SessionHttpsTests ... just to abstract what is going on I will repeat the comments detailing the steps taken above the two failing tests

205 //Clear Browser cookie jar
..
208 // Start an anonymous session on secure site
..
211 // Mock login to secure site using secure session cookie.
..
217 // Test that the user is also authenticated on the insecure site.
..

221 // Test that tokens can be shared in mixed mode. .
Looking at the verbose from browser testing ( /admin/config/development/testing/results/2 )
222 drupalGet() returns "Valid HTML found on "http://dev.drupal.co.uk/core/modules/system/tests/http.php/session-test/..."
its contains the standard Access denied | Drupal html page.

I think this test get access denied because the of the brand new entry in sytem_test.routing.yml

session_test.shared_token:
  path: '/session-test/shared-token'
  defaults:
    _title: 'Test that tokens can be shared in mixed mode'
    _controller: '\Drupal\session_test\Controller\SessionTestController::sharedToken'
  requirements:
    _permission: 'access content'

_permission: 'access content' I don't think that has been properly mocked.

Please tell me I am wrong ... or steer me in the direction of another place in testing that mocks 'access content' correctly.

sanduhrs’s picture

The last submitted patch, 266: https-session-cookies-961508_266.patch, failed testing.

znerol’s picture

I'd prefer when CsrfTokenGenerator would get decoupled from session_id() completely. Instead I suggest to add a method (e.g. getTokenSeed) to AccountInterface. The token seed would be generated the first time it is used (just like the private key) and then stored in the session. Note that when using a generated token seed, it wouldn't be necessary to concatenate that with the private key anymore, because the risk of disclosing the session id via tokens wouldn't exist any longer.

Optionally it would be possible to only generate and store the token seed when actually using mixed mode SSL and otherwise just continue to use the original method.

martin107’s picture

patch no longer applied - reroll

(No conflicts during rebase just auto-merging)

martin107’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 273: https-session-cookies-961508_273.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
9.5 KB
2.7 KB

call to access 'mixed_mode_sessions' has change to be

 Settings::get('mixed_mode_sessions', FALSE)

I have stripped out the now redundant $settings property from CsrfTokenGenerator.

Lets see what testbot has to say..

Status: Needs review » Needs work

The last submitted patch, 276: https-session-cookies-961508_276.patch, failed testing.

cosmicdreams’s picture

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -829,6 +829,11 @@ public function validateForm($form_id, &$form, &$form_state) {
+      \Drupal::formBuilder()->setErrorByName('', $form_state, t('This form must be submitted over a secure connection.'));

Aren't you adding this to the FormBuilder object? Why not create a new instance of this object? Or use $this?

martin107’s picture

Status: Needs work » Needs review
FileSize
9.49 KB

Patch no longer applied .. no conflicts in reroll just 3-way merging

I will address #278 ...next

Status: Needs review » Needs work

The last submitted patch, 279: https-session-cookies-961508_279.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
9.47 KB
0 bytes

addressed #278. I can see other example of \Drupal::request() in this file ... in comments -- but the comments don't make that much sense to me..
That is they don't connect to the code below it

So I have left them alone for now...

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
    @@ -53,6 +62,16 @@ public function setCurrentUser(AccountInterface $current_user = NULL) {
    +   * Sets the current request.
    +   *
    +   * @param \Symfony\Component\HttpFoundation\Request $request
    +   *   The current request.
    +   */
    +  public function setRequest(Request $request) {
    +    $this->request = $request;
    +  }
    +
    

    Instead of injection the request like that we should better use the request stack already.

  2. +++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
    @@ -72,7 +91,22 @@ public function setCurrentUser(AccountInterface $current_user = NULL) {
    +    if ($this->request->isSecure() && Settings::get('mixed_mode_sessions', FALSE)) {
    

    Can't we also use the settings service instead of this singleton call? this makes it potentially easier to test in the future.

martin107’s picture

FileSize
895 bytes

reposting interdiff

Status: Needs review » Needs work

The last submitted patch, 281: https-session-cookies-961508_281.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
4.21 KB
9.74 KB

Addressed issues from #282.

Status: Needs review » Needs work

The last submitted patch, 285: https-session-cookies-961508_285.patch, failed testing.

martin107’s picture

Ah so in browser testing "no test results to display" means patch uninstallable.....good to know

Simplefix

-use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\RequestStack;
martin107’s picture

Status: Needs work » Needs review
znerol’s picture

Status: Needs review » Postponed

I set this to postponed in an attempt to redirect the attention to the underlying problem: #2245003: Use a random seed instead of the session_id for CSRF token generation.

Status: Postponed » Needs work

The last submitted patch, 287: https-session-cookies-961508_287.patch, failed testing.

znerol’s picture

Status: Needs work » Postponed
YesCT’s picture

Issue summary: View changes
Issue tags: -Novice

removing the novice tag (that was added in #250)
if there are other small novice tasks that could be done here, please update the issue summary to make it clear and add the tag back. :)

also, editing the issue this is postponed on to make it critical (since this is critical)

catch’s picture

Status: Postponed » Closed (duplicate)

Just committed #2245003: Use a random seed instead of the session_id for CSRF token generation to 8.x and marked it for backport to 8.x. So this is a duplicate now.

webservant316’s picture

I am currently upgrading from Drupal 7.26 to 7.28 and checking back here to see if this issue is solved. I installed a patch above in order to use https://drupal.org/project/securepages. Securepages still advise...

Steps to install Secure Pages 7.x-dev
Set $conf['https'] = TRUE; in settings.php.
Apply these two patches:
#961508-71: Dual http/https session cookies interfere with drupal_valid_token()
#471970-13: DrupalWebTestCase->getAbsoluteUrl should use internal browser's base URL
Despite these issues still exist in Drupal 7, and are unlikely to be resolved, we are now working towards a stable release of Secure Pages.

How could it be that this issue would not be resolved in Drupal 7? So everyone that wants to have a mixed http/https session in Drupal 7 needs to patch core? That is hard to believe. I am missing something? Perhaps it would be better to ditch securepages and run https all the time rather than maintain a core patch in my configuration.

krlucas’s picture

Perhaps it
would be better to ditch securepages and run https all the time rather than
maintain a core patch in my configuration.

Yes. You are exactly right.

webservant316’s picture

Ok I reapplied this core patch to Drupal 7.28 and Securepages. All seems well. I needed to maintain mixed http/https sessions because I need to iframe a few http and https pages. Thus the parent page needs to be http and https correspondingly. How would a non PHP person even use Drupal 7?

Island Usurper’s picture

I put a patch in #2245003: Use a random seed instead of the session_id for CSRF token generation to backport it to D7. Go review it!

mikemccaffrey’s picture

Reapplied the patch from #232 to my site after the Drupal 7.30 security release, and everything seems to work fine.

The last submitted patch, 71: drupal-https-only-961508-23-32.patch, failed testing.

realityloop queued 232: 961508-232.patch for re-testing.

The last submitted patch, 232: 961508-232.patch, failed testing.

mgifford’s picture

@krlucas - on "run https all the time" - that is still a bit of a problem with Varnish. I still don't see a clear way around https://www.varnish-cache.org/docs/trunk/phk/ssl.html

hanoii’s picture

@mgifford you could use an ssl terminator in front of varnish, like haproxy, nginx, stunnel, or pound:

https://www.adammalone.net/post/why-pound-awesome-front-varnish

I am currently still running a mixed http/https session with nginx cache (which works pretty much similar to varnish). At some point I tried HTTPS only, but performance was really noticeable on high load when run on https. I am running in a 2gb digitalocean droplet, and the load difference was enormous with and without.

webservant316’s picture

I wish I understood the real hang up here so that I could help enable mixed mode sessions for Drupal. Seems like a problem this big ought to be getting some action toward a solution. I really don't like running a patched core and I am sure no one else does either.

webservant316’s picture

I was using this patch in order to use securepages with mixed mode sessions. However, having no desire to maintain a core patch and since this issues does not appear to be moving forward I abandoned the effort and have moved to constant HTTPS for my entire site. If this patch ever gets added to core I will be glad to use it. The main hassle for me is that I was iframing a 3rd party HTTP and HTTPS pages on my site, so I needed to specify HTTP and HTTPS for those pages so that browsers would not complain.

mausolos’s picture

So what's the status on this? Which patch do we use, the one at the top, or one of the failed patches?

manoj.surala’s picture

Hi all,

I have some kind of issue like this. I'm getting this form outdated error throughout my site.
I was unable to know why. So I tried installing a fresh drupal installation in which I've not used any other modules. Even then also I'm getting this form outdated error.
Could you please suggest me what all should I look after to get rid of this error.

I've googled and found some solution which didn't seem pretty good.
For example some are suggesting to set #token value to Null/False. I think that will breach the security of the site.
So could someone suggest me the best way to test what exactly is going wrong and get this error removed.

Thanks in advance.

YesCT’s picture

Issue summary: View changes

This issue was closed duplicate of #2245003: Use a random seed instead of the session_id for CSRF token generation by @catch in #293.

adding the issue this was closed duplicate of to the issue summary, since this is getting lost (issue is two pages) and confusing new commenters.

alimac’s picture

Issue tags: -#SprintWeekend2014 +SprintWeekend2014

Minor tag cleanup - please ignore.

The last submitted patch, 71: drupal-https-only-961508-23-32.patch, failed testing.

The last submitted patch, 71: drupal-https-only-961508-23-32.patch, failed testing.

denix queued 232: 961508-232.patch for re-testing.