XWIKI-20386: CSRF privilege escalation/RCE via the edit action

Author: tmortagne

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/EditAction.java

@@ -29,6 +29,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.xwiki.component.annotation.Component;
+import org.xwiki.csrf.CSRFToken;
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.rendering.syntax.Syntax;
 import org.xwiki.user.UserReference;
@@ -58,6 +59,9 @@ public class EditAction extends XWikiAction
     @Named("document")
     private UserReferenceResolver<DocumentReference> documentReferenceUserReferenceResolver;
 
+    @Inject
+    private CSRFToken csrf;
+
     /**
      * Default constructor.
      */
@@ -116,19 +120,29 @@ protected XWikiDocument prepareEditedDocument(XWikiContext context) throws XWiki
         updateDocumentTitleAndContentFromRequest(editedDocument, context);
         editedDocument.readAddedUpdatedAndRemovedObjectsFromForm(editForm, context);
 
-        // If the metadata is modified, modify the effective metadata author
-        if (editedDocument.isMetaDataDirty()) {
-            UserReference userReference =
-                this.documentReferenceUserReferenceResolver.resolve(context.getUserReference());
-            editedDocument.getAuthors().setEffectiveMetadataAuthor(userReference);
-            editedDocument.getAuthors().setOriginalMetadataAuthor(userReference);
-        }
+        // Check if the document in modified
+        if (editedDocument.isMetaDataDirty() || editedDocument.isContentDirty()) {
+            // If the document is modified make sure a valid CSRF is provided
+            String token = context.getRequest().getParameter("form_token");
+            if (!this.csrf.isTokenValid(token)) {
+                // or make the document restricted
+                editedDocument.setRestricted(true);
+            }
 
-        // If the content is modified, modify the content author
-        if (editedDocument.isContentDirty()) {
-            UserReference userReference =
-                this.documentReferenceUserReferenceResolver.resolve(context.getUserReference());
-            editedDocument.getAuthors().setContentAuthor(userReference);
+            // If the metadata is modified, modify the effective metadata author
+            if (editedDocument.isMetaDataDirty()) {
+                UserReference userReference =
+                    this.documentReferenceUserReferenceResolver.resolve(context.getUserReference());
+                editedDocument.getAuthors().setEffectiveMetadataAuthor(userReference);
+                editedDocument.getAuthors().setOriginalMetadataAuthor(userReference);
+            }
+
+            // If the content is modified, modify the content author
+            if (editedDocument.isContentDirty()) {
+                UserReference userReference =
+                    this.documentReferenceUserReferenceResolver.resolve(context.getUserReference());
+                editedDocument.getAuthors().setContentAuthor(userReference);
+            }
         }
 
         // Set the current user as creator, author and contentAuthor when the edited document is newly created to avoid

xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/web/EditActionTest.java

@@ -19,11 +19,14 @@
  */
 package com.xpn.xwiki.web;
 
+import java.util.Map;
+
 import javax.inject.Named;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mock;
+import org.xwiki.csrf.CSRFToken;
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.store.TemporaryAttachmentSessionsManager;
 import org.xwiki.test.junit5.mockito.InjectMockComponents;
@@ -39,6 +42,7 @@
 import com.xpn.xwiki.test.junit5.mockito.OldcoreTest;
 import com.xpn.xwiki.test.reference.ReferenceComponentList;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -75,6 +79,9 @@ public class EditActionTest
     @Named("document")
     private UserReferenceSerializer<DocumentReference> documentReferenceUserReferenceSerializer;
 
+    @MockComponent
+    private CSRFToken csrf;
+
     @Mock
     private XWikiRequest request;
 
@@ -85,6 +92,9 @@ public void beforeEach()
         when(this.documentReferenceUserReferenceSerializer.serialize(USER_REFERENCE)).thenReturn(USER_DOCUMENT_REFERENCE);
 
         this.oldcore.getXWikiContext().setUserReference(USER_DOCUMENT_REFERENCE);
+
+        this.oldcore.getXWikiContext().setRequest(new XWikiServletRequestStub.Builder().
+            setRequestParameters(Map.of("form_token", new String[] {"tokenvalue"})).build());
     }
 
     private String initAndRenderAction() throws XWikiException
@@ -141,7 +151,18 @@ void documentAuthorsWhenDocumentExist() throws XWikiException
     }
 
     @Test
-    void documentAuthorsWhenDocumentExistAndContentIsModified() throws XWikiException
+    void documentAuthorsWhenDocumentExistAndContentIsModifiedAndInvalidValidCSRF() throws XWikiException
+    {
+        documentAuthorsWhenDocumentExistAndContentIsModified(false);
+    }
+
+    @Test
+    void documentAuthorsWhenDocumentExistAndContentIsModifiedAndValidCSRF() throws XWikiException
+    {
+        documentAuthorsWhenDocumentExistAndContentIsModified(true);
+    }
+
+    void documentAuthorsWhenDocumentExistAndContentIsModified(boolean validToken) throws XWikiException
     {
         XWikiDocument document = this.oldcore.getSpyXWiki().getDocument(new DocumentReference("wiki", "space", "page"),
             this.oldcore.getXWikiContext());
@@ -158,6 +179,8 @@ void documentAuthorsWhenDocumentExistAndContentIsModified() throws XWikiExceptio
 
         when(this.request.getParameter("content")).thenReturn("modified content");
 
+        when(this.csrf.isTokenValid("tokenvalue")).thenReturn(validToken);
+
         initAndRenderAction();
 
         document = this.oldcore.getXWikiContext().getDoc();
@@ -166,5 +189,6 @@ void documentAuthorsWhenDocumentExistAndContentIsModified() throws XWikiExceptio
         assertSame(OTHERUSER_REFERENCE, document.getAuthors().getCreator());
         assertSame(OTHERUSER_REFERENCE, document.getAuthors().getEffectiveMetadataAuthor());
         assertSame(OTHERUSER_REFERENCE, document.getAuthors().getOriginalMetadataAuthor());
+        assertEquals(!validToken, document.isRestricted());
     }
 }