Unsafe handling of SVG Path

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?

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

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.

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
		}
	}
	...

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

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.

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 ??

Hi @benbodhi,

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

Thank you ??
MyStream

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

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

I completely missed this reply!
I’ll see if I can test this as soon as I have time. I just moved country so been flat out lately.