| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | Contributed project blocker, D7 stable release blocker, needs backport to D7, Needs issue summary update, needs security review, Needs tests, SecurePages, session |
Issue Summary
This is a follow-up to #1577: Mapping privilege separation on non-SSL/SSL.
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.
Comments
#1
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.
#2
subscribe just for fun (I'm not a fan of $conf['https'] but curious to follow the #action)
#3
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.
#4
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.
#5
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.
#6
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)
#7
#8
subscribing
#9
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
#10
@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.
#11
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.
#12
AFAICS, Drupal never supported this so far, so this is a good and possibly backportable improvement, but not a bug.
#13
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.
#14
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. ;)
#15
Tagging issues not yet using summary template.
#16
#17
Subscribing
#18
I think this tag is appropriate.
#19
#9: 961508.patch queued for re-testing.
#20
The last submitted patch, 961508.patch, failed testing.
#21
Needs a reroll?
#22
#23
Rerolled for core/.
#24
In an effort to move these along by providing feedback, here is a quick overview.
#25
#26
Thanks for fixing the shoddy work I did with the HTTP/HTTPS mechanism. However, this needs a test. Thanks again.
#27
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.
#28
Adding session tag.
#29
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?
#30
For what it's worth, #3 (for D7) and #23 (for D8) are almost identical and #23 applies fine on D7.10 as well.
#31
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.
#32
Patch from #23 re-rolled against 7.x for drush make file.
#33
Still needs a test I think. Thanks!
#34
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.
#35
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.
#37
@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?
#38
Oh, I didn't realize httpsUrl() was in a different class. You might have to move those two functions to DrupalWebTestCase.
#39
Does anyone disagree with @grendzy's recommendation to move httpUrl() and httpsUrl() to DrupalWebTestCase?
#40
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?
#41
I'm not sure about 1. For 2, it looks like session.test alters the action this way:
<?php$this->drupalGet('user');
$form = $this->xpath('//form[@id="user-login"]');
$form[0]['action'] = $this->httpsUrl('user');
?>
#42
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.
#43
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.#44
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
#45
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.
#46
Thank you @grendzy - That was my problem then (I was applying the patch to D7 D8)
Thank you...
#47
Tagging as a SecurePages release blocker.
#48
Something went wrong with the other tags it seems. Re-adding them...
#49
#21: 961508-20.patch queued for re-testing.
#50
The last submitted patch, 961508-23-32-D7.patch, failed testing.
#51
@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?
#52
@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.
#53
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.
#54
#32: 961508-23-32-D7.patch queued for re-testing.
#55
The last submitted patch, 961508-23-32-D7.patch, failed testing.
#56
Hey Robin (#54). It seems we're waiting on Everett in #52 because he's already made progress on the testing portion.
#57
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.
#58
@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.
#59
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?
#60
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.
#61
Bumping this to critical. Without SSL switching, how can you build an eCommerce site.
#62
+1 for critical, 7.16 release blocker
#63
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.
#64
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.
#65
@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.
#66
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.
#67
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...
#68
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:
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
#69
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.
#70
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.
#71
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)?
#72
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.
#73
The last submitted patch, drupal-https-only-961508-23-32.patch, failed testing.
#74
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
#75
@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:
#79
#75: 961508-75-https-dual-tokens.patch queued for re-testing.
#80
The last submitted patch, 961508-75-https-dual-tokens.patch, failed testing.
#81
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?
#82
This patch fixes the paths to the https.php/http.php files in WebTestBase.php, but still has problems submitting the form.
#83
Lets test it anyway.
#84
The last submitted patch, drupal-n961508-82-d8.patch, failed testing.
#85
After help from chx on IRC I was able to fix the "Form HTTPS only" test, but the others need work.
#86
The last submitted patch, drupal-n961508-85-d8.patch, failed testing.
#87
#23: 961508-23.patch queued for re-testing.
#88
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.
#89
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.
#90
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.
#91
The last submitted patch, drupal-n961508-90-d8.patch, failed testing.
#92
Nixed an extra }
#93
The last submitted patch, drupal-n961508-92-d8.patch, failed testing.
#94
Subscribing, it is hard to know which patch to use.
#95
Taking this over for a little while. Re-rolling against latest head and cleaning some doc blocks.
#96
And here is new patch for test bot.
#97
The last submitted patch, drupal-https_session_cookies-961508-96.patch, failed testing.
#98
Missed some forward slashes. Try again.
#99
The last submitted patch, drupal-https_session_cookies-961508-98.patch, failed testing.
#100
Working on fixing tests. Hope to post new patch tomorrow.
#101
OK think I have fixed some of the tests. But will probably still fail some. Will continue working on this tomorrow.
#102
The last submitted patch, drupal-https_session_cookies-961508_101.patch, failed testing.
#103
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.
#104
ooppps and send to bot.
#105
The last submitted patch, drupal-https_session_cookies-961508-103.patch, failed testing.
#106
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.
#107
The last submitted patch, drupal-https_session_cookies-961508-106.patch, failed testing.
#108
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.
#109
OK, let me get it green anyway. Just one more test. Just in case.
#110
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.
#111
Also this work may be of use for backport to D7
#112
Yes, it would be great for D7 (and D8 if Symfony session handling does not get in).
#113
Yes, regardless, this needs a backport to Drupal 7, regardless of what ends up happening with 8.
#114
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.
#115
#71 added test coverage to the D7 patch. It still needs to be cleaned up but it's almost there:
#116
#21: 961508-20.patch queued for re-testing.
#117
This should not be blocked for D7 just because we don't know where D8 goes.
#118
+1
#119
Un-assigning myself, as my work here is done for now.
#120
I reviewed the patch and read through the entire issue. Comments:
$conf['https'] = TRUEin 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.+ // 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.
+ $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).#121
#110: drupal-https_session_cookies-961508-110.patch queued for re-testing.
#122
The last submitted patch, drupal-https_session_cookies-961508-110.patch, failed testing.
#123
Working on fixing the issues from #120. Thanks for the review @David_Rothstein.
#124
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', FALSEas 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_tokeneach time, and then testing to see that each time it sent the same token. However, it could not, as the function containsdrupal_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.
#125
ooppps here bot.
#126
The last submitted patch, drupal-https_session-961508-124.patch, failed testing.
#127
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.
#128
The last submitted patch, drupal-https_session-961508-127.patch, failed testing.
#129
OK lets try that again.
#130
The last submitted patch, drupal_https_sessions-961508-129.patch, failed testing.
#131
Ok missed a ';', lets try that again.
#132
The last submitted patch, drupal_https_sessions-961508-129.patch, failed testing.
#133
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).
#134
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...
#135
#136
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.
#137
sounds like #136 Identified some things that need to be addressed, so marking needs work.
#138
It sounds more like needs review.
#139
#134: drupal_https_sessions-961508-134.patch queued for re-testing.
#140
David_Rothstein:
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".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).
#141
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.
#142
The last submitted patch, 961508-141.patch, failed testing.
#143
#134: drupal_https_sessions-961508-134.patch queued for re-testing.
#144
#21: 961508-20.patch queued for re-testing.
#145
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....
#146
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 ofsession_id()seems wrong. Should this logic be in that function instead?#147
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.
#148
#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".
#149
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.
#150
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.
#151
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?
#152
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.
#153
Here's a rewrite of 961508-20.patch for 7.22
#154
The last submitted patch, 961508-153.patch, failed testing.
#155
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.
#156
#153: 961508-153.patch queued for re-testing.
#157
There is trailing white space and all tests are missing.
#158
...that's great! Now back to 8.x ;)
#159
Fixed whitespace issue. @hass: there are no tests in the original patch this is updating either. Feel free to add tests and re-up.
#160
NOW back to 8.x
#161
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:
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...
#162
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.
#163
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!
#164
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:
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.
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 :)
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).
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.
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.
#165
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.
#166
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.
#167
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 :)
#168
@David_Rothstein What do you think about #167?
#169
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.
#170
@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?
#171
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.
#172
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.
#173
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. :)
#174
@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.
#175
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?
#176
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.
#177
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?
#178
Not quite sure why my post came up with all those tags, I didn't set or change any..... :S
#179
Restoring the tags...
#180
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.
#181
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.
#182
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.
#183
I have the same issue as in #181 as well.
But again my question:
#184
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):
<?phpif (isset($_SERVER['HTTPS']) && strtolower($_SERVER['HTTPS']) == 'on') {
$_SESSION['secure'] = 'foo';
}
else {
$_SESSION['insecure'] = 'bar';
}
?>
#185
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!
#186
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:
#187
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?