[Page] ComponentFilesImpl should not allow arbitrary directory listings

Current Behavior The ComponentFilesImpl model allows for security bypass. The ComponentFiles model may be constructed injecting any resourceTypes (path) and a all-inclusive filter (e.g. .*) and will provide a full directory listing as permitted for the sling-scripting authorizable.

Consider the following htl script:

<ol data-sly-use.componentFiles="${'com.adobe.cq.wcm.core.components.models.ComponentFiles' @ resourceTypes = '/apps/core/wcm/config', filter='.*'}"
    data-sly-list.toInclude="${componentFiles.paths}">
    <li>${toInclude}</li>
</ol>

The output of this call will be:


<ol>

    <li>/apps/core/wcm/config/com.adobe.cq.ui.wcm.commons.internal.servlets.rte.RTEFilterServletFactory.amended-core-components.config</li>
    <li>/apps/core/wcm/config/com.day.cq.wcm.foundation.forms.impl.MailServlet-core-components.config</li> 
... etc
</ol>

A service user mapping should be limited to performing a specific task that is identified by the bundle and sub-service identifier; and should have the minimum required permissions to accomplish the task.

The task performed by this API is to allow a full directory listing of any application code path. I believe that this model is exporting, beyond the scope of the bundle + sub-service, a level of access that should not be available.

If I want another bundle to have this level of access, I would expect to have to explicitly add a new service user mapping for that bundle/subservice; as it stands now, that bundle would be free to piggy-back off of the privileges granted to the CC bundle

Expected behavior/code Not to allow this.

Environment

AEM 6.5.5.0
Core Components 2.11.0
JDK 11

Possible Solution This feature looks like it is intended to allow headlibs inclusion for AMP pages. A first step might be removing the filter injection from the model and constraining it internally to the headlibs pattern (customheadlibs\\.amp\\.html) such that the API cannot be used to list all contents.

@ky940819 , this is an interesting discussion.

We initially planned to define ACLs for the service user along with its usages (see removed ACLs in f622067). Since AEM best practices are now to use principal-based authorisation, we decided to switch to that and, by using the sling-scripting principal, allowed read rights to /apps and /libs, which is where components live.

This expands quite a bit the range of things that can be accessed, indeed. However, to access them, one would need to be a developer that has at least write access to /apps (or /libs), to be able to place the HTL scripts. So we don't have a privilege escalation situation, as the strongly-worded security bypass implies. Lower privilege users would not be able to use this mechanism to gain access to things.

We chose to allow filter configuration because we imagined at some point we might want to include other things (when a certain component is added to the page): footer JS scripts, meta tags and so on. Hardcoding these filters would reduce the usefulness of this helper. We even added it to the main Core Components bundle (as opposed to the AMP extension one) because we thought other developers would want to use it too. Although, care should be taken to not misuse it (<enter Peter Parker Principle quote here>).

If you feel allowing this kind of access is still too broad, we could probably enforce in the implementation checking that the paths specified in resourceTypes are actually cq:Components. This will reduce the access of this helper closer to its envisioned scope. WDYT?

Hello,

By "security bypass" i did not intend to imply an actionable exploit at this time. However, it does potentially open the door to one in the future if this feature is ever misused (i.e. if any form of user input is allowed to define the injected filter value for the model).

My main concern is that I think it goes against the sling security model, allowing this type of read access to any script that may be executed seems to undermine the rationale behind the removal of any script being able to perform an administrative login.

My concerns would be addressed if the utility was not accessible outside of the bundle. In that way the models could still define the resourceType and filter, but the utility wouldn't be made available to others.

Or, the utility methods could be made public, but require that a ResourceResolver be provided, which would shift the responsibility for performing the service user login to the calling class/method/bundle; it would not provide an elevated resource resolver for you. In this set up if the AMP bundle wanted to use this API, then it would require that a service user mapping be configured to allow the AMP bundle to have this access. This would also allow for different sub-services to be granted different permissions.

I understand what you mean but this helper was designed to be used directly from HTL, and it lives inside the Core Components bundle. I'm not sure we would be able to pass it a ResourceResolver that uses a service user mapped to another bundle. Will need to test.

Considering #1118, passing a resource resolver between bundles (via HTL) will also make it difficult to close it after usage without some over-engineering: passing a factory from the consumer bundle and making sure to acquire/close the RR properly from the helper.

I think what could be done there, if the ComponentFiles utility is required outside the bundle is to make it not a model but a utility instead, with the interface:


public interface ComponentFiles {

    getPaths(Collection<String> resourceTypes, String regex, boolean inherited, ResourceResolver resourceResolver);

}

Or it could be declared as a service, if you want the implementation to be internal.

Then in ampheadlibs.html I would remove:

    <sly data-sly-use.componentFiles="${'com.adobe.cq.wcm.core.components.models.ComponentFiles' @ resourceTypes = page.componentsResourceTypes, filter='customheadlibs\\.amp\\.html'}"
         data-sly-repeat.toInclude="${componentFiles.paths}"
         data-sly-include="${toInclude}"></sly>

and replace it with something like

    <sly data-sly-repeat.toInclude="${ampPage.headLibs}"
         data-sly-include="${toInclude}"></sly>

Then AmpPage#getHeadLibs() would need to be defined. Something along the lines of:

    public Collection<String> getHeadLibs() {
        try (ResourceResolver resourceResolver = resolverFactory.getServiceResourceResolver(Collections.singletonMap(ResourceResolverFactory.SUBSERVICE, COMPONENTS_SERVICE))) {
            return ComponentFiles.getPaths(currentPage.getComponentsResourceTypes(), "customheadlibs\\.amp\\.html", true, resourceResolver);
        } catch (LoginException e) {
            LOG.error("Cannot login as a service user", e);
            return Collections.emptySet();
        }
    }

It doesn't have to be in AmpPage, that is just an example.

I whipped this up as an example of a static utility. It doesn't have to be done this way at all, and the example doesn't fix the issue. It just demonstrates the abstraction of the utility from the model: https://github.com/ky940819/aem-core-wcm-components/commit/b8dc0f36a94e3744ec19d76bacf03c0703e241e8

It can't be directly used by the presentation; but it allows for reusable logic while putting the onus of managing the resource resolver onto the caller.

adobe / aem-core-wcm-components

[Page] ComponentFilesImpl should not allow arbitrary directory listings #1119