In Browser and FunctionalJavascript tests SIMPLETEST_USER_AGENT cookie needs to be set every 5 seconds

I had some issues with some CURL timeouts before which has been 30 seconds on my local system for some reason. I never had the time to actually debug it, so I'm not sure what could have caused that ... This issue is totally worth to do!

What does 2016-07-24T20:30:20 [DEBUG] CookieJar - Rejected Cookie actually means?

@Lendude
This makes sense.

Here is an update of the issue summary.

I also asked @chx to clarify why we need this time validation in the first place.

According to chx this is some way of protection against replay attacks.

This patch solves a lot of problems that we have in Entity browser: #2765789: [Test] Create basic Javascript test base. Solving this is critical for test coverage improvements of Entity browser.

Hmm most APIs I've used had like 5-15 minute windows on their anti-replay timestamps, so would it be ok to raise it here too? I have no idea.

Yeah. 5-15 minutes still sounds sane. I still believe that you should simply not consider to even install simpletest module, in which case we exit early.

Let's see whether the security team could help us out here. Feel free to ping me at any point.

Could this explain the random fails for postgres in #2737805: Convert web tests to browser tests for forum module?

+++ b/core/includes/bootstrap.inc
@@ -642,7 +642,7 @@ function drupal_valid_test_ua($new_prefix = NULL) {
     // Since we are making a local request a 5 second time window is allowed,
     // and the HMAC must match.
-    if ($time_diff >= 0 && $time_diff <= 5 && $hmac === $test_hmac) {
+    if ($time_diff >= 0 && $time_diff <= 180 && $hmac === $test_hmac) {

The comment is now out of sync, please update it. We should mention why we check for 180.

I'm not too happy about the fix since it just fixes the symptom by increasing the time limit. Why do we have the time limit here in the first place? What could a replay attack look like that we need this?

For me its not only #23, but there is also the fact that this all doesn't depend on a system without a running simpletest instance.

I looked at the code and from my understanding a replay attack scenario has quite some requirements:

1. Simpletest must have been used on a production site.
2. An attacker got into possession of a test HTTP request that the site has sent locally to itself during testing. Which is not possible for a remote attacker, so they must have access to the server already to capture requests.
3. Simpletest did not shut down correctly and failed to clean up the test directory. It has to leave a settings.php file and .htkey behind in the file system.
4. The attacker finds a weakness to break out of the child test site and get access to the database of the main site. I can't think of a way to accomplish that, but would be very curious to see creative attempts :-)

Given that at least 2. makes an attack not practical I think the timestamp check is redundant and can be removed.

Recommended based on the original issue: ask pwolanin instead of me. (note that the git commit message is broken and refers to #3518404 instead of #518404 but this is the issue without a doubt, Dries accidentally typed an unshifted # after the # in the commit message)

I pinged pwolanin to check this issue.

Patch looks good, some nitpicks in the meantime:

1. +++ b/core/includes/bootstrap.inc
   @@ -638,11 +638,9 @@ function drupal_valid_test_ua($new_prefix = NULL) {
   +    // Check that the HMAC match.
   

   "matches"

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Session/SessionTest.php
   @@ -0,0 +1,58 @@
   +  /**
   +   * @var User
   +   * The current logged in user.
   +   */
   +  protected $user;
   

   This variable is never used and can be removed.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Session/SessionTest.php
   @@ -0,0 +1,58 @@
   +    $this->user = $this->drupalCreateUser([
   +      'administer site configuration',
   +      'access administration pages',
   +      'access content overview',
   +    ]);
   +    $this->drupalLogin($this->user);
   

   This can just be a local variable $user, since we never use it later.

I wonder if we should mention drupal_valid_test_ua() in the test case comment somewhere to leave behind a hint why we have this test?

I really don't think we should remove this time check completely since in addition to replay protection, it also offers some protection against brute force attacks. If possible, we should make the whole mechanism stronger. It seems there is a suggestion of writing a temp file with a random value also? I'm not sure who that will work for a parallel test run? We don't want to rely on user input in the header to find the file.

I think dawhner has a good suggestion of also making sure the simpletest module is enabled, if we don't already check that?

We are already writing a file with a random value: the .htkey file we are using, or do you mean something else?

As I said: an attacker cannot perform a replay attack because they can never capture the local request the test system performs on itself. So we are protecting something that cannot be exploited anyway, unless I'm missing something.

If we want to have better protection against brute force attacks then we can simply make our private .htkey longer.

Simplestest module does not have to be enabled when phpunit browser tests are executed, so we cannot check for that.

Addressed my nitpicks and made the .htkey private key longer to better protect against brute force attacks.

Does that address our concerns sufficiently? (I would really like to avoid raising the time limit, because then people will just run into this problem later again when they have long running tests)

Looks good to me. Based on the discussion it seems that this still provides sufficient level of security.

Thank you all!

Looks good! It seems we have agreement from several people here and addressed the concerns. @pwolanin: feel free to push back if you see something we missed.

We can't check if the simpletest module is enabled because under 99.9% of tests it is not enabled on the child site. I agree with @klausi now we've added the .htkey we're fine. Another thing to look at in the future is to move all the drupal_valid_test_ua stuff into middleware that is only added by test system when it writes the settings.php for the child site. Thereby making it impossible for the parent site to ever become a test site.

Here is a quick followup: #2783125: Exploring to move drupal_valid_test_ua() functionality into a middleware

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Session/SessionTest.php
@@ -0,0 +1,54 @@
+    $this->drupalGet('admin/content');
+
+    $session_assert = $this->assertSession();
+
+    $page = $this->getSession()->getPage();
+
+    for ($i = 0; $i < 15; $i++) {
+      $page->clickLink('sort by Title');
+      $session_assert->statusCodeEquals(200);
+    }

We could make this more explicit by sleeping for some seconds and not using views, but I'm not convinced this should block this patch.

The test prefix is sent in the headers, so a file upload vuln could possibly be leveraged into a site install vuln?

I don't think the threat and security model here is sufficiently fleshed out or documented.

If we don't want to check a timestamp in the UA header, maybe we should check the ctime of the .htkey file? certainly there should be a reasonably short limit to its validity (minutes)? If a bug or fatal error mean it's not cleaned up and we allow a replay attack by removing the timestamp check, then that's a new vuln avenue?

Should we use the inode number of the .htkey file instead of in addition to ctime of the code file?

The prefix is currently a 10 digit number (?), and that's used for the path to the .htkey file.

I don't understand your point about the prefix. Code in drupal_valid_test_ua():

if (isset($user_agent) && preg_match("/^(simpletest\d+):(.+):(.+):(.+)$/", $user_agent, $matches)) {
    list(, $prefix, $time, $salt, $hmac) = $matches;

So $prefix must always be a string starting with "simpletest" and then having only digits. There is no way for an attacker to supply a malicious prefix that would contain other characters that are necessary to read data from an attacker supplied key file. Maybe I'm missing something and you can point that out?

Checking the ctime of the .htkey: that would then be the same timestamp check we are doing now, and we would potentially run into this same problem again with long running tests.

About replay attacks: we have already established that they are not possible, because an attacker would have to capture requests to replay them. Since the site is sending requests locally to itself the attacker would need to have access to the server already to catch them there. Can you elaborate on the replay attack scenario you think would be possible?

Inode number: we are already using it, please check the code in drupal_valid_test_ua().

I'm setting this back to needs review to indicate others should have a look as well, but given that there is no new info about potential attack scenarios I think we can go back to RTBC?

Checking the ctime of the .htkey: that would then be the same timestamp check we are doing now, and we would potentially run into this same problem again with long running tests.

Right, let's assume we would have the ConfigImportAllTest and use an actual browser for that, we would probably run into those timestamp issues as well. For me its also a bit of a problem to have a slightly different behaviour in a test compared to a real site. You never know what kind of problems might come out of that ...

This issue is now blocking all browser test conversion issues from #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase).

Can someone else confirm my analysis and put this back to RTBC?

I don't think this is RTBC. A fatal error that leaves a .htkey file behind would allows a replay attack?

I think we need a doc page that goes through the threat model and how possible problems are mitigated?

I tried out the idea in #35... didn't work unfortunately.

Why don't we just bump the timeout to 180 seconds to start with? And open another issue to discuss removing it - so at least we get to fix the current issue. Because to be honest whether it's 5 or 180 it should be fine.

There is still a potential of random failures for some long interactions on the screen, but its hopefully less likely.
I imagine for example a fully tested outside in workflow to potentially take longer than 3 minutes.

@pwolanin: I repeat for the third time: a replay attack is not possible because an attacker cannot capture the requests. See my previous comments.

And I was thinking about possible threat models and attack vectors already, see my previous comments. If you know of anything more please mention it here, otherwise we would just have an empty doc page? It looks like we are operating on FUD here.

OK I give up - for the sake of making progress let's just increase the timeout. I propose something higher to not run into this timeout again too soon ==> how does 10 minutes == 600 seconds sound?

Here is a patch increasing the time limit to 10 minutes, including a comment pointing to this issue that the time check should be removed in a future version.

Thank you @klausi
I would have commented earlier, but I don't just feel comfortable with that security question, even if I agree with your analysis.

@klausi - I'm not trying to spread FUD, but saying that a replay is "impossible" ignores that many silly things that people do in the real world.

A doc page about this shouldn't be empty if we actually explain how it works and what threats are considered.

I still think adding a check on the time of the .htkey file would harden this whole system more usefully.

+++ b/core/includes/bootstrap.inc
@@ -640,9 +640,13 @@ function drupal_valid_test_ua($new_prefix = NULL) {
+    // @todo the additional time check appears to not add any security and can
+    // cause random test fails for long running tests. See the discussion in
+    // https://www.drupal.org/node/2771547 for why it should be removed in a
+    // future version.

Nit: @todo additional lines should be indented.

1. +++ b/core/includes/bootstrap.inc
   @@ -640,9 +640,13 @@ function drupal_valid_test_ua($new_prefix = NULL) {
   +    // @todo the additional time check appears to not add any security and can
   +    // cause random test fails for long running tests. See the discussion in
   +    // https://www.drupal.org/node/2771547 for why it should be removed in a
   +    // future version.
   

   Let's not add this @todo - the facts of this have been disputed so let's not add it.

2. +++ b/core/includes/bootstrap.inc
   @@ -640,9 +640,13 @@ function drupal_valid_test_ua($new_prefix = NULL) {
   +    if ($time_diff >= 0 && $time_diff <= 600 && $hmac === $test_hmac) {
   

   I think we should throw an exception exception in the case the timeout is exceeded. So it is easy to track down the test that has caused it. Obviously we shouldn't given any helpful info to the attackers.

3. It's weird that we through an exception if the htkey file is not readable but just don't set the test prefix if the time limit is up.

We do not throw an exception when the .htkey is not readable, we just return 403 and exit immediately. Of course it makes sense to do the same when timeout or HMAC are wrong.

Also removed the @todo, but we still haven't collected even one attack scenario that would work without the timestamp check.

+++ b/core/includes/bootstrap.inc
@@ -640,11 +640,15 @@ function drupal_valid_test_ua($new_prefix = NULL) {
-    // Since we are making a local request a 5 second time window is allowed,
+    // Since we are making a local request a 600 second time window is allowed,

We had 5 seconds, we went to 600.
Should we validate how long the test-suite runs?
Mentioned by #36

Like

// After 600 seconds/10 minutes a session must expire.
$page = $this->getSession()->getPage();
  page->clickLink('sort by Title');
  $session_assert->statusCodeEquals(200);

wait(550);
  $page->clickLink('sort by Title');
  $session_assert->statusCodeEquals(200);

wait(600);
  $page->clickLink('sort by Title');
  $session_assert->statusCodeEquals(403);

A timeout-test gives an upper-limit for all other tests.
It might help analyze future test-failures.

#52 Not a security expert and I don't know an attack scenario. But I do see a 120x stampede attack surface if you get through #25.

One problem with waiting for that long is that it would slow down potentially the entire test suite for quite a while.

I'm not in favour of a test

+++ b/core/includes/bootstrap.inc
@@ -640,11 +640,15 @@ function drupal_valid_test_ua($new_prefix = NULL) {
+      header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
+      exit;

Should we add information that this is due to an invalid SIMPLETEST_USER_AGENT? I don't think that that would give an attacker anything since they already know this...

And I'm definitely -1 to 10 minute test doing nothing.

Should we add information that this is due to an invalid SIMPLETEST_USER_AGENT? I don't think that that would give an attacker anything since they already know this...

Yeah, I mean if they want to use this, non existing vulnerability, they already, due to the header prefix, know exactly what they are dealing with.

@alexpott
Would you be okay with a test which sleeps for 6 seconds?

@dawehner not adverse to such a test... but I'd rather just bump the number of clicks to the test added here...

@dawehner not adverse to such a test... but I'd rather just bump the number of clicks to the test added here...

For me this clicking is kind of a really implicit test.

It is important to perform the test as logged-in user, so that the page caching does not get in the way.

I think clicking a couple of times is the right approach, since that is the actual problem we ran into when doing multiple requests with Mink. 50 times seems a bit high, but good enough for me.

+1 on printing a message with the 403 saying the SIMPLETEST_USER_AGENT is invalid.

If we do something like:

header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden (SIMPLETEST_USER_AGENT invalid)');

Would work.

I just looked at RFC 2616 and we are allowed to change the reason phrase in HTTP: "The reason phrases listed here are only recommendations -- they MAY be replaced by local equivalents without affecting the protocol."

So yes, alex' suggestion should be ok.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Session/SessionTest.php
@@ -0,0 +1,61 @@
+    for ($i = 0; $i < 25; $i++) {
+      $page->clickLink('Link to front page');
+      $session_assert->statusCodeEquals(200);

I still think that explicit waiting and then clicking once would be better to read.

For example wait for 6 seconds, and then click a link

Looks good, thanks! I also think the test case with clicking multiple times is valid.

@dawehner I think that would be a better explicit test for the fix, but I do think the actual issue we are trying to fix here is not being able to do enough consecutive actions before something goes wrong. With a 6 second test, you could introduce a 7 seconds timeout and everything would be green, but the same issue of not being able to do enough consecutive actions would be back.
So in that light I think the current test is pretty valid.

Well, when you don't do at least something for +5 seconds, you haven't written a test which ensures that this issue continues to work. Well, I don't care :)

Here is a new followup to discuss the removing again: #2790439: Remove drupal_valid_ua() time protection

Committed and pushed ee38d6f to 8.3.x and 2a4eab9 to 8.2.x. Thanks!