CVE-2023-37910: Missing Authorization (CWE-862)
value: XWiki Platform is a generic wiki platform offering runtime services for applications built on top of it. Starting with the introduction of attachment move support in version 14.0-rc-1 and prior to versions 14.4.8, 14.10.4, and 15.0-rc-1, an attacker with edit access on any document (can be the user profile which is editable by default) can move any attachment of any other document to this attacker-controlled document. This allows the attacker to access and possibly publish any attachment of which the name is known, regardless if the attacker has view or edit rights on the source document of this attachment. Further, the attachment is deleted from the source document. This vulnerability has been patched in XWiki 14.4.8, 14.10.4, and 15.0 RC1. There is no workaround apart from upgrading to a fixed version.
CWE description: The product does not perform an authorization check when an actor attempts to access a resource or perform an action.
CVSS v3 base score: 8.1
Published: 2023-10-25T18:17Z
Repository: https://github.com/xwiki/xwiki-platform
Language: Java
Files changed: xwiki-platform-core/xwiki-platform-attachment/xwiki-platform-attachment-api/src/main/java/org/xwiki/attachment/internal/refactoring/job/MoveAttachmentJob.java, xwiki-platform-core/xwiki-platform-attachment/xwiki-platform-attachment-api/src/test/java/org/xwiki/attachment/internal/refactoring/job/MoveAttachmentJobTest.java
Commit date: 2023-01-11 16:27:40 +0100
Commit message: XWIKI-20334: Add rights check to MoveAttachmentJob

Diff: diff --git a/xwiki-platform-core/xwiki-platform-attachment/xwiki-platform-attachment-api/src/main/java/org/xwiki/attachment/internal/refactoring/job/MoveAttachmentJob.java b/xwiki-platform-core/xwiki-platform-attachment/xwiki-platform-attachment-api/src/main/java/org/xwiki/attachment/internal/refactoring/job/MoveAttachmentJob.java
index 72099290abaa9..6c297d0a5ea6b 100644
--- a/xwiki-platform-core/xwiki-platform-attachment/xwiki-platform-attachment-api/src/main/java/org/xwiki/attachment/internal/refactoring/job/MoveAttachmentJob.java
+++ b/xwiki-platform-core/xwiki-platform-attachment/xwiki-platform-attachment-api/src/main/java/org/xwiki/attachment/internal/refactoring/job/MoveAttachmentJob.java
@@ -36,10 +36,11 @@
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.model.reference.EntityReference;
 import org.xwiki.model.reference.EntityReferenceSerializer;
 import org.xwiki.refactoring.internal.job.AbstractEntityJob;
 import org.xwiki.refactoring.job.EntityJobStatus;
+import org.xwiki.security.authorization.Right;
 import org.xwiki.user.UserReference;
 import org.xwiki.user.UserReferenceResolver;
 
 import com.xpn.xwiki.XWiki;
 import com.xpn.xwiki.XWikiContext;
@@ -104,23 +105,45 @@ protected void process(EntityReference source)
         XWiki wiki = this.xcontextProvider.get().getWiki();
 
         // Update the author for the attribution of the attachment uploader.
         this.modelBridge.setContextUserReference(this.request.getUserReference());
         try {
-            this.progressManager.startStep(this);
-            moveAttachment(source, destination, autoRedirect, wiki);
-            this.progressManager.endStep(this);
-
-            this.progressManager.startStep(this);
-            this.observationManager.notify(new AttachmentMovedEvent((AttachmentReference) source, destination), this,
-                this.request);
-            this.progressManager.endStep(this);
+            if (checkMoveRights(source, destination)) {
+                this.progressManager.startStep(this);
+                moveAttachment(source, destination, autoRedirect, wiki);
+                this.progressManager.endStep(this);
+
+                this.progressManager.startStep(this);
+                this.observationManager.notify(new AttachmentMovedEvent((AttachmentReference) source, destination),
+                    this,
+                    this.request);
+                this.progressManager.endStep(this);
+            }
         } finally {
             this.progressManager.popLevelProgress(this);
         }
     }
 
+    private boolean checkMoveRights(EntityReference source, EntityReference destination)
+    {
+        // Check view and edit right on source to ensure that the user doesn't get view right through the move.
+        // While edit right implies view right, the view right might be explicitly denied.
+        boolean hasSourceRight = hasAccess(Right.VIEW, source) && hasAccess(Right.EDIT, source);
+        boolean hasDestinationRight = hasAccess(Right.EDIT, destination);
+
+        if (!hasSourceRight) {
+            this.logger.error("You don't have sufficient permissions over the source attachment [{}].", source);
+        } else if (!hasDestinationRight) {
+            // The destination's document might be the same as the source, therefore, only log the error when there is
+            // no error regarding the source.
+            this.logger.error("You don't have sufficient permissions over the destination attachment [{}].",
+                destination);
+        }
+
+        return hasSourceRight && hasDestinationRight;
+    }
+
     private void moveAttachment(EntityReference source, AttachmentReference destination, boolean autoRedirect,
         XWiki wiki)
     {
         try {
             XWikiDocument sourceDocument = wiki.getDocument(source.getParent(), this.xcontextProvider.get());
diff --git a/xwiki-platform-core/xwiki-platform-attachment/xwiki-platform-attachment-api/src/test/java/org/xwiki/attachment/internal/refactoring/job/MoveAttachmentJobTest.java b/xwiki-platform-core/xwiki-platform-attachment/xwiki-platform-attachment-api/src/test/java/org/xwiki/attachment/internal/refactoring/job/MoveAttachmentJobTest.java
index e2ff172056af6..05ed6bc9823d6 100644
--- a/xwiki-platform-core/xwiki-platform-attachment/xwiki-platform-attachment-api/src/test/java/org/xwiki/attachment/internal/refactoring/job/MoveAttachmentJobTest.java
+++ b/xwiki-platform-core/xwiki-platform-attachment/xwiki-platform-attachment-api/src/test/java/org/xwiki/attachment/internal/refactoring/job/MoveAttachmentJobTest.java
@@ -23,10 +23,12 @@
 import javax.inject.Provider;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.mockito.Mock;
 import org.xwiki.attachment.internal.AttachmentsManager;
 import org.xwiki.attachment.internal.RedirectAttachmentClassDocumentInitializer;
 import org.xwiki.attachment.refactoring.MoveAttachmentRequest;
 import org.xwiki.localization.ContextualLocalizationManager;
@@ -34,10 +36,12 @@
 import org.xwiki.model.reference.AttachmentReference;
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.model.reference.EntityReference;
 import org.xwiki.model.reference.EntityReferenceSerializer;
 import org.xwiki.refactoring.internal.ModelBridge;
+import org.xwiki.security.authorization.AuthorizationManager;
+import org.xwiki.security.authorization.Right;
 import org.xwiki.test.LogLevel;
 import org.xwiki.test.junit5.LogCaptureExtension;
 import org.xwiki.test.junit5.mockito.ComponentTest;
 import org.xwiki.test.junit5.mockito.InjectMockComponents;
 import org.xwiki.test.junit5.mockito.MockComponent;
@@ -52,12 +56,15 @@
 
 import ch.qos.logback.classic.Level;
 
 import static java.util.Collections.singletonList;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
 
@@ -75,10 +82,12 @@
     private static final AttachmentReference SOURCE_ATTACHMENT_LOCATION =
         new AttachmentReference("oldName", SOURCE_LOCATION);
 
     private static final DocumentReference AUTHOR_REFERENCE = new DocumentReference("xwiki", "XWiki", "User1");
 
+    private static final DocumentReference USER2_REFERENCE = new DocumentReference("xwiki", "XWiki", "User2");
+
     private static final DocumentReference TARGET_LOCATION = new DocumentReference("xwiki", "Space", "Target");
 
     private static final AttachmentReference TARGET_ATTACHMENT_LOCATION =
         new AttachmentReference("newName", TARGET_LOCATION);
 
@@ -99,16 +108,19 @@
 
     @MockComponent
     private AttachmentsManager attachmentsManager;
 
     @MockComponent
-    protected ModelBridge modelBridge;
+    private ModelBridge modelBridge;
 
     @MockComponent
     @Named("document")
     private UserReferenceResolver<DocumentReference> documentReferenceUserReferenceResolver;
 
+    @MockComponent
+    private AuthorizationManager authorizationManager;
+
     @Mock
     private XWikiContext context;
 
     @Mock
     private XWiki wiki;
@@ -127,11 +139,11 @@
 
     @Mock
     private UserReference authorReference;
 
     @RegisterExtension
-    LogCaptureExtension logCapture = new LogCaptureExtension(LogLevel.WARN);
+    private LogCaptureExtension logCapture = new LogCaptureExtension(LogLevel.WARN);
 
     private MoveAttachmentRequest request;
 
     @BeforeEach
     void setUp() throws Exception
@@ -151,10 +163,16 @@ void setUp() throws Exception
         when(this.referenceSerializer.serialize(TARGET_ATTACHMENT_LOCATION)).thenReturn("xwiki:Space.Target@newName");
         when(this.referenceSerializer.serialize(SOURCE_ATTACHMENT_LOCATION)).thenReturn("xwiki:Space.Source@oldName");
         when(this.referenceSerializer.serialize(TARGET_LOCATION)).thenReturn("xwiki:Space.Target");
         when(this.referenceSerializer.serialize(SOURCE_LOCATION)).thenReturn("xwiki:Space.Source");
         when(this.documentReferenceUserReferenceResolver.resolve(AUTHOR_REFERENCE)).thenReturn(this.authorReference);
+
+        // Grant global view and edit right.
+        when(this.authorizationManager.hasAccess(eq(Right.VIEW), eq(AUTHOR_REFERENCE), any(AttachmentReference.class)))
+            .thenReturn(true);
+        when(this.authorizationManager.hasAccess(eq(Right.EDIT), eq(AUTHOR_REFERENCE), any(AttachmentReference.class)))
+            .thenReturn(true);
     }
 
     @Test
     void process() throws Exception
     {
@@ -162,10 +180,11 @@ void process() throws Exception
         this.request.setEntityReferences(singletonList(SOURCE_ATTACHMENT_LOCATION));
         this.request.setProperty(MoveAttachmentRequest.DESTINATION, TARGET_ATTACHMENT_LOCATION);
         this.request.setProperty(MoveAttachmentRequest.AUTO_REDIRECT, true);
         this.request.setInteractive(false);
         this.request.setUserReference(AUTHOR_REFERENCE);
+        this.request.setAuthorReference(AUTHOR_REFERENCE);
 
         XWikiAttachment sourceAttachment = mock(XWikiAttachment.class);
         XWikiAttachment targetAttachment = mock(XWikiAttachment.class);
 
         when(sourceAttachment.getFilename()).thenReturn("oldName");
@@ -202,10 +221,12 @@ void processTargetSaveFail() throws Exception
         // Request initialization.
         this.request.setEntityReferences(singletonList(SOURCE_ATTACHMENT_LOCATION));
         this.request.setProperty(MoveAttachmentRequest.DESTINATION, TARGET_ATTACHMENT_LOCATION);
         this.request.setProperty(MoveAttachmentRequest.AUTO_REDIRECT, true);
         this.request.setInteractive(false);
+        this.request.setUserReference(AUTHOR_REFERENCE);
+        this.request.setAuthorReference(AUTHOR_REFERENCE);
 
         XWikiAttachment sourceAttachment = mock(XWikiAttachment.class);
         XWikiAttachment targetAttachment = mock(XWikiAttachment.class);
         XWikiAttachment newSourceAttachment = mock(XWikiAttachment.class);
         when(sourceAttachment.getFilename()).thenReturn("oldName");
@@ -255,10 +276,11 @@ void processRename() throws Exception
         this.request.setProperty(MoveAttachmentRequest.DESTINATION,
             new AttachmentReference("newName", SOURCE_LOCATION));
         this.request.setProperty(MoveAttachmentRequest.AUTO_REDIRECT, true);
         this.request.setInteractive(false);
         this.request.setUserReference(AUTHOR_REFERENCE);
+        this.request.setAuthorReference(AUTHOR_REFERENCE);
 
         XWikiAttachment sourceAttachment = mock(XWikiAttachment.class);
         XWikiAttachment targetAttachment = mock(XWikiAttachment.class);
         when(sourceAttachment.getFilename()).thenReturn("oldName");
         when(this.sourceDocument.getExactAttachment("oldName")).thenReturn(sourceAttachment);
@@ -280,6 +302,47 @@ void processRename() throws Exception
             this.context);
         verify(this.modelBridge).setContextUserReference(AUTHOR_REFERENCE);
         verify(this.sourceAuthors, times(2)).setEffectiveMetadataAuthor(this.authorReference);
         verify(this.sourceAuthors, times(2)).setOriginalMetadataAuthor(this.authorReference);
     }
+
+    @ParameterizedTest
+    @CsvSource({
+        "true, true",
+        "false, true",
+        "true, false",
+        "false, false"
+    })
+    void failWithoutRights(boolean canView, boolean canEdit) throws Exception
+    {
+        // Request initialization.
+        this.request.setEntityReferences(singletonList(SOURCE_ATTACHMENT_LOCATION));
+        this.request.setProperty(MoveAttachmentRequest.DESTINATION, TARGET_ATTACHMENT_LOCATION);
+        this.request.setProperty(MoveAttachmentRequest.AUTO_REDIRECT, true);
+        this.request.setInteractive(false);
+        this.request.setUserReference(USER2_REFERENCE);
+        this.request.setAuthorReference(AUTHOR_REFERENCE);
+
+        when(this.authorizationManager.hasAccess(Right.EDIT, USER2_REFERENCE, SOURCE_ATTACHMENT_LOCATION))
+            .thenReturn(canEdit);
+        when(this.authorizationManager.hasAccess(Right.VIEW, USER2_REFERENCE, SOURCE_ATTACHMENT_LOCATION))
+            .thenReturn(canView);
+        when(this.authorizationManager.hasAccess(Right.EDIT, USER2_REFERENCE, TARGET_ATTACHMENT_LOCATION))
+            .thenReturn(false);
+
+        this.job.process(SOURCE_ATTACHMENT_LOCATION);
+
+        // Verify nothing has been modified.
+        verifyNoInteractions(this.sourceDocument, this.targetDocument);
+        verifyNoInteractions(this.attachmentsManager);
+        verify(this.wiki, never()).saveDocument(any(), any(), any());
+        verifyNoInteractions(this.targetAuthors, this.sourceAuthors);
+
+        if (!canEdit || !canView) {
+            assertEquals("You don't have sufficient permissions over the source attachment "
+                + "[Attachment xwiki:Space.Source@oldName].", this.logCapture.getMessage(0));
+        } else {
+            assertEquals("You don't have sufficient permissions over the destination attachment "
+                + "[Attachment xwiki:Space.Target@newName].", this.logCapture.getMessage(0));
+        }
+    }
 }

Vulnerable code:             this.progressManager.startStep(this);
            moveAttachment(source, destination, autoRedirect, wiki);
            this.progressManager.endStep(this);

            this.progressManager.startStep(this);
            this.observationManager.notify(new AttachmentMovedEvent((AttachmentReference) source, destination), this,
                this.request);
            this.progressManager.endStep(this);
    protected ModelBridge modelBridge;
    LogCaptureExtension logCapture = new LogCaptureExtension(LogLevel.WARN);
Fixed code: import org.xwiki.security.authorization.Right;
            if (checkMoveRights(source, destination)) {
                this.progressManager.startStep(this);
                moveAttachment(source, destination, autoRedirect, wiki);
                this.progressManager.endStep(this);

                this.progressManager.startStep(this);
                this.observationManager.notify(new AttachmentMovedEvent((AttachmentReference) source, destination),
                    this,
                    this.request);
                this.progressManager.endStep(this);
            }
    private boolean checkMoveRights(EntityReference source, EntityReference destination)
    {
        // Check view and edit right on source to ensure that the user doesn't get view right through the move.
        // While edit right implies view right, the view right might be explicitly denied.
        boolean hasSourceRight = hasAccess(Right.VIEW, source) && hasAccess(Right.EDIT, source);
        boolean hasDestinationRight = hasAccess(Right.EDIT, destination);

        if (!hasSourceRight) {
            this.logger.error("You don't have sufficient permissions over the source attachment [{}].", source);
        } else if (!hasDestinationRight) {
            // The destination's document might be the same as the source, therefore, only log the error when there is
            // no error regarding the source.
            this.logger.error("You don't have sufficient permissions over the destination attachment [{}].",
                destination);
        }

        return hasSourceRight && hasDestinationRight;
    }

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.xwiki.security.authorization.AuthorizationManager;
import org.xwiki.security.authorization.Right;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.never;
    private static final DocumentReference USER2_REFERENCE = new DocumentReference("xwiki", "XWiki", "User2");

    private ModelBridge modelBridge;
    @MockComponent
    private AuthorizationManager authorizationManager;

    private LogCaptureExtension logCapture = new LogCaptureExtension(LogLevel.WARN);

        // Grant global view and edit right.
        when(this.authorizationManager.hasAccess(eq(Right.VIEW), eq(AUTHOR_REFERENCE), any(AttachmentReference.class)))
            .thenReturn(true);
        when(this.authorizationManager.hasAccess(eq(Right.EDIT), eq(AUTHOR_REFERENCE), any(AttachmentReference.class)))
            .thenReturn(true);
        this.request.setAuthorReference(AUTHOR_REFERENCE);
        this.request.setUserReference(AUTHOR_REFERENCE);
        this.request.setAuthorReference(AUTHOR_REFERENCE);
        this.request.setAuthorReference(AUTHOR_REFERENCE);

    @ParameterizedTest
    @CsvSource({
        "true, true",
        "false, true",
        "true, false",
        "false, false"
    })
    void failWithoutRights(boolean canView, boolean canEdit) throws Exception
    {
        // Request initialization.
        this.request.setEntityReferences(singletonList(SOURCE_ATTACHMENT_LOCATION));
        this.request.setProperty(MoveAttachmentRequest.DESTINATION, TARGET_ATTACHMENT_LOCATION);
        this.request.setProperty(MoveAttachmentRequest.AUTO_REDIRECT, true);
        this.request.setInteractive(false);
        this.request.setUserReference(USER2_REFERENCE);
        this.request.setAuthorReference(AUTHOR_REFERENCE);

        when(this.authorizationManager.hasAccess(Right.EDIT, USER2_REFERENCE, SOURCE_ATTACHMENT_LOCATION))
            .thenReturn(canEdit);
        when(this.authorizationManager.hasAccess(Right.VIEW, USER2_REFERENCE, SOURCE_ATTACHMENT_LOCATION))
            .thenReturn(canView);
        when(this.authorizationManager.hasAccess(Right.EDIT, USER2_REFERENCE, TARGET_ATTACHMENT_LOCATION))
            .thenReturn(false);

        this.job.process(SOURCE_ATTACHMENT_LOCATION);

        // Verify nothing has been modified.
        verifyNoInteractions(this.sourceDocument, this.targetDocument);
        verifyNoInteractions(this.attachmentsManager);
        verify(this.wiki, never()).saveDocument(any(), any(), any());
        verifyNoInteractions(this.targetAuthors, this.sourceAuthors);

        if (!canEdit || !canView) {
            assertEquals("You don't have sufficient permissions over the source attachment "
                + "[Attachment xwiki:Space.Source@oldName].", this.logCapture.getMessage(0));
        } else {
            assertEquals("You don't have sufficient permissions over the destination attachment "
                + "[Attachment xwiki:Space.Target@newName].", this.logCapture.getMessage(0));
        }
    }