Details
-
Task
-
Resolution: Unresolved
-
Critical
-
None
-
4.0, 4.1-milestone-2
-
None
-
security
-
Low
-
Hard
-
Description
The basic idea behind this branch is to not rely on "the context document" for validating programming rights. Instead, we push a security item to a security stack as late as possible before displaying any content. (During displaying, the transformations are executed, which is essentially user code.) This means that the responsibility for pushing this security item falls on the Displayer clases, and the XWikiRenderingEngine (for syntax/1.0).
Here is my analysis of the problem that is the basis for this refactoring:
Programming rights is assigned to the content author. The content author is the user who is the author of the content document. This basic model is sane, but we are missing two very important details in validating the content author:
- We do not ensure that the content that is rendered is actually extracted from the content document.
- We do not ensure that the content author is in fact the author of the content in the content document.
I have factored out the information that the right service bases its decisions on into a separate object called the authorization context, which is declared in the execution context. The authorization context is in itself a read-only object. However, various aspects of the state of the authorization context can be modified via controller components that can be obtained from the component manager:
- EffectiveUserController - set the user on which access rights will be resolved
- ContentAuthorController - push and pop content documents, from which the content author will be resolved
- PrivilegedModeController - disable and restore the 'privileged' mode, in which programming rights at all will be granted
Pushing a content document is done immediately prior to rendering content. That means in the Displayers and in the RenderingEngine. It is the callers to the display and renderText method that are responsible for setting the context document appropriately. Here one must take great care to ensure that the content that is about to be rendered is in fact extracted from the content document. If the content cannot be associated with any content document, null MUST be pushed.
Pushing null as content document means that the content about to be rendered have not been verified as originating from any content document.
If there is a content document at the top of the stack, a content author can be resolved from it. A number of conditions must be met before accepting a content author, however:
- The content document MUST NOT be new. The content author is validated when saving the document, so the document must have been saved to ensure that the content author is in fact correct.
- The content document MUST NOT be marked 'content dirty'. This ensures that the contents of the document have not been updated after the document was loaded.
- The content document MUST NOT be marked 'metadata dirty'. This ensures that the metadata of the document have not been updated after the document was loaded.
If any one of these conditions doesn't hold, we will reject the content author. Condition 1. and 3. are maybe not strictly necessary, as the API should not allow unprivileged code to tamper with the document metadata. But I have choosen to be conservative and include these conditions as well.
In addition to this, I have added a configuration option to refuse the content author, unless the document have been marked with an object of type RequiredRightClass with level "programming". I strongly recommend making this the default soon.
I have kept the methods 'dropPrivileges' but it should no longer be necessary to actually call XWikiContext.dropPrivileges. Authors of PR-scripts may want to use Document.dropPrivileges, though.
I noticed that the wiki-macro reinstated the potential PR-privileges when executing wiki-macros. As this violates the contract of 'dropPrivileges' I removed this reinstantiation, so wiki-macros may no longer execute with PR after 'dropPrivileges'.
The fatally flawed API-method XWiki.renderText(String, Document) will also run with disabled privileges, unless programming rights were already active when it was called.
Included documents and wiki macros will have the content author set appropriately to the content author of the included document and the document that declares the wiki macro respectively.
I have confirmed that this fixes these critical issues: XWIKI-7941, XWIKI-6844, XWIKI-6760, XWIKI-6671, and XWIKI-5477, XWIKI-5027.
Attachments
Issue Links
- blocks
-
XWIKI-6740 PR leak in Activity Stream
- Closed
-
XWIKI-6844 PR leak in Document#display(), exploitable by any registered user with 4 lines of velocity.
- Closed
-
XWIKI-5255 PR leak using #includeTopic
- Closed
-
XWIKI-7941 PR leak in sheets when displaying TextArea properties
- Closed
- is related to
-
XWIKI-5027 In Syntax xwiki/2.x, programming rights may be inherited by inclusion which may leads to security issues
- Closed
-
XWIKI-6671 Translated document gets the PR rights of the original document
- Closed