JSPWIKI-1297 see jira for details

jspwiki-main/src/main/java/org/apache/wiki/plugin/Image.java

@@ -32,6 +32,7 @@
 
 import java.util.Map;
 import java.util.ResourceBundle;
+import java.util.Set;
 
 
 /**
@@ -84,6 +85,16 @@
     /** The parameter name for setting the title.  Value is <tt>{@value}</tt>. */
     public static final String PARAM_TITLE    = "title";
 
+    /** Purely presentational CSS properties the <tt>style</tt> parameter may set. Repositioning and stacking
+        properties (position, z-index, transform, inset, ...) are deliberately absent, so an author cannot lift
+        the rendered box out of the page flow and overlay other content with it (in-wiki phishing overlays). */
+    private static final Set< String > SAFE_STYLE_PROPERTIES = Set.of(
+            "background", "background-color", "border", "border-color", "border-radius", "border-style",
+            "border-width", "color", "display", "float", "font-family", "font-size", "font-style", "font-weight",
+            "height", "margin", "margin-bottom", "margin-left", "margin-right", "margin-top", "max-height",
+            "max-width", "min-height", "min-width", "opacity", "padding", "padding-bottom", "padding-left",
+            "padding-right", "padding-top", "text-align", "vertical-align", "white-space", "width" );
+
     /**
      *  This method is used to clean away things like quotation marks which
      *  a malicious user could use to stop processing and insert javascript.
@@ -103,6 +114,47 @@
         return  "Image src='{image.jpg}'";
     }
 
+    /**
+     *  Accepts an author-supplied style only if every declaration sets an allow-listed presentational property.
+     *  CSS escapes and functions (backslash, parentheses) and negative lengths are rejected outright: they could
+     *  disguise a forbidden property (<tt>\\70 osition</tt>), smuggle a URL (<tt>url(...)</tt>), or drag the
+     *  rendered box over other page content (<tt>margin-top:-9999px</tt>).
+     *
+     *  @param style the author-supplied style parameter, already entity-encoded
+     *  @return true if every declaration is allow-listed
+     */
+    static boolean isSafeStyle( final String style ) {
+        if( style.indexOf( '\\' ) >= 0 || style.indexOf( '(' ) >= 0 ) {
+            return false;
+        }
+        for( final String declaration : style.split( ";" ) ) {
+            if( declaration.trim().isEmpty() ) {
+                continue;
+            }
+            final int colon = declaration.indexOf( ':' );
+            if( colon < 0 ) {
+                return false;
+            }
+            final String property = declaration.substring( 0, colon ).trim().toLowerCase( Locale.ENGLISH );
+            final String value = declaration.substring( colon + 1 );
+            if( !SAFE_STYLE_PROPERTIES.contains( property ) || value.matches( ".*-\\s*\\.?\\d.*" ) ) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    /**
+     *  Restricts the <tt>class</tt> parameter to a whitespace-separated list of CSS identifiers, so it cannot
+     *  carry CSS or markup fragments.
+     *
+     *  @param cssclass the author-supplied class parameter
+     *  @return true if it is a plain list of identifiers
+     */
+    static boolean isSafeCssClass( final String cssclass ) {
+        return cssclass.matches( "[\\w][\\w-]*(\\s+[\\w][\\w-]*)*" );
+    }
+
     private boolean needsSanitization(String link) {
         String testVal = link.toLowerCase().replaceAll("\\s+", "").trim();
         if (testVal.startsWith("data")
@@ -120,15 +172,15 @@
     public String execute( final Context context, final Map<String, String> params ) throws PluginException {
         final Engine engine  = context.getEngine();
         String src           = getCleanParameter( params, PARAM_SRC );
-        final String align   = getCleanParameter( params, PARAM_ALIGN );
+        String align         = getCleanParameter( params, PARAM_ALIGN );
         final String ht      = getCleanParameter( params, PARAM_HEIGHT );
         final String wt      = getCleanParameter( params, PARAM_WIDTH );
         final String alt     = getCleanParameter( params, PARAM_ALT );
         final String caption = getCleanParameter( params, PARAM_CAPTION );
         String link          = getCleanParameter( params, PARAM_LINK );
         String target        = getCleanParameter( params, PARAM_TARGET );
-        final String style   = getCleanParameter( params, PARAM_STYLE );
-        final String cssclass= getCleanParameter( params, PARAM_CLASS );
+        String style         = getCleanParameter( params, PARAM_STYLE );
+        String cssclass     = getCleanParameter( params, PARAM_CLASS );
         final String border  = getCleanParameter( params, PARAM_BORDER );
         final String title   = getCleanParameter( params, PARAM_TITLE );
 
@@ -142,6 +194,18 @@
             target = null; // not a valid value so ignore
         }
 
+        if( align != null && !align.equals( "left" ) && !align.equals( "right" ) && !align.equals( "center" ) ) {
+            align = null; // not a valid value so ignore; it is emitted into a CSS float declaration
+        }
+
+        if( style != null && !isSafeStyle( style ) ) {
+            style = null; // only allow-listed presentational CSS may pass, see isSafeStyle()
+        }
+
+        if( cssclass != null && !isSafeCssClass( cssclass ) ) {
+            cssclass = null; // not a plain list of CSS identifiers so ignore
+        }
+
         try {
             final AttachmentManager mgr = engine.getManager( AttachmentManager.class );
             final Attachment att = mgr.getAttachmentInfo( context, src );

jspwiki-main/src/test/java/org/apache/wiki/plugin/ImageStyleTest.java

@@ -0,0 +1,79 @@
+/*
+    Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+    Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+ */
+package org.apache.wiki.plugin;
+
+import org.apache.wiki.TestEngine;
+import org.apache.wiki.render.RenderingManager;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+
+/**
+ * Regression tests for the Image plugin style/class/align validation: author CSS must not be able to reposition
+ * content outside the plugin's own box (overlay phishing).
+ */
+public class ImageStyleTest {
+
+    static TestEngine testEngine = TestEngine.build();
+
+    @Test
+    public void overlayStyleIsDropped() throws Exception {
+        final String src = "[{Image src='img.png' link='https://evil.example/fake-login' style='position:fixed;top:0;left:0;width:100vw;height:100vh;z-index:9999;background:#fff'}]";
+        testEngine.saveText( "ImageStylePage1", src );
+        final String res = testEngine.getManager( RenderingManager.class ).getHTML( "ImageStylePage1" );
+        Assertions.assertFalse( res.contains( "position" ), res );
+        Assertions.assertFalse( res.contains( "z-index" ), res );
+    }
+
+    @Test
+    public void benignStyleIsKept() throws Exception {
+        final String src = "[{Image src='img.png' style='width:120px; border: 1px solid'}]";
+        testEngine.saveText( "ImageStylePage2", src );
+        final String res = testEngine.getManager( RenderingManager.class ).getHTML( "ImageStylePage2" );
+        Assertions.assertTrue( res.contains( "width:120px" ), res );
+    }
+
+    @Test
+    public void invalidAlignAndClassAreDropped() throws Exception {
+        final String src = "[{Image src='img.png' align='none;position:fixed' class='x onmouseover=alert(1)'}]";
+        testEngine.saveText( "ImageStylePage3", src );
+        final String res = testEngine.getManager( RenderingManager.class ).getHTML( "ImageStylePage3" );
+        Assertions.assertFalse( res.contains( "position:fixed" ), res );
+        Assertions.assertFalse( res.contains( "onmouseover" ), res );
+    }
+
+    @Test
+    public void isSafeStyleRejectsRepositioningAndEscapes() {
+        Assertions.assertTrue( Image.isSafeStyle( "width:120px; border: 1px solid" ) );
+        Assertions.assertFalse( Image.isSafeStyle( "position:fixed;top:0" ) );
+        Assertions.assertFalse( Image.isSafeStyle( "\\70 osition:fixed" ) );
+        Assertions.assertFalse( Image.isSafeStyle( "margin-top:-9999px" ) );
+        Assertions.assertFalse( Image.isSafeStyle( "background:url(//evil.example/x)" ) );
+        Assertions.assertFalse( Image.isSafeStyle( "transform:translate(-100px,-100px)" ) );
+    }
+
+    @Test
+    public void isSafeCssClassAcceptsIdentifiersOnly() {
+        Assertions.assertTrue( Image.isSafeCssClass( "imageplugin" ) );
+        Assertions.assertTrue( Image.isSafeCssClass( "one two-three" ) );
+        Assertions.assertFalse( Image.isSafeCssClass( "x{color:red}" ) );
+        Assertions.assertFalse( Image.isSafeCssClass( "a;b" ) );
+    }
+
+}