#2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE adds a new security mechanism to prevent untrusted callbacks. In a typical flow this will be implemented by first deprecating and then throwing an exception. This allows for code to move with the times without breaking the world. So for example in Drupal 8.8.x all render callbacks must be trusted or a deprecation error will be triggered. This only affects tests and then in Drupal 9.0.0 this will be changed to throw an exception.
Some security team members want an additional per site switch to opt into throwing exceptions early.
To discuss:
$settings['security_harderned'] vs $settings['security_trustedcallback_exception']None - likely a new setting in default.settings.php
None
None
@tbd
Comments
Comment #2
dsnopekMy vote is for "yes, it's worth it."
This is currently the easiest way to escalate an XSS vulnerability to RCE, when the XSS script is run as an admin user. We removed the 'php' module from core to prevent this same kind of escalation pattern. Without a way to make this throw an exception, we're stuck with similar workarounds from the D6/D7 days, like
rm -rf'ing certain modules from core in your site build process. I think more people will add a line to settings.php, than will implement a build process.Between those two options, I'd prefer the more specific one ('security_trustedcallback_exception') because this will eventually become a no-op once D9 is always throwing an exception. We might want to use the more generic name ('security_hardened') for something else some day and don't want to conflict with some old code in settings.php.
In Drupal 9, I think we should just always throw an exception and ignore this setting.
Comment #3
dww@alexpott: Thanks for opening this issue and for being willing to consider a setting to force exceptions for this.
FWIW, I totally +1 everything from @dsnopek in #2:
- Yes it's worth adding a flag for this in 8.8.x.
- Flag should be specific in name to avoid both conflicts and potential confusion (not that the specific name "security_trustedcallback_exception" is self-documenting for mere-mortals... we can perhaps bikeshed something more clear in this issue).
- Flag should be ignored in D9* and beyond once we always throw the exceptions.
Thanks,
-Derek
Comment #4
dsnopekUnpostponing because #2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE was committed
Comment #5
xjmComment #6
xjmComment #7
samuel.mortensonIs making this configurable before D9 required to throw exceptions in D9? I'm wondering if a separate issue should be filed to remove the deprecation and throw an error in D9, since I think that should happen regardless of what happens in this issue.
Comment #8
alexpottNope it's not necessary - we're already promising to throw exceptions for this in Drupal in the deprecation message we're emitting.
I think we should leave this the discussion about making it configurable in Drupal 8 and have a new issue to ensure that we remember to make the change in Drupal 9.
Comment #9
samuel.mortensonThanks @alexpott, I just filed #3081025: Remove technical debt and complication from when doTrustedCallback() could either trigger errors or throw exceptions.
Comment #16
dwwI don’t think this is still relevant. 8.x is no longer supported, and 9.x already throws the exceptions. I believe “won’t fix” is the right status. Any objections?
Thanks,
-Derek
Comment #17
dwwI guess "outdated" is more accurate. This was only relevant for 8.x. Once 9.0.x was the minimum supported, this was no longer needed.
Thanks / sorry,
-Derek