diff --git a/jspwiki-main/src/main/java/org/apache/wiki/plugin/Image.java b/jspwiki-main/src/main/java/org/apache/wiki/plugin/Image.java index 1429b3ebdd..e8b8db047f 100644 --- a/jspwiki-main/src/main/java/org/apache/wiki/plugin/Image.java +++ b/jspwiki-main/src/main/java/org/apache/wiki/plugin/Image.java @@ -32,6 +32,7 @@ Licensed to the Apache Software Foundation (ASF) under one import java.util.Map; import java.util.ResourceBundle; +import java.util.Set; /** @@ -84,6 +85,16 @@ public class Image implements Plugin { /** The parameter name for setting the title. Value is {@value}. */ public static final String PARAM_TITLE = "title"; + /** Purely presentational CSS properties the style 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 @@ public String getSnipExample() { 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 (\\70 osition), smuggle a URL (url(...)), or drag the + * rendered box over other page content (margin-top:-9999px). + * + * @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 class 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 @@ private boolean needsSanitization(String link) { public String execute( final Context context, final Map 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 @@ public String execute( final Context context, final Map params ) 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 ); diff --git a/jspwiki-main/src/test/java/org/apache/wiki/plugin/ImageStyleTest.java b/jspwiki-main/src/test/java/org/apache/wiki/plugin/ImageStyleTest.java new file mode 100644 index 0000000000..9ba73c915f --- /dev/null +++ b/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" ) ); + } + +}