CVE-2024-31987: Insufficient Information (NVD-CWE-noinfo)
value: XWiki Platform is a generic wiki platform. Starting in version 6.4-milestone-1 and prior to versions 4.10.19, 15.5.4, and 15.10-rc-1, any user who can edit any page like their profile can create a custom skin with a template override that is executed with programming right, thus allowing remote code execution. This has been patched in XWiki 14.10.19, 15.5.4 and 15.10RC1. No known workarounds are available except for upgrading.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-04-10T21:15Z
Repository: https://github.com/xwiki/xwiki-platform
Language: Java
Files changed: xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/internal/velocity/XWikiVelocityManager.java, xwiki-platform-core/xwiki-platform-oldcore/src/test/java/org/xwiki/internal/velocity/XWikiVelocityManagerTest.java
Commit date: 2023-10-31 11:27:27 +0100
Commit message: XWIKI-21478: Improve rights check of template macros

Diff: diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/internal/velocity/XWikiVelocityManager.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/internal/velocity/XWikiVelocityManager.java
index 07ddaa6f112db..841980cd1b475 100644
--- a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/internal/velocity/XWikiVelocityManager.java
+++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/internal/velocity/XWikiVelocityManager.java
@@ -40,10 +40,13 @@
 import org.xwiki.context.ExecutionContext;
 import org.xwiki.environment.Environment;
 import org.xwiki.observation.EventListener;
 import org.xwiki.observation.ObservationManager;
 import org.xwiki.observation.event.Event;
+import org.xwiki.security.authorization.AuthorExecutor;
+import org.xwiki.security.authorization.AuthorizationManager;
+import org.xwiki.security.authorization.Right;
 import org.xwiki.skin.Skin;
 import org.xwiki.skin.SkinManager;
 import org.xwiki.template.Template;
 import org.xwiki.template.TemplateManager;
 import org.xwiki.template.event.TemplateDeletedEvent;
@@ -71,11 +74,11 @@
 public class XWikiVelocityManager extends DefaultVelocityManager implements Initializable
 {
     private static final String VELOCITYENGINE_CACHEKEY_NAME = "velocity.engine.key";
 
     private static final List<Event> EVENTS =
-        Arrays.<Event>asList(new TemplateUpdatedEvent(), new TemplateDeletedEvent());
+        Arrays.asList(new TemplateUpdatedEvent(), new TemplateDeletedEvent());
 
     /**
      * Used to access the current {@link XWikiContext}.
      */
     @Inject
@@ -94,14 +97,20 @@ public class XWikiVelocityManager extends DefaultVelocityManager implements Init
     private ObservationManager observation;
 
     @Inject
     private Environment environment;
 
+    @Inject
+    private AuthorExecutor authorExecutor;
+
+    @Inject
+    private AuthorizationManager authorizationManager;
+
     @Inject
     private Logger logger;
 
-    private Map<String, VelocityEngine> velocityEngines = new ConcurrentHashMap<>();
+    private final Map<String, VelocityEngine> velocityEngines = new ConcurrentHashMap<>();
 
     @Override
     public void initialize() throws InitializationException
     {
         super.initialize();
@@ -112,11 +121,11 @@ public void initialize() throws InitializationException
             public void onEvent(Event event, Object source, Object data)
             {
                 if (event instanceof TemplateEvent) {
                     TemplateEvent templateEvent = (TemplateEvent) event;
 
-                    velocityEngines.remove(templateEvent.getId());
+                    XWikiVelocityManager.this.velocityEngines.remove(templateEvent.getId());
                 }
             }
 
             @Override
             public String getName()
@@ -175,12 +184,12 @@ private Template getVelocityEngineMacrosTemplate()
         return template;
     }
 
     /**
      * @return the Velocity Engine corresponding to the current execution context. More specifically returns the
-     *         Velocity Engine for the current skin since each skin has its own Velocity Engine so that each skin can
-     *         have global velocimacros defined
+     *     Velocity Engine for the current skin since each skin has its own Velocity Engine so that each skin can have
+     *     global velocimacros defined
      * @throws XWikiVelocityException in case of an error while creating a Velocity Engine
      */
     @Override
     public VelocityEngine getVelocityEngine() throws XWikiVelocityException
     {
@@ -246,13 +255,16 @@ private void injectBaseMacros(VelocityEngine velocityEngine, Template skinMacros
                     velocityEngine.addGlobalMacros(mainMacros.getMacros());
                 }
             }
         }
 
-        // Inject skin macros
-        if (skinMacrosTemplate != null) {
-            VelocityTemplate skinMacros = compile("", new StringReader(skinMacrosTemplate.getContent().getContent()));
-
+        // Inject skin macros if their author has at least Script rights.
+        if (skinMacrosTemplate != null
+            && this.authorizationManager.hasAccess(Right.SCRIPT, skinMacrosTemplate.getContent().getAuthorReference(),
+            skinMacrosTemplate.getContent().getDocumentReference()))
+        {
+            VelocityTemplate skinMacros =
+                compile("", new StringReader(skinMacrosTemplate.getContent().getContent()));
             velocityEngine.addGlobalMacros(skinMacros.getMacros());
         }
     }
 }
diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/org/xwiki/internal/velocity/XWikiVelocityManagerTest.java b/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/org/xwiki/internal/velocity/XWikiVelocityManagerTest.java
index 7ab150ee48947..8815d5b14ce0b 100644
--- a/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/org/xwiki/internal/velocity/XWikiVelocityManagerTest.java
+++ b/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/org/xwiki/internal/velocity/XWikiVelocityManagerTest.java
@@ -20,19 +20,28 @@
 package org.xwiki.internal.velocity;
 
 import org.apache.velocity.VelocityContext;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import org.xwiki.component.manager.ComponentLookupException;
+import org.mockito.Mock;
 import org.xwiki.internal.script.XWikiScriptContextInitializer;
 import org.xwiki.localization.ContextualLocalizationManager;
 import org.xwiki.logging.internal.DefaultLoggerConfiguration;
 import org.xwiki.model.reference.DocumentReference;
+import org.xwiki.properties.ConverterManager;
 import org.xwiki.script.internal.DefaultScriptContextManager;
+import org.xwiki.security.authorization.AuthorizationManager;
+import org.xwiki.security.authorization.Right;
+import org.xwiki.skin.Skin;
+import org.xwiki.skin.SkinManager;
+import org.xwiki.template.Template;
+import org.xwiki.template.TemplateContent;
+import org.xwiki.template.TemplateManager;
 import org.xwiki.test.annotation.ComponentList;
 import org.xwiki.test.junit5.mockito.InjectMockComponents;
 import org.xwiki.test.junit5.mockito.MockComponent;
+import org.xwiki.velocity.VelocityEngine;
 import org.xwiki.velocity.internal.DefaultVelocityConfiguration;
 import org.xwiki.velocity.internal.VelocityExecutionContextInitializer;
 
 import com.xpn.xwiki.api.Document;
 import com.xpn.xwiki.doc.XWikiDocument;
@@ -42,43 +51,83 @@
 
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNotSame;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.mockito.ArgumentMatchers.anyMap;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 /**
  * Validate {@link XWikiVelocityManager}.
- * 
+ *
  * @version $Id$
  */
-@ComponentList(value = {DefaultScriptContextManager.class, XWikiScriptContextInitializer.class,
-    DefaultVelocityConfiguration.class, DefaultLoggerConfiguration.class})
+@ComponentList({
+    DefaultScriptContextManager.class,
+    XWikiScriptContextInitializer.class,
+    DefaultVelocityConfiguration.class,
+    DefaultLoggerConfiguration.class
+})
 @OldcoreTest
 public class XWikiVelocityManagerTest
 {
+    private static final DocumentReference TEMPLATE_DOCUMENT = new DocumentReference("xwiki", "XWiki", "TestMacros");
+
+    private static final DocumentReference SCRIPT_USER = new DocumentReference("xwiki", "XWiki", "Script");
+
     @MockComponent
     private ContextualLocalizationManager localizationManager;
 
     @InjectMockComponents
     private XWikiVelocityManager velocityManager;
 
     @InjectMockitoOldcore
     private MockitoOldcore oldcore;
 
+    @MockComponent
+    private SkinManager skinManager;
+
+    @Mock
+    private Skin skin;
+
+    @MockComponent
+    private TemplateManager templateManager;
+
+    @Mock
+    private Template template;
+
+    @Mock
+    private TemplateContent templateContent;
+
+    @MockComponent
+    private ConverterManager converterManager;
+
     @BeforeEach
-    public void beforeEach() throws ComponentLookupException
+    void beforeEach() throws Exception
     {
         this.oldcore.getExecutionContext().setProperty(VelocityExecutionContextInitializer.VELOCITY_CONTEXT_ID,
             new VelocityContext());
+
+        when(this.skinManager.getCurrentSkin(true)).thenReturn(this.skin);
+        when(this.templateManager.getTemplate("macros.vm")).thenReturn(this.template);
+        when(this.template.getId()).thenReturn("testMacros");
+        when(this.template.getContent()).thenReturn(this.templateContent);
+        when(this.templateContent.getDocumentReference()).thenReturn(TEMPLATE_DOCUMENT);
+        when(this.templateContent.getContent()).thenReturn("");
+
+        AuthorizationManager authorizationManager = this.oldcore.getMockAuthorizationManager();
+        when(authorizationManager.hasAccess(Right.SCRIPT, SCRIPT_USER, TEMPLATE_DOCUMENT)).thenReturn(true);
     }
 
     // Tests
 
     @Test
-    public void getVelocityContext()
+    void getVelocityContext()
     {
-        VelocityContext context = velocityManager.getVelocityContext();
+        VelocityContext context = this.velocityManager.getVelocityContext();
 
         assertNull(context.get("doc"));
         assertNull(context.get("sdoc"));
 
         DocumentReference docReference = new DocumentReference("wiki", "space", "doc");
@@ -110,6 +159,21 @@ public void getVelocityContext()
         assertNotNull(context.get("doc"));
         assertNotSame(doc, context.get("doc"));
         assertNotNull(context.get("sdoc"));
         assertNotSame(sdoc, context.get("sdoc"));
     }
+
+    @Test
+    void checkMacrosInjectionWithoutScriptRights() throws Exception
+    {
+        VelocityEngine engine = this.velocityManager.getVelocityEngine();
+        verify(engine, never()).addGlobalMacros(anyMap());
+    }
+
+    @Test
+    void checkMacrosInjectionWithScriptRights() throws Exception
+    {
+        when(this.templateContent.getAuthorReference()).thenReturn(SCRIPT_USER);
+        VelocityEngine engine = this.velocityManager.getVelocityEngine();
+        verify(engine).addGlobalMacros(anyMap());
+    }
 }

Vulnerable code:         Arrays.<Event>asList(new TemplateUpdatedEvent(), new TemplateDeletedEvent());
    private Map<String, VelocityEngine> velocityEngines = new ConcurrentHashMap<>();
                    velocityEngines.remove(templateEvent.getId());
     *         Velocity Engine for the current skin since each skin has its own Velocity Engine so that each skin can
     *         have global velocimacros defined
        // Inject skin macros
        if (skinMacrosTemplate != null) {
            VelocityTemplate skinMacros = compile("", new StringReader(skinMacrosTemplate.getContent().getContent()));

import org.xwiki.component.manager.ComponentLookupException;
 * 
@ComponentList(value = {DefaultScriptContextManager.class, XWikiScriptContextInitializer.class,
    DefaultVelocityConfiguration.class, DefaultLoggerConfiguration.class})
    public void beforeEach() throws ComponentLookupException
    public void getVelocityContext()
        VelocityContext context = velocityManager.getVelocityContext();
Fixed code: import org.xwiki.security.authorization.AuthorExecutor;
import org.xwiki.security.authorization.AuthorizationManager;
import org.xwiki.security.authorization.Right;
        Arrays.asList(new TemplateUpdatedEvent(), new TemplateDeletedEvent());
    @Inject
    private AuthorExecutor authorExecutor;

    @Inject
    private AuthorizationManager authorizationManager;

    private final Map<String, VelocityEngine> velocityEngines = new ConcurrentHashMap<>();
                    XWikiVelocityManager.this.velocityEngines.remove(templateEvent.getId());
     *     Velocity Engine for the current skin since each skin has its own Velocity Engine so that each skin can have
     *     global velocimacros defined
        // Inject skin macros if their author has at least Script rights.
        if (skinMacrosTemplate != null
            && this.authorizationManager.hasAccess(Right.SCRIPT, skinMacrosTemplate.getContent().getAuthorReference(),
            skinMacrosTemplate.getContent().getDocumentReference()))
        {
            VelocityTemplate skinMacros =
                compile("", new StringReader(skinMacrosTemplate.getContent().getContent()));
import org.mockito.Mock;
import org.xwiki.properties.ConverterManager;
import org.xwiki.security.authorization.AuthorizationManager;
import org.xwiki.security.authorization.Right;
import org.xwiki.skin.Skin;
import org.xwiki.skin.SkinManager;
import org.xwiki.template.Template;
import org.xwiki.template.TemplateContent;
import org.xwiki.template.TemplateManager;
import org.xwiki.velocity.VelocityEngine;
import static org.mockito.ArgumentMatchers.anyMap;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
 *
@ComponentList({
    DefaultScriptContextManager.class,
    XWikiScriptContextInitializer.class,
    DefaultVelocityConfiguration.class,
    DefaultLoggerConfiguration.class
})
    private static final DocumentReference TEMPLATE_DOCUMENT = new DocumentReference("xwiki", "XWiki", "TestMacros");

    private static final DocumentReference SCRIPT_USER = new DocumentReference("xwiki", "XWiki", "Script");

    @MockComponent
    private SkinManager skinManager;

    @Mock
    private Skin skin;

    @MockComponent
    private TemplateManager templateManager;

    @Mock
    private Template template;

    @Mock
    private TemplateContent templateContent;

    @MockComponent
    private ConverterManager converterManager;

    void beforeEach() throws Exception

        when(this.skinManager.getCurrentSkin(true)).thenReturn(this.skin);
        when(this.templateManager.getTemplate("macros.vm")).thenReturn(this.template);
        when(this.template.getId()).thenReturn("testMacros");
        when(this.template.getContent()).thenReturn(this.templateContent);
        when(this.templateContent.getDocumentReference()).thenReturn(TEMPLATE_DOCUMENT);
        when(this.templateContent.getContent()).thenReturn("");

        AuthorizationManager authorizationManager = this.oldcore.getMockAuthorizationManager();
        when(authorizationManager.hasAccess(Right.SCRIPT, SCRIPT_USER, TEMPLATE_DOCUMENT)).thenReturn(true);
    void getVelocityContext()
        VelocityContext context = this.velocityManager.getVelocityContext();

    @Test
    void checkMacrosInjectionWithoutScriptRights() throws Exception
    {
        VelocityEngine engine = this.velocityManager.getVelocityEngine();
        verify(engine, never()).addGlobalMacros(anyMap());
    }

    @Test
    void checkMacrosInjectionWithScriptRights() throws Exception
    {
        when(this.templateContent.getAuthorReference()).thenReturn(SCRIPT_USER);
        VelocityEngine engine = this.velocityManager.getVelocityEngine();
        verify(engine).addGlobalMacros(anyMap());
    }