Attribute Manipulations are a security risk

[relaxnow]

AMs are written in PHP and as such have complete control over EngineBlock, if someone were to gain access to the Service Registry (with something like CSRF) then it would be trivial to inject something malicious into EB.

It might be worthwile to migrate AMs to a more restricted language (that is still turing complete but specifically designed for such a purpose) like Lua: http://php.net/manual/en/book.lua.php

[relaxnow]

7.3.3 SUR-703 – Code Execution Vulnerability ID: SUR-703 Vulnerability Type: Code Execution Threat Level: Extreme

Description

The response manipulation mechanism provided by EngineBlock is exposed through a web form in ServiceRegistry. This form enables admin users to directly edit the PHP hooks that are executed when SAML messages pass through EngineBlock.

Technical Details

EngineBlock provides a facility for manipulating SAML responses that pass through it. This is accomplished through admin-defined PHP hooks. ServiceRegistry provides direct access to these PHP hooks, presumably for ease of administration. An attacker which has access to an admin account (either via stolen credentials/sessions or through XSS in an admin's browser) can modify these hooks arbitrarily. By inserting exploit code and either waiting for a suitable SAML response to pass through EngineBlock or even triggering such a response explicitly, this grants the attacker code execution on the webserver in the context of the webserver process' user.

Impact

This vulnerability represents a critical avenue for code execution on the webserver. The triviality of exploitation coupled with the relative ease of accessibility (e.g. obtained using SUR-701 and SUR-702) make this an especially dangerous vulnerability.

Recommendation

It should be considered whether this functionally is really needed, and has to be exposed through a web interface. If at all possible, we strongly recommend removing access to these hooks through the webapp. Sandboxing of the PHP hook execution is not a promising avenue of mitigation.

[thijskh]

Let's state up front that the choice for a turing complete attribute manipulation has been deliberate and is the raison d'etre of this functionality. Re the recommendation: the functionality is really needed.

Although the potential abuse when malicious code is injected is large, on a more abstract level it's not an elevation of possibilities compared to other metadata fields. That is, if we were to remove this feature but someone would still be able to manipulate the SR web interface in the same way, they could replace certData for an IdP and have all the freedom they want to impersonate people, thereby violating the core purpose of the platform. Singling out this feature for removal would therefore not in that sense make the platform significantly more secure. Having the facility in SR means that it's detectable and reversible when the field is being abused. Therefore, I reject the 'extreme' classification of this issue.

Nonetheless, as OP said on a new implementation a good case could be made to use a separate language like LUA instead, so it's more easily sandboxed. This would limit the actions malicious code can do to a very defined set, instead of doing random things on the system, so would certainly be an improvement.

However, given the arguments above and the expensive migration scenario, I do not see this fixed in the short term.

[thijskh]

Setting an appropriate disable_functions in PHP does limit what AM code can do. I've added some documentation about this. For now I think we consider this "as designed".