Changeset 11526 in josm

Timestamp:

2017-02-02T01:22:52+01:00 (7 years ago)

Author:

Don-vip

Message:

fix #14319 - CVE-2017-5617: svgSalamander SSRF (Server-Side Request Forgery)

Location:

trunk

Files:

• src/com/kitfox/svg/ImageSVG.java (modified) (3 diffs)
• test/data/regress/14319 (added)
• test/data/regress/14319/attack.svg (added)
• test/unit/org/openstreetmap/josm/tools/ImageProviderTest.java (modified) (4 diffs)

trunk/src/com/kitfox/svg/ImageSVG.java

@@ -115 +115 @@
             {
                 URI src = sty.getURIValue(getXMLBase());
+                // CVE-2017-5617: Allow only data scheme
                 if ("data".equals(src.getScheme()))
                 {
                     imageSrc = new URL(null, src.toASCIIString(), new Handler());
-                } else
-                {
-                    try
-                    {
-                        imageSrc = src.toURL();
-                    } catch (Exception e)
-                    {
-                        Logger.getLogger(SVGConst.SVG_LOGGER).log(Level.WARNING,
-                            "Could not parse xlink:href " + src, e);
-//                        e.printStackTrace();
-                        imageSrc = null;
-                    }
                 }
             }
@@ -137 +126 @@
         }
 
-        diagram.getUniverse().registerImage(imageSrc);
-
-        //Set widths if not set
-        BufferedImage img = diagram.getUniverse().getImage(imageSrc);
-        if (img == null)
-        {
+        if (imageSrc != null)
+        {
+            diagram.getUniverse().registerImage(imageSrc);
+
+            //Set widths if not set
+            BufferedImage img = diagram.getUniverse().getImage(imageSrc);
+            if (img == null)
+            {
+                xform = new AffineTransform();
+                bounds = new Rectangle2D.Float();
+                return;
+            }
+
+            if (width == 0)
+            {
+                width = img.getWidth();
+            }
+            if (height == 0)
+            {
+                height = img.getHeight();
+            }
+
+            //Determine image xform
             xform = new AffineTransform();
-            bounds = new Rectangle2D.Float();
-            return;
-        }
-
-        if (width == 0)
-        {
-            width = img.getWidth();
-        }
-        if (height == 0)
-        {
-            height = img.getHeight();
-        }
-
-        //Determine image xform
-        xform = new AffineTransform();
-//        xform.setToScale(this.width / img.getWidth(), this.height / img.getHeight());
-//        xform.translate(this.x, this.y);
-        xform.translate(this.x, this.y);
-        xform.scale(this.width / img.getWidth(), this.height / img.getHeight());
+            xform.translate(this.x, this.y);
+            xform.scale(this.width / img.getWidth(), this.height / img.getHeight());
+        }
 
         bounds = new Rectangle2D.Float(this.x, this.y, this.width, this.height);
@@ -336 +326 @@
                 URI src = sty.getURIValue(getXMLBase());
 
-                URL newVal;
+                URL newVal = null;
+                // CVE-2017-5617: Allow only data scheme
                 if ("data".equals(src.getScheme()))
                 {
                     newVal = new URL(null, src.toASCIIString(), new Handler());
-                } else
-                {
-                    newVal = src.toURL();
                 }
 
-                if (!newVal.equals(imageSrc))
+                if (newVal != null && !newVal.equals(imageSrc))
                 {
                     imageSrc = newVal;

trunk/test/unit/org/openstreetmap/josm/tools/ImageProviderTest.java

@@ -3 +3 @@
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 
@@ -9 +10 @@
 import java.io.File;
 import java.io.IOException;
+import java.util.logging.Handler;
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+
+import javax.swing.ImageIcon;
 
 import org.junit.BeforeClass;
@@ -15 +21 @@
 import org.openstreetmap.josm.TestUtils;
 
+import com.kitfox.svg.SVGConst;
+
 /**
  * Unit tests of {@link ImageProvider} class.
  */
 public class ImageProviderTest {
+
+    private static final class LogHandler14319 extends Handler {
+        boolean failed;
+
+        @Override
+        public void publish(LogRecord record) {
+            if ("Could not load image: https://host-in-the-trusted-network.com/test.jpg".equals(record.getMessage())) {
+                failed = true;
+            }
+        }
+
+        @Override
+        public void flush() {
+        }
+
+        @Override
+        public void close() throws SecurityException {
+        }
+    }
 
     /**
@@ -53 +80 @@
 
     /**
+     * Non-regression test for ticket <a href="https://josm.openstreetmap.de/ticket/14319">#14319</a>
+     * @throws IOException if an error occurs during reading
+     */
+    @Test
+    public void testTicket14319() throws IOException {
+        LogHandler14319 handler = new LogHandler14319();
+        Logger.getLogger(SVGConst.SVG_LOGGER).addHandler(handler);
+        ImageIcon img = new ImageProvider(
+                new File(TestUtils.getRegressionDataDir(14319)).getAbsolutePath(), "attack.svg").get();
+        assertNotNull(img);
+        assertFalse(handler.failed);
+    }
+
+    /**
      * Test fetching an image using {@code wiki://} protocol.
      */