OpenImageIO icon indicating copy to clipboard operation
OpenImageIO copied to clipboard

OIIO attribute "plugins_override"

Open lgritz opened this issue 12 years ago • 3 comments

OIIO attribute "plugins_override" causes externally found plugins to override built-in plugins with the same name (instead of the default of built-in plugins always winning). Also can be overridden with the OIIO_PLUGINS_OVERRIDE environment variable.

lgritz avatar Jun 22 '13 16:06 lgritz

I don't honestly know why this sat for years.

If nobody ever complained or asked about it again for 6 years, presumably its need is minuscule and perhaps this PR should just be abandoned?

On the other hand, it's a simple change and intuitively it seems like it may at some point be useful to somebody (if you really really wanted to override a built-in plugin with a custom external one for the same file format, there is no simple way to do it without this patch).

I will rebase on top of master and push an update, see if it passes all the tests, then mull over whether to go ahead and merge it or abandon.

Thoughts?

lgritz avatar Nov 26 '19 05:11 lgritz

This is our oldest festering PR that was never merged or closed -- now revived, as suggested in #3099!

@MarkCallow, are you able to apply this patch on your side and test it out?

Everybody, this is an important thing to consider and I would like some feedback:

When I first proposed this (in 2013!), security was the last thing on my mind. But with many years under the bridge, and OIIO used critically in many places (including commercial apps), I think we need to grapple with the possibility of unintended consequences before we merge this.

Some people would consider it to be a security vulnerability that any app using OIIO that allows users to specify image filenames to read opens the door to specifying an image filename with a custom extension, and placing a custom DSO ImageInput implementation where it will be found when reading from that file, would allow arbitrary code, potentially malicious, to be executed from within the application (and with the application's privileges).

That's probably bad enough, but the hole is easily plugged by the application ensuring that no filenames are passed to OIIO that end with unknown extensions that aren't the built-in formats (like, allow ".tif", ".exr", ".jpg" only, and you know that no DSOs will be loaded). This PR makes it so that setting an option (which can be done with an environment variable) will allow externally-found plugins to take precedence over built-in. I hope it's clear why this is problematic.

I can think of several ways to mitigate this, and I would like your thoughts. Here are a few things I thought of, these are neither exhaustive nor mutually exclusive:

  • Don't care, this flexibility is great.
  • Build-time way to disable this feature entirely. (Side discussion: should the default be OFF, i.e. safe, or ON, i.e. flexible?)
  • Leave it as is, but make enabling it require a special API call from the app itself, NOT able to turn it on via environment variable. That would make it so a user couldn't externally hijack an app that didn't want to allow it.
  • Something else?

lgritz avatar Sep 22 '21 20:09 lgritz

@MarkCallow, are you able to apply this patch on your side and test it out?

I'm having problems building OIIO. I was able to build the latest version of the library (2.4) but building the "package" target failed. I was trying to build this to get the includes for my application. I then tried to build the library at the release-2.17.1.0 tag instead so I could use the includes I already have from my MacPorts installation. I got compile errors with the library about some C++ features that are not part of c++11. The need for updated headers stems from the version number being embedded in the namespace name. Frankly a PITA.

I using a Mac so there are additional issues to resolve regarding dyld finding and agreeing to link the library I build to my app.

MarkCallow avatar Sep 23 '21 05:09 MarkCallow

Closing this stale PR. Obviously nobody (including me) really needed it if it's taken us over 10 years to merge it.

lgritz avatar Mar 19 '24 03:03 lgritz