Support flood control for CSP violation reports

I can port this over to D8. Based on the description I feel this is a sensible way to do it. By default things are the same and open. People can then choose to adjust the setting to prevent any sort of performance issue.

@jweowu I was wondering why there is the disable seckit value in the config. Shouldn't users disable the module to ensure that functionality?

Fair enough.

One question though, I am trying to write a test for this, but it doesn't seem as if the simpletest browser test case sends the report, so not sure its possible to write automated test for this. Do you know if this is possible or not with simpletest?

updated patch for D8.

+1 RTBC

Thanks for the patch

1. +++ b/config/install/seckit.settings.yml
   @@ -39,3 +39,13 @@ seckit_various:
   +seckit_advanced:
   +  disable_seckit: FALSE
   +  unlimited_csp_reports: TRUE
   +  csp_limits:
   +    max_size: 4096
   +    flood:
   +      limit_user: 100
   +      window_user: 900
   +      limit_global: 1000
   +      window_global: 3600
   

   We are adding more keys in the config. This needs an hook_post_update_NAME for adding new keys to the config store. and a test class that would extend UpdatePathTestBase

2. +++ b/src/Form/SecKitSettingsForm.php
   @@ -520,6 +537,94 @@ class SecKitSettingsForm extends ConfigFormBase {
   +      '#title' => t('Advanced options'),
   

   use injectable t instead of t i.e. use $this->t
   Address it at other places as well.

3. +++ b/src/Form/SecKitSettingsForm.php
   @@ -520,6 +537,94 @@ class SecKitSettingsForm extends ConfigFormBase {
   +      // '#description' => t(''),
   

   Remove this commented line as the title makes more sense.

4. +++ b/src/Form/SecKitSettingsForm.php
   @@ -520,6 +537,94 @@ class SecKitSettingsForm extends ConfigFormBase {
   +    if ($config->get('seckit_advanced.disable_seckit') && empty($_POST)) {
   

   Inject the request stack instead of naked usage of $_POST

5. +++ b/src/Form/SecKitSettingsForm.php
   @@ -520,6 +537,94 @@ class SecKitSettingsForm extends ConfigFormBase {
   +      '#open' => TRUE, //($options['seckit_advanced']['csp_limits']['flood'] == $defaults['seckit_advanced']['csp_limits']['flood']),
   

   is commented code needed here ? if not remove it.

I have updated the patch to reflect these comments.

Regarding #17.1,
Maintainers,
As the module has relesed a alpha yet and in the alpha phase the changes in config schema is possible as drupal core follows. So do we really need the hook_post_update_NAME ? If yes then I think no.

#20.1

Could someone please make the unrelated t() vs $this->t() changes into a separate issue/patch so that can be committed independently/first. The patch for this can depend on that one.

I'm +1 to this. Let's limit the scope of this issue.

As far as the update, I don't think its completely necessary given the alpha state, but Its there in the patch, so I don't see any harm in keeping it in, unless there is some precedence that states that it absolutely shouldn't be there.

I am also +1 for removing the $this->t, I can create another issue for that and update this patch.

but Its there in the patch,

I was more concerned about the hook_update_n in the module which is not unit-testable.So in my opinion instead of pushing the non-testable code, better to remove it if its not needed.Rest is all on maintainers.

I've reverted the $this->t() so the patch doesn't have to put a dependency on another patch.

actually, need to update the 'seckit_advanced' section to use $this->t() because that section is not in the patch for #2788701: Use t() using DI in SecKitSettingsForm

Great thanks for the patch! Few more that are needed to let me RTBC it

1. +++ b/seckit.module
   @@ -45,3 +54,55 @@ function _seckit_form_alter_login_form(&$form, FormStateInterface &$form_state)
   +function _seckit_csp_report_flooding_detected() {
   

   Move this function to a service with name may be "seckit.helper" choose a better name if you know.

2. +++ b/src/EventSubscriber/SecKitEventSubscriber.php
   @@ -32,6 +32,11 @@ class SecKitEventSubscriber implements EventSubscriberInterface {
   +    $config = \Drupal::config('seckit.settings');
   +    if ($config->get('seckit_advanced.disable_seckit')) {
   +      return;
   +    }
   

   What's the need of getting the seckit settings again. you can access it from $this->config which is getting set in constructor

3. +++ b/src/EventSubscriber/SecKitEventSubscriber.php
   @@ -41,6 +46,11 @@ class SecKitEventSubscriber implements EventSubscriberInterface {
   +    $config = \Drupal::config('seckit.settings');
   +    if ($config->get('seckit_advanced.disable_seckit')) {
   +      return;
   +    }
   

   same as above. access it from $this->config

4. +++ b/src/EventSubscriber/SecKitEventSubscriber.php
   @@ -106,7 +116,6 @@ class SecKitEventSubscriber implements EventSubscriberInterface {
   -    global $base_root;
   

   +1

5. +++ b/src/EventSubscriber/SecKitEventSubscriber.php
   @@ -131,7 +140,6 @@ class SecKitEventSubscriber implements EventSubscriberInterface {
   -    $warning = t($message, $args);
   

   +1 Nice :)

6. +++ b/src/Form/SecKitSettingsForm.php
   @@ -522,6 +539,94 @@ class SecKitSettingsForm extends ConfigFormBase {
   +      drupal_set_message(t("Security Kit is currently disabled in the Advanced options (below)."), 'warning');
   

   use injectable $this->t and use single quotes

7. +++ b/src/Form/SecKitSettingsForm.php
   @@ -522,6 +539,94 @@ class SecKitSettingsForm extends ConfigFormBase {
   +    // CSP report limits
   

   dot at the end of the comment.

8. +++ b/src/Form/SecKitSettingsForm.php
   @@ -522,6 +539,94 @@ class SecKitSettingsForm extends ConfigFormBase {
   +      '#description' => $this->t("Reports breaching these limits will not be logged."),
   

   use single quotes. Its not a standard yet but php gives performance hits using single quotes over doubel

as far as #1, I think there are a lot of methods that need to go in this. I agree that we need a service and it should be implemented. I think its outside the scope of this 1 issue. I think we should create another issue for it. I've addressed all the other issues as well as implemented the patch that @jweowu just posted.

as far as #1, I think there are a lot of methods that need to go in this. I agree that we need a service and it should be implemented. I think its outside the scope of this 1 issue. I think we should create another issue for it.

+1 Create a followup issue for it

1. +++ b/seckit.install
   @@ -0,0 +1,21 @@
   +<?php
   +
   +/**
   + * @file
   + *   Install file for seckit module.
   + */
   +
   +/**
   + * Set defaults for seckit_advanced settings.
   + */
   +function seckit_update_8001() {
   +  $config = \Drupal::configFactory()->getEditable('seckit.settings');
   +  $config->set('seckit_advanced.disable_seckit', FALSE)
   +    ->set('seckit_advanced.unlimited_csp_reports', TRUE)
   +    ->set('seckit_advanced.csp_limits.max_size', 4096)
   +    ->set('seckit_advanced.csp_limits.flood.limit_user', 100)
   +    ->set('seckit_advanced.csp_limits.flood.window_user', 900)
   +    ->set('seckit_advanced.csp_limits.flood.limit_global', 1000)
   +    ->set('seckit_advanced.csp_limits.flood.window_global', 3600)
   +    ->save();
   +}
   

   As specified above we need hook_post_update_NAME for the config update changes. Profit for this is hook_post_update_NAME is testable as well.

2. +++ b/seckit.module
   @@ -20,6 +20,15 @@ define('SECKIT_X_FRAME_ALLOW_FROM', 3); // set X-Frame-Options HTTP header to Al
   +define('SECKIT_CSP_REPORT_MAX_SIZE', 4096); // Max accepted byte count
   +define('SECKIT_CSP_REPORT_FLOOD_LIMIT_USER', 100); // Max reports per IP address...
   +define('SECKIT_CSP_REPORT_FLOOD_WINDOW_USER', 900); // ...per time window (in seconds)
   +define('SECKIT_CSP_REPORT_FLOOD_LIMIT_GLOBAL', 1000); // Max reports globally...
   +define('SECKIT_CSP_REPORT_FLOOD_WINDOW_GLOBAL', 3600); // ...per time window (in seconds)
   

   Move the comments to above. This is violating the coding standards.Address it at other places as well.

Rather than having the update at all, I am removing this since its an alpha release and we don't necessarily need to support this. I fixed #2 for the code added in the patch, but there is already an issue to fix coding style for the rest of the module here #2715611: Fix coding style to meet Drupal standards so I did not address it in other places.

Rather than having the update at all, I am removing this since its an alpha release and we don't necessarily need to support this.

yup we don't need during alpha phase

I fixed #2 for the code added in the patch, but there is already an issue to fix coding style for the rest of the module here #2715611: Fix coding style to meet Drupal standards so I did not address it in other places.

Thanks but we should not pollute more.Fix this at other places too. I'll be more comfortable to RTBC it after these gets fixed

@jweowu ,
could you share the link of the commit & issue ?

I've updated this with the updates from the second patch.

@naveenvalecha, I updated the coding style for the sections that I updated with this patch to not pollute it more. I disagree with updating it in other places as that is already taken care of by the issue I linked to above.

jweowu, I don't necessarily need to throw a 404, but I can not just return, The API controller requires a specific Response object, I can return a blank object I believe which will be a 200. But if CSP is not enabled, I would think that it should act as if the call back doesn't exist, which IMO would be a 404.

I disagree with updating it in other places as that is already taken care of by the issue I linked to above.

I'm fine with that

RTBC +1 leaving this for @jweowu to see his changes in #30 got accomodated

while D8 code in general is all kinds of different to D7 code, I don't wish the essential behavior of this module to diverge between the two versions

I completely agree. Makes sense about returning a 200 rather than a 404. I had not thought of the case of the browser agent response if it had already received a 404.

As far as the code, I see your point with the way the variables are named and used. There are a few methods in D8 that we used as improvements, i.e. loading things from the config rather than using the defaults array, I am not sure that affects functionality and there should be no performance hits to calling the settings from config, but I see your point about code readability and comprehension and can update those sections to ensure that the essence of the code remains intact with the D7 version. Do you prefer its update in this patch, or should we create an issue to review the code and update things like that?

@jweowu sounds good, I will take a quick look to ensure the changes specific to this patch are updated, then we can do a deeper review of the entire port.

I've updated the responses to be 200 instead of 404, and updated the logic in conditionals to exactly match D7 version.

@jweowu are you able to review the latest patch?

+++ b/seckit.module
@@ -45,3 +59,58 @@ function _seckit_form_alter_login_form(&$form, FormStateInterface &$form_state)
+    // flood_is_allowed() does not presently allow us to ignore the identifier,
+    // meaning we would need to log two flood events per CSP report in order to
+    // check both the global and per-user counts using the API function. This
+    // query enables us to do this while only registering one event per report.
+    // @see https://www.drupal.org/node/2472941
+    $connection = \Drupal::database();
+    $global_count = $connection->select('flood', 'f')
+      ->condition('event', 'seckit_csp_report')
+      ->condition('timestamp', REQUEST_TIME - $global_window, '>')
+      ->countQuery()
+      ->execute()
+      ->fetchField();

+++ b/src/Form/SecKitSettingsForm.php
@@ -69,9 +68,9 @@ class SecKitSettingsForm extends ConfigFormBase {
     // CSP report-only mode

Wouldn't registering two flood events would be safer and simpler than copying the Database Backed Flood service query into Seckit?

Couldn't you register a global and user flood event like this:

// Using identifier 'any' for global limit from any Client IP address.
$flood_service->register('seckit_csp_report', $global_window, 'any');
$flood_service->register('seckit_csp_report', $user_window);

Then, couldn't you check a global and user flood event like this:

// Using identifier 'any' for global limit from any Client IP address.
$global_allowed = flood_service->isAllowed('seckit_csp_report', $global_limit, $global_window, 'any');
$user_allowed = flood_service->isAllowed('seckit_csp_report', $user_limit, $user_window);

IMO this would be safer, as it doesn't need the Seckit module to assume / understand the schema and queries used by the Flood service.

Increasing priority.

For as long as this issue remains unresolved seckit with CSP on Drupal 8 represents a potential DDOS vector.

We have client sites where we've had to block the csp violation report URI at the edge WAF because misconfiguration of Seckit CSP rules (by the client) has lead to Database deadlock exceptions.

The core problem is that these POST requests bypass all cache layers used in a high availability architecture. They are passed through Cloudflare CDN, Varnish. A percentage of reports are also causing the HTTP 404 page, bypassing fast 404, because the browser didn't conform with the expected HTTP Request Content-Type headers.

We'll be applying this patch above as it currently stands and then testing some more. I've raised this with our team here and we'll try to get some resources focused on helping to progress with the fix.

neither is ideal so I'd say to go with whichever feels best to you.

It's arguably better to use the API we have and maintain the separation of concerns, rather than creating a tight coupling between Seckit and the current Flood service.

That's all.

What is missing to get this fixed?

Linking the related core enhancement issue for Flood control API.

Agree with @jweowu that if we're to use Drupal core Flood Control API it needs to be extended to support global event logging without duplicating stored events using a static key.

It is a pity that #2472941: Facilitate global (spanning all users) flood tests hasn't had any real attention. I think the proposal is solid and you said the security team were at least receptive to it. We just really need to get that core issue tested and sorted so this can proceed.

We're actively looking at using the patch from #52 in production so we've created a MR for it. It also incorporates some minor fixes for D10 compatibility; there may be more adjustments to come. Will look at 2472941 when I have the chance.

Rerolled for 2.x in MR 52.

Revisiting this patch after a while on a 10.6 site. It's still got a reference to REQUEST_TIME in there which I'll have to remove.