Support » Plugin: SVG Support » Unsafe handling of SVG Path

  • Hi, thank you for this plugin.

    Please consider the following change to line 40 of /svg-support/functions/attachment-modal.php

    
    function bodhi_svgs_get_dimensions( $svg ) {
    
    	$svg = @simplexml_load_file( $svg );
            // add @ or otherwise validate this
            // check this uses basepath()
            // check the file exists
            // check the file is readable
            // check this is an SVG
    
    	if ( $svg === FALSE ) {
    
    		$width = '0';
    		$height = '0';
    
    	} else {
    
    		$attributes = $svg->attributes();
    		$width = (string) $attributes->width;
    		$height = (string) $attributes->height;
    	}
    
    	return (object) array( 'width' => $width, 'height' => $height );
    
    }
    • This topic was modified 5 months, 1 week ago by  mystream. Reason: code not shown as intended
Viewing 12 replies - 1 through 12 (of 12 total)
  • Plugin Author Benbodhi

    (@benbodhi)

    Thanks for your support and suggestion. I am actually working on this file and hoping to release the next version soon. I will look at adding this.

    Can you please tell me what you’re experiencing to prompt this change to make sure I can reproduce and fully understand the issue?

    mystream

    (@mystream)

    Hi Benbodhi,

    We downloaded the Origen theme from themeforest.net.

    In this theme it references your plugin.

    When we logged in to Customize the theme, we found the Customize menu didn’t load fully. When I checked the XHR requests in Firefox I noticed that the CSS file included a PHP error in the output.

    I tracked that error to the file in your plugin.

    The error seemed to be caused by the theme author’s use of a utf8 character in the filename of an svg file. When your file tried to load the file passed by the theme, it was not able to and generated an error notice that showed up in the rendering of the CSS file.

    I added @ in front of the call to suppress the error, but it’s only a temporary fix.

    Let me know if you would like any help fixing this one and I would be happy to try to assist.

    Warmest thanks,
    MyStream

    Plugin Author Benbodhi

    (@benbodhi)

    Interesting, thanks for the info!
    Iā€™m not too sure how that even works to be honest, but will look into it.
    Of course if you have time, I would be super appreciative of any input.

    mystream

    (@mystream)

    Hi Benbodhi,

    Perhaps:

    
    function bodhi_svgs_get_dimensions( $svg ) {
    
    	// Suppress errors internally
    	libxml_use_internal_errors(true);
    
    	// Create a known, reliable path
    	$path = $_SERVER['DOCUMENT_ROOT'].'/path/to/svg/'.basename($svg);
    
    	// Make sure it exists and can be read
    	if(file_exists($path) && is_readable($path)) {
    		$svg = simplexml_load_file( $path );
    
    		// Make sure the document returned is valid
    		if(false !== $svg) {
    			// Rest of your code here
    		}
    	}
    	...
    
    igarciaoliver

    (@igarciaoliver)

    great work @mystream thank you for taking the time to help the community. I use this plugin on pretty much every project and I appreciate it

    Plugin Author Benbodhi

    (@benbodhi)

    Hi @mystream and @igarciaoliver,

    Thanks for chiming in on this! It wasn’t showing up in my unresolved threads for some reason, and I KNEW there was a thread about this, but couldn’t find it. Perfect timing as I’m working on this again today.
    I actually tackled it slightly differently, but it opened up a different issue for me. So I will play more with this code and see if I can get something into an update today.

    Plugin Author Benbodhi

    (@benbodhi)

    Hey @mystream,

    I’ve been playing with this code all afternoon and can’t seem to get any implementation to work unfortunately.

    I don’t see this issue because I don’t have the theme. I tried to replicate but can’t. So I just tried to implement your code to see if I could work it in but I keep breaking things šŸ™

    mystream

    (@mystream)

    Hi @benbodhi,

    Can you post your update here and I’ll try to offer any thoughts if I can?

    Thank you šŸ™‚
    MyStream

    Plugin Author Benbodhi

    (@benbodhi)

    Sorry, I missed this reply.
    The code from the file in question:

    <?php
    /**
     * Display SVG in attachment modal
     */
    if ( ! defined( 'ABSPATH' ) ) {
    	exit; // Exit if accessed directly
    }
    
    function bodhi_svgs_response_for_svg( $response, $attachment, $meta ) {
    
    	if ( $response['mime'] == 'image/svg+xml' && empty( $response['sizes'] ) ) {
    
    		$svg_path = get_attached_file( $attachment->ID );
    
    		if ( ! file_exists( $svg_path ) ) {
    			// If SVG is external, use the URL instead of the path
    			$svg_path = $response['url'];
    		}
    
    		$dimensions = bodhi_svgs_get_dimensions( $svg_path );
    
    		$response['sizes'] = array(
    			'full' => array(
    				'url' => $response['url'],
    				'width' => $dimensions->width,
    				'height' => $dimensions->height,
    				'orientation' => $dimensions->width > $dimensions->height ? 'landscape' : 'portrait'
    				)
    			);
    
    	}
    
    	return $response;
    
    }
    add_filter( 'wp_prepare_attachment_for_js', 'bodhi_svgs_response_for_svg', 10, 3 );
    
    function bodhi_svgs_get_dimensions( $svg ) {
    
    	$svg = simplexml_load_file( $svg );
    
    	if ( $svg === FALSE ) {
    
    		$width = '0';
    		$height = '0';
    
    	} else {
    
    		$attributes = $svg->attributes();
    		$width = (string) $attributes->width;
    		$height = (string) $attributes->height;
    
    	}
    
    	return (object) array( 'width' => $width, 'height' => $height );
    
    }
    

    Hi @benbodhi,

    Imagine the $svg file contains a utf8 character or an invalid path.

    Instead of $svg being FALSE it will kick out an error. Because it won’t be FALSE, it will move to the ELSE statement.

    That will then cause an error with $svg->attributes();

    This is the bug I was getting.

    I think you need to check if you can read $svg using is_file() or is_readable() etc along with basename and a path so that someone can’t use ../../ to do directory traversal through the code.

    Once you know it’s a proper file, in an acceptable place on the drive, and that you can read it, then I think you should either try/catch it or add an error handler in case it fails. If the error handler’s not caught an error, and the $svg variable has returned a resource, then carry on to the else.

    I think it’s just more error checking needed when you can’t control the origin of $svg.

    These were my notes for it:

    // add @ or otherwise validate this
    // check this uses basepath()
    // check the file exists
    // check the file is readable
    // check this is an SVG

    Is that something you can add?

    Thanks,
    MyStream

    Plugin Author Benbodhi

    (@benbodhi)

    Hey, thanks for the explanation. I understand the logic, but whenever I try to implement these things, I keep breaking it. I guess I don’t understand it quite enough.

    I’ll keep playing though and see how I go.

    Thanks for taking the time to help out.

    Hi Benbodhi,

    Is this any help at all?
    https://pastebin.com/qz8vUL5u

    Thanks!
    Andrew

Viewing 12 replies - 1 through 12 (of 12 total)
  • You must be logged in to reply to this topic.