Add facility to allow security advisories to advise to disable certain routes to mitigate security risk temporarily

I think we should just look at using an array of strings (maybe with block path visibility style wildcard support) in settings.php rather than regexp, because there's less to go wrong - especially for self-builders trying to deal with a security release at short notice. Something like:

$settings['routes_disable'][] = 'system.entity_autocomplete';
$settings['routes_disable'][] = 'entity.node_type.*';

Allowing a single regexp only would mean if you have two related SAs for different contrib modules you'd have to merge the logic. An array of regexps seems a bit much.

Answer to #4 is yes - I'd rather we have to make an explicit list sometimes.

Since the whole point of this is to provide non-technical site builders a way to quickly mitigate possible security holes without deploying new code, in order to make the non-CS-trained folks more likely to get this right, I'm strongly +1 to #3 and an array of strings (with wildcard support), not a regexp.

Frankly, it makes life better for the sec team, since they can put out new SAs that always include working example code for settings.php for mitigation:

"Add this:"

$settings['routes_disable'][] = 'whoops';

vs.: "Find your routes_deny regular expression, parse it, and understand how to safely add |whoops in the right spot without breaking your existing logic." :(

However, I think we need to get the names of these settings right.

Original patch has: routes_exclude vs. routes_include
#3 proposes routes_disable. It's not explicit, but it implies routes_enable

"exclude" vs. "include" seems pretty vague. "disable" vs. "enable" seems more explicit about what's happening.

"routes_deny" might be even more clear.

However, the exception setting (as it works in the code) does not "include", "enable" or "allow" the listed routes. It's really routes_X_exceptions (e.g. "routes_deny_exceptions") or something. It acts like AccessNeutral, not AccessAllow. Putting a route in that list doesn't "enable" it. It's a case where double negative is not necessarily positive. ;) Confusing. Not sure the best approach, but I actually think a setting name with a double negative might be more clear for this.

e.g. "routes_deny_exceptions" not "routes_allow".

Needs work for converting to an array of strings with wildcards (and tests)
Needs bikeshed for better setting names. ;)

Cheers,
-Derek

@effulgentsia, why did you opt for regexp instead?

Because I forgot that we already had precedent for glob-style expressions elsewhere in Drupal. Given that we do, yeah, +1 to that, since they're less fragile than regex. All we need is support for *.

"routes_deny" might be even more clear.

After I wrote the original patch, I realized that it would be more secure to move the logic from the access manager to a route filter. That way it runs earlier, thereby mitigating additional places where vulnerable code paths might exist: e.g., route enhancers, which run before access checks but after route filters. Therefore, disable would be more accurate than deny.

I actually think a setting name with a double negative might be more clear for this.

It might help with clarity to combine the two settings within a single structure, perhaps:

$settings['routes_disable'] = [
  'disable' => [
    'jsonapi.*',
  ],
  'except' => [
    'jsonapi.node--article.*',
    'jsonapi.comment--*',
  ],
];

That puts the word disable in there twice, but potentially that could change if we put this within a larger structure that could mitigate other things than just routes? For example, if we combined with #3037534: Add a mechanism to mark TypedData (entities and fields) internal via settings.php, then:

$settings['security_mitigations'] = [
  'routes' => [
    'disable' => [
      'jsonapi.*',
    ],
    'except' => [
      'jsonapi.node--article.*',
      'jsonapi.comment--*',
    ],
  ],
  'typed_data' => [
    'internalize' => [
      ...
    ],
    'except' => [
      ...
    ],
  ],
];

Re: disable vs. deny -- cool, I'm fine with "disable" if we change this to do more than just "deny". ;) Agreed that the earlier in the chain this can kick in to disable the routes, the better.

Re: nested arrays: While elegant from one perspective, I worry that makes this harder to use. SAs no longer can include standalone, paste-able snippets for settings.php. We're back to "find your X setting, and add this stuff in the right spot" not "add this". #9 would still make the first case way better than a regexp would, but it's still a bit clumsy. That's why I like something very simple, that's easily "stackable" with subsequent SAs, etc. Hence, a flat array for each setting.

(Edit: retracted, since I realized in #11 this isn't really true).

Meanwhile, based on conversations upstream, I think #3037534: Add a mechanism to mark TypedData (entities and fields) internal via settings.php is probably headed for won't fix. It seems there's too much disagreement over what "internal" means in core (at least as of now), so that's not a viable or useful "mitigation" vector.

Regardless, I don't think adding yet more nesting to this setting is a good idea, so I don't think we should worry about this at all.

Thanks,
-Derek

Meanwhile, for this to be a viable mitigation strategy for the JSON:API case (as intended), it seems there's significant interest in the GET vs. POST case. Sites might want to globally put JSON:API in "read-only" mode (prevent POST:/jsonapi* but continue to allow GET/jsonapi*). Or almost entirely GET-only, but allow POST to specific routes, etc.

Perhaps we should consider splitting this setting into something like:

$settings['get_routes_disable'][] = 'jsonapi/whoops';
$settings['post_routes_disable'][] = 'jsonapi*';
...

Not sure if those should be 'routes_disable_get', 'routes_get_disable', etc. All of the permutations seem to have drawbacks. ;) #include "bikeshed.h"

I guess a nested structure could still work for this, since we could do something like:

$settings['routes_disable']['post'][] = 'whatever*';

in the SAs...

And now that I think of it, I suppose that would work for #9, too. ;) New SA simply says:

$settings['routes_disable']['disable'][] = 'newly.understood.vulnerable.route';

So long as that's after your previous routes_disable array, we don't have to get into the business of telling people to modify their existing setting...

-Derek

As an alternative, instead of a single disable array and a single except array, we might want to have an array of match/exception pairs? That would allow something like this:

$settings['security_mitigations']['routes']['disable'] = [
  [
    'match' => ['jsonapi.*'],
    'except' => ['jsonapi.node--*'],
  ],
  [
    'match' => ['jsonapi.*.patch'],
  ],
];

If we wanted to disable all resource types but nodes and also wanted to disable all PATCH requests (including PATCH requests to nodes). With the single disable/except structure of #9 that would be harder to do.

SAs no longer can include standalone, paste-able snippets for settings.php. We're back to "find your X setting, and add this stuff in the right spot" not "add this"... That's why I like something very simple, that's easily "stackable" with subsequent SAs, etc.

#12 allows for that as so:

// SA 1
$settings['security_mitigations']['routes']['disable'][] = [
  'match' => ['jsonapi.*'],
  'except' => ['jsonapi.node--*'],
];
$settings['security_mitigations']['routes']['disable'][] = [
  'match' => ['jsonapi.*.patch'],
];

// SA 2
$settings['security_mitigations']['routes']['disable'][] = [
  ...
];

// SA 3
$settings['security_mitigations']['typed_data']['internalize'][] = [
  'match' => [...],
  'except' => [...],
];

Re: #13 yeah, sorry, I came to the same conclusion myself in #11. ;)

Oh, but re: #12 -- that's where wildcard vs. regexp starts to break down:

'match' => ['jsonapi.*'],

already matches everything, including: jsonapi.*.patch. So the 2nd set of match/except stuff doesn't matter, it's already been denied above...

edit: I misread, please ignore.

Re #15, no that's the key change from #9 to #12: for #12, each item in the disable array is its own separate mitigation. Or in other words, the except only applies to the item that it's in.

Or in other words, the first item says "disable all resources except nodes". The second item says "disable all PATCH requests". The total result is that even PATCH requests to nodes are disabled.

That could be equivalently expressed by the 2nd item being written as:

'match' => ['jsonapi.node--*.patch'],

But if our goal in that mitigation is to disable ALL patch requests, then we're relying on the presence of the earlier item to accomplish the rest of our intent. By expressing the 2nd item in a non-node-specific way, we accomplish our intent independently of the earlier mitigation.

Re: #16: Sorry for being dense. Makes perfect sense now. I mis-read the last paragraph of #12, leading to my confusion.

"match/except" solves my confusing setting names concerns and is better than the double-negative from #8.

We've both demonstrated how a nested array is still "stackable" from the SA example settings perspective.

The code for all this will be more complicated than the original patch, but I think the expressiveness and ease-of-use make it totally worth doing this way.

I'm still wondering if we want something in here to handle GET vs. POST, since the fact JSON:API names its routes consistently for this seems like something we don't want to have to rely on (and as you pointed out upstream, they don't provide ".get" suffixes to target).

p.s. I still don't think we want to over-generalize this to "security_mitigations" and add plumbing in anticipation that #3037534: Add a mechanism to mark TypedData (entities and fields) internal via settings.php might happen. That seems headed towards a brick wall. We don't know what other mitigations, if any, this is ever going to support, and if so, what their syntax needs will be. I think it'd be really confusing if the different parts of this array use wildly different syntax or are targeting different kinds of things. For now, I think we should keep this route-specific and named accordingly.

I.e. this:

$settings['routes_disable'][] = [
  'match' => ['jsonapi.*'],
  'except' => ['jsonapi.node--*'],
];
...

not:

$settings['security_mitigations']['routes']['disable'][] = [
  'match' => ['jsonapi.*'],
  'except' => ['jsonapi.node--*'],
];
...

$settings['routes_disable'], NOT $settings['security_mitigations']['routes']['disable']

+1

I'm still wondering if we want something in here to handle GET vs. POST, since the fact JSON:API names its routes consistently for this seems like something we don't want to have to rely on (and as you pointed out upstream, they don't provide ".get" suffixes to target).

So, I guess we could do either:

// Disables all jsonapi routes for all methods.
$settings['routes_disable']['*'][] = [
  'match' => ['jsonapi.*']
];

// Disables all jsonapi routes for POST, PATCH, and DELETE.
$settings['routes_disable']['POST'][] = [
  'match' => ['jsonapi.*']
];
$settings['routes_disable']['PATCH'][] = [
  'match' => ['jsonapi.*']
];
$settings['routes_disable']['DELETE'][] = [
  'match' => ['jsonapi.*']
];

OR

// Disables all jsonapi routes for all methods.
$settings['routes_disable'][] = [
  'match' => ['jsonapi.*']
];

// Disables all jsonapi routes for POST, PATCH, and DELETE.
$settings['routes_disable'][] = [
  'match' => ['jsonapi.*'],
  'methods' => ['POST', 'PATCH', 'DELETE'],
];

+1 to #19B: an optional 'methods' key, which if absent, means "all methods". Option A seems harder to configure properly.

Update summary to reflect the current consensus (at least between me and @effulgentsia) on how this should work.

Cleanup a few formatting things and further clarify (I hope).

Thanks @catch! I've two concerns:

1. It is that it's not all that easy for site owners to implement. It requires knowing the relevant route names and would be easy to get wrong. Regexes and end users. :(
2. It potentially makes the list of available resources not match the /jsonapi listing, leading to DX problems.

It's better than nothing, but I'm hesitant.

@xjm:

1.a. The current proposal is to use an array of strings (with * wildcard support), not regular expressions.

1.b. Given the main use case for this is immediate mitigation when a security advisory is released, the SA would include a settings.php snippet with the specific routes.

Something like:

Paste the following into settings.php to immediately mitigate the security vulnerability prior to applying the patch.


$settings['routes_disable']['PATCH'][] = [
'match' => ['jsonapi.*']
];

2. Is a fair point, but given this is for mitigation between an SA being released and a core update being performed it seems OK? If sites want to permanently disable routes, they could do that using 'internal' or JSON:API extras rather than this mechanism.

Also: it would be possible to put this in config, and having system module provide a configuration page with a textarea. It could have one textarea for routes (all methods), and one each for POST/PATCH/DELETE. One rule per line same as block visibility.

Disadvantages are:

1. You have a weird configuration page we hope you're never going to have to use, and someone playing around could lock themselves out of their own admin UI by putting admin/config/development/deny-routes in there or just admin/config/* or user/*
2. Heavier to get config every request in early routing than settings.
3. A bit more work in this issue

1. You have a weird configuration page we hope you're never going to have to use, and someone playing around could lock themselves out of their own admin UI by putting admin/config/development/deny-routes in there or just admin/config/* or user/*

lol, yes, that sounds like a reason not to expose this in the UI.

I think there's an assumption here that doing a code push to settings.php is significantly easier than updating core or a module, which is not always the case. Being able to mitigate a vulnerability in the user interface is extremely valuable and can be done by any administrator, without knowledge of the codebase or hosting stack.

I agree with Sam on this. For many clients changing the settings.php is just as difficult as changing composer.json (requires a full code deployment). For many of our Enterprise clients, any sort of code change requires going through a deployment process that can take hours (or even days of testing and signoff). So the UI would be *super* useful for quick mitigation.

However, a potential side effect of this would be that if the deployment process performs a config-import, anything set "live on prod" might be lost (don't typically config-export on Prod). So devs would need to be careful to
1) set the deny config using UI on Prod, then
2) set the config within dev/stage and export it so the next deployment doesn't revert it (if the module can't be immediately updated for some reason), then
3) once the module(s) are updated, remove this config on dev/stage as part of the final deployment.

As far as locking out of the site, perhaps it could ignore this configuration for User-1 to avoid accidentally locking yourself out. Or have a permission for ignoring the denial to admin/config/* or user/* that could be assigned to whatever Admin role the site might have.

I'm not sure doing a config->get during the routing process would be a problem.

#29 gives good reasons for it be in UI and also good reasons for it to not be in config.

So, how about using State?

someone playing around could lock themselves out of their own admin UI by putting admin/config/development/deny-routes in there or just admin/config/* or user/*

Ban module has a validation error if you try to ban your own IP address. Perhaps we could do similar validation for this form's submission?