Limit available macros in restricted mode and enable restricted mode in HTML converter without edit rights

To limit the impact of potential security issues with macros, I suggest the following two changes:

  1. Enforce restricted mode in the HTML converter that is used by the WYSIWYG editor if the current user doesn’t have edit rights on the current document.
  2. Restrict the available macros in restricted mode to a list of allowed macros defined by the admin. By default this list should include a few macros like the info/error/warning macros.

The idea of this is proposal is that any security issue that involves macros then cannot be exploited anymore on a public wiki where registration is closed but comments are possibly enabled.

I’m aware that the first limitation probably interferes with how the change request application works so we might need to introduce a way for change request to indicate if a user should be able to use the HTML converter without restrictions on a document.

Any opinions on this?

Sounds good to me. +1

So you propose to have fewer macros available to comments than we currently have ?

Yes. At the moment, all macros can in theory be used in comments but some fail with strange errors, possibly due to restricted mode (for example, you can use macros that load LiveTable, but the LiveTable won’t load any entries or the office viewer will display an error regarding a Velocity macro). I don’t see any reason why you should be able to use macros like async, template, include, display, cache, … in a comment. And these macros are just needlessly increasing the attack surface for guests.

Admins might also install dangerous macros in a public wiki, thinking that only the trusted editors can use them but in fact anonymous users with view or comment right can execute them with arbitrary parameters. And even worse than this problem of awareness, there is not even a setting at the moment to change this.

It’s not as random as you express it, some macros take into account the restricted indication and have different behaviors depending on it.

Does that mean that those macros should stop taking that into account if they are part of the allowed macros ?

No, at least that wasn’t my intention.

What’s your idea for implementing this? I would find it not a great design if we were to hardcode a list of macros since these macros may not be available/installed, they may not be core macros and they may be extension macros and you can always imagine new dangerous contributed macro that wouldn’t be listed.

Thus having a blacklist is probably not the best. If we want to be safe by default, we probably need a whitelist parameter (in xwiki.properties), initially containing only a few core macros (ie from commons, rendering and platform). Admins would then need to add macros to this list if they want their users to have more macros available.

WDYT?

Yes, that was exactly my proposal. I suggest introducing a property which macros should be allowed in restricted mode. Initially, I wouldn’t include all core macros, just those for which we believe it makes sense to use them in comments (for example, I wouldn’t include the cache, async, include or display macros).

Note that we’re only talking about macros available in restricted mode, i.e., primarily in comments. In all other contexts, still all macros would be available. This proposal is primarily to limit the attack surface that is available for guest users. Further, it improves the usability by limiting the available macros to macros that make sense in comments (we would need a mechanism similar to what has been discussed for macro permissions to also make sure those macros aren’t listed in CKEditor).