| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Contributed project blocker, D7 stable release blocker, Issue summary initiative, needs backport to D7, 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.