Problem/Motivation
#2242749: Port Form API security fix SA-CORE-2014-002 to Drupal 8 forwarded ported SA-CORE-2014-002 to Drupal 8.
Unfortunately that left a security issue whenever a reverse proxy is in use. Just discussed this with Wim Leers in irc and we think it's only an 8.x security issue.
D7
+ if (variable_get('cache', 0) && drupal_page_is_cacheable()) {
+ $form_state['build_info']['immutable'] = TRUE;
+ }
This is OK, since $conf['cache'] determines cacheable headers.
if ($this->configFactory->get('system.performance')->get('cache.page.use_internal') && $this->isPageCacheable()) {
$form_state->addBuildInfo('immutable', TRUE);
}
While this looks superficially to be a direct port, unfortunately it's not, since the internal page cache setting only affects the internal page cache now, not whether cacheable headers were sent.
Proposed resolution
Option 1:
Wait for #2502785: Remove support for $form_state->setCached() for GET requests. If/when that issue is done, this problem goes away since there'd be no $form_state persistence during any cacheable request.
Option 2:
Wait for #2503327: Make all persisted form builds immutable. If/when that issue is done, this problem goes away since immutability would be in effect always.
Option 3:
While waiting for options 1 and/or 2, move the code that's currently in page_cache_form_alter()
into system_form_alter()
or similar. That solves the immediate goal of this issue to decouple that code from Drupal's internal page cache. When either option 1 or 2 are done, that alter function can be removed entirely, regardless of which module it's in at the time.
Remaining tasks
Decide on option.
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#48 | sa_core_2014_002-2421503-48.patch | 5.78 KB | lauriii |
#48 | sa_core_2014_002-2421503-test-only-48.patch | 2.9 KB | lauriii |
#47 | sa_core_2014_002-2421503-47.patch | 0 bytes | lauriii |
#47 | sa_core_2014_002-2421503-test-only-47.patch | 0 bytes | lauriii |
#44 | teenage-ninja-immutable-turtles-2421503.44.patch | 5.25 KB | larowlan |
Comments
Comment #1
Wim LeersTo clarify: "enabling the page cache" was done:
cache
setting (variable_get('cache', 0)
, the "Cache pages for anonymous users" checkbox on the performance settings page)system.performance.cache.page.use_internal
config key (the "Use internal page cache" checkbox on the performance settings page)So far, they're the same, and we're fine. Note that on the performance settings page in both 7 and 8, you have the ability to configure the
max-age
to be used inCache-Control
headers emitted by Drupal.But what's different, is that in Drupal 7, you had to enable the page cache to get the maximum age you had configured to actually be used. So, effectively, you had to enable the page cache in Drupal 7 to get the headers you need for a reverse proxy to have any effect at all. That was strange and annoying, and therefore it was fixed in Drupal 8, where that is no longer required: just configuring a maximum age will make it show up on any cacheable page (e.g. frontpage as the anonymous user).
So, the patch was ported 1:1 from Drupal 7 to 8, but because it is now possible that even without Drupal's page cache being enabled reverse proxies will be caching Drupal pages, the "immutable" flag is now no longer guaranteed to be set when necessary.
(See http://www.wunderkraut.com/blog/caching-with-varnish-drupal-7-and-cache-..., http://www.rklawson.com/blog/2012/04/14/caching-drupal-7-varnish-apc-and... for info about Drupal 7.)
Comment #2
Wim LeersIdeas so far for fixing this:
$is_cacheable
in\Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond()
) set the "immutable" flag.Comment #3
Wim LeersI think we also want this tag?
Comment #4
neclimdulWas going for a quick response and it grew...
So option 1 we'd be enforcing the rule in code that only anonymous forms can ever be cached. That's definitely true for common use cases at least.
Pros:
Simple
Cons:
Makes assumptions about site caching behavior (though seemingly reasonable ones)
Would likely require patching core to change the behavior
So option 2 assumes that if a page is cacheable, it is being cached somewhere. That is also a pretty reasonable assumption.
Pros:
Cons:
I am not sure it will be a used much but I like the idea of assuming that the page is being cached if its cacheable. Assuming the questions in the cons can be resolved or crossed out. Performance might need a quick profile and I can't think of anything immediately wrong with the non-cached immutable form id but maybe someone else knows of something?
Side note, isn't the logic in
\Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond()
basically the same logic we already have in\Drupal\Core\Form\FormCache::isPageCacheable()
?Comment #5
Wim Leershttps://www.drupal.org/SA-CORE-2014-002 says:
(Emphasis mine.)
In other words, there is a problem when:
hook_form_alter()
and set theimmutable
flag — so no patching to core would be necessary for them; they'd only have to take care of it themselves, just like in Drupal 7 (see https://www.drupal.org/node/2242659).immutable
flag unnecessarily (in case anonymous pages are actually all generated independently). It just means that some unnecessary work is done.The upside of not going for option 2, is that it automatically works with any reverse proxies — because Drupal's page cache policies cannot model how those behave, since they're external to Drupal. Varnish, CDNs, internet provider proxies, and even a search engine's cache.
Comment #7
znerol CreditAttribution: znerol commentedUnless you use Ajax forms (e.g. Drupal Commerce).
Applies if you use any server side page caching method.
Regrettably not unless you never expose any Ajax forms on cached pages.
Partially agree. It is desirable to not set it after the form cache has been cloned and the form-build id has been replaced for a particular user/session.
Comment #8
catch#2263569: Bypass form caching by default for forms using #ajax. could help with this.
1. Only store form IDs when forms are viewed (either state for anonymous users, or session for auth)
2. Move actual form state storage to $_SESSION - then it's guaranteed to be per-user.
Comment #9
Wim LeersYes, but a minority of forms uses AJAX forms. I just wanted to indicate: it's only a subset of forms that uses FormState, i.e. not 100% of forms use it.
Or even a reverse proxy! i.e. not necessarily the origin server, and not necessarily under your control.
What do you mean?
Comment #10
neclimdulWim, your final comment in #5 implies option 2 doesn't work out of the box with reverse proxies. If we use the exact same logic that defines how Drupal tells proxies if pages are cacheable how would it not work out of the box with proxies?
Comment #11
Wim Leers#10: because a reverse proxy may have its own configuration (think VCL files). In other words: it may not simply respect
Cache-Control
headers.Comment #12
znerol CreditAttribution: znerol commented@Wim Leers, when using gateway caches, you normally also install the respective integration module. This is mainly in order to make cache-flushing work. With Drupal 8 and cache-tags, it will be even more important that the backend triggers the proper
ban()
s. Therefore I do not think that core can/should do anything more than what has been implemented in the original SA for D7.Comment #13
Wim LeersRight, while I wrote #11, I was also wondering whether my argument was valid. I agree with you (#12 & #10) that my argument is pointless, because if we go down that route, you can go so far as saying that the reverse proxy is caching responses for authenticated users too, in which case… you're utterly screwed.
So, basically, yes, both options 1 and 2 in #2 & #4 are viable.
Except… (!!!) that we can only know about the request cache policy. Not the response cache policy. Because the response cache policy includes the page cache kill switch. And while building a form, we cannot possibly know whether that killswitch is going to be activated.
Therefore, we cannot actually know whether a response will be cacheable. And therefore option 1 (anonymous) is safer than option 2 (which requires knowing whether the response being built is going to be cacheable, which is impossible to know while building a form).
(That's what I noticed while working on #5, but I completely forgot to explain that. Wim--)
Comment #14
Fabianx CreditAttribution: Fabianx commentedThe default of Akamai usually is to cache everything for 10 min and ignore any and all cache control headers (btw.).
Only Edge-Control is respected.
To the issue:
- I think I agree with catch that form state should be tied to the SESSION of the user.
For getting we would of course never open a SESSION, but for progressing through a workflow - e.g. multi step form - we would.
On the hand the same can be achieved without SESSION, but similar to the approach within cacheable_csrf we need to:
- Distinguish between creation and execution of a form
- Executing a form can store state, creation cannot
Any variation in form creation needs to be done in the same way, but that is achievable:
a) because if the original was coming from SESSION, we know the SESSION and can do the same
b) If it comes from a cookie, the cookie is the same.
b) if it comes from e.g. a GET parameter for an anonymous user, the GET can be stored as hidden text - the security level is the same.
On the other hand our normal forms already work like that, so its just the AJAX special case that would need to be changed in this way.
While 'hacky' in a way, cacheable_csrf has shown it is possible to re-create the form fully and not depend on any cache, so can we add the necessary API's restrictions to make this all simpler?
Just some more food for thought.
Comment #15
Wim LeersTo ensure I understand things: #8 and #14 basically are saying that another option (let's call it option 3) is to remove the
immutable
flag altogether, by tying form state to$_SESSION
?Comment #16
znerol CreditAttribution: znerol commentedThe
immutable
flag was just the least intrusive way to fix the issue in D6/D7. All the better if we can find a fix for D8 which works regardless of whether the resulting page is cached or not.Comment #17
Fabianx CreditAttribution: Fabianx commented#15 Not yet clear how, but lets make it a goal to kill form_state. That solves also a lot of memory problems related to serialization.
Comment #18
webchickThe core committers talked and agree this is a critical, since it's a security issue. Tagging.
Comment #19
catchComment #20
larowlanSo here it is with session for form state cache entries stored in $_SESSION instead of key-value expiry
Just for giggles.
Comment #22
larowlanSometimes $session can be NULL, any pointers on how to boot a new one?
Comment #23
znerol CreditAttribution: znerol commentedThe session is populated in the Session middleware (for normal requests going through
$kernel->handle()
) and inprepareLegacyRequest()
for old procedural code. Currently the session is not available from the request in the installer (that is taken care of in #2228393: Decouple session from cookie based user authentication).The session is never available from the request when running from the command line (
PHP_SAPI === 'cli'
). As a result it will not be there on the simpletest parent site when running from the command line and/or when theRequest
on top of the stack was instantiated manually.I will take a look at the test failures.
Comment #24
jhedstromIndeed. I tested some of these failing tests from the command line, and they failed, but work when run through the Simpletest module's UI (not sure if all of them do or not).
Comment #25
jhedstromI can reproduce the failures in
Drupal\user\Tests\UserAdminTest
through the Simpletest UI (but not through manual testing of the form).The fatal error is a bit baffling, somehow the
FieldItemDataDefinition::$fieldDefinition
is being changed from a proper field definition object to an array:. It doesn't happen during instantiation of the object either, since that code relies on it being a proper object.
Comment #26
webchickMaking up a new tag, since Daniel cited this as something that can't really be moved on until a direction is chosen.
Comment #27
catchI think this should be postponed on #2263569: Bypass form caching by default for forms using #ajax. (and also #2465053: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state should probably be postponed on that) - however #20 is worth exploring for these so we should move the patch here.
Going ahead and postponing now, but if you disagree with this please move back to CNW with an explanation why.
Comment #28
Wim LeersComment #29
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNot necessarily true. Even if we remove core's usages of custom #ajax['url'], we still need to consider whether to leave it as an option for contrib. But, tim.plunkett and I have been discussing an approach that would fix this issue for such cases. He'll post a POC of that here for feedback once it's sufficiently promising.
Comment #30
Wim LeersHuh? I remember discussions concluding that the "immutable" flag becomes obsolete with #2263569: Bypass form caching by default for forms using #ajax.?
Comment #31
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThose discussions might have been based on outdated assumptions of how that issue would be solved. But, the "immutable" flag will be obsolete with the approach that Tim might post here, so maybe you're thinking of discussions about that? These issues were kind of blended together when we discussed them at Drupalcon LA and since.
Comment #32
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAlso, you might have been in a discussion that assumed we'd remove #ajax['url'] and all other $form_state->setCached() ability on GET requests, even for contrib, which maybe we should, and which would then solve this issue. But that's not part of the current scope of #2263569: Bypass form caching by default for forms using #ajax., and should get its own issue to evaluate the pros and cons of.
Comment #33
effulgentsia CreditAttribution: effulgentsia at Acquia commentedUpdated the issue summary with 3 options on how to proceed. Unpostponing this issue, since there's nothing blocking us from choosing an option. If we choose option 1 or 2, we'll need to postpone on the corresponding issue, but my recommendation is option 3, so that a supported upgrade path isn't held up on this.
Comment #34
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #35
dawehnerImplemented suggestion nr.3 and expanded the documentation a bit.
Comment #37
Fabianx CreditAttribution: Fabianx as a volunteer commentedCan we try what happens when we always set the immutable flag?
Comment #38
tim.plunkettComment #43
xjmDiscussed with the core maintainers this morning and we decided this does not need to block an upgrade path.
Comment #44
larowlanback to option 3
Comment #45
dawehnerI see, so we basically go the 3) way + test coverage which is IMHO the most important part of the issue now.
Would it be possible to provide a test only patch, so we could compare the test together with #2502785: Remove support for $form_state->setCached() for GET requests
Comment #46
Fabianx CreditAttribution: Fabianx for Drupal Association commentedYes, lets add some tests, which is a valuable actionable item for the London sprint.
Comment #47
lauriiiAdded some test coverage
Comment #48
lauriiiSuch noob
Comment #53
lauriiiComment #54
dawehnerNice! So the question is now, do we get this IN and remove the alter code once #2502785: Remove support for $form_state->setCached() for GET requests is in?
Does someone has an opinion about it?
Comment #55
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think it makes sense to add this in as-is, because after #2502785: Remove support for $form_state->setCached() for GET requests, it might make sense to open a non-critical follow-up to remove all traces of
$form_state->getBuildInfo()['immutable']
, of which this is just one.Comment #56
dawehnerYeah to independent babysteps and actual progress instead of dependency chains!
Had a look at the test coverage and it makes sense for me.
We already have additional test coverage in
\Drupal\system\Tests\Form\FormStoragePageCacheTest::testRebuildFormStorageOnCachedPage
,which ensures that immutability works still in general, so IMHO we are fine here.
Comment #57
Fabianx CreditAttribution: Fabianx for Drupal Association commentedRTBC + 1
Comment #58
catchAgreed. Opened #2524408: Remove $form_state immutability.
Committed/pushed to 8.0.x, thanks!