[Spice-devel] [PATCH 1/2] Ensure that plugins cannot bypass version check
Frediano Ziglio
fziglio at redhat.com
Fri Mar 23 15:50:56 UTC 2018
>
> On Fri, 2018-03-23 at 13:05 +0100, Christophe de Dinechin wrote:
> > From: Christophe de Dinechin <dinechin at redhat.com>
> >
> > This change addresses three issues related to plugin version checking:
> >
> > 1. It is possible for plugins to bypass version checking or do it wrong
> > (as a matter of fact, the mjpeg fallback sets a bad example)
> >
> > 2. The current plugin version check violates the C++ ODR, i.e.
> > it relies on undefined behaviors when the header used to compile
> > the plugin and to compile the agent are not identical,
> >
> > 3. A major.minor numbering scheme is not ideal for ABI checks.
> > In particular, it makes it difficult to react to an incompatibility
> > that was detected post release.
> >
> > [More info]
> >
> > 1. Make it impossible to bypass version checking
> >
> > The current code depends on the plugin implementing the version check
> > correctly by calling PluginVersionIsCompatible. To make things worse,
> > the only publicly available example gets this wrong and uses an
> > ad-hoc version check, so anybody copy-pasting this code will get it
> > wrong.
> >
> > It is more robust to do the version check in the agent before calling
> > any method in the plugin. It ensures that version checking cannot be
> > bypassed, is done consistently and generates consistent error messages.
> >
> > As an indication that the aproach is robust, the new check correctly
> > refuses to load older plugins that use the old version checking method:
> >
> > spice-streaming-agent[5167]:
> > error loading plugin [...].so: no version information
> >
> > 2. ODR-related problems
> >
> > The C++ One Definition Rule (ODR) states that all translation units
> > must see the same definitions. In the current code, when we call
> > Agent::PluginVersionIsCompatible from the plugin, it is an ODR
> > violation as soon as we have made any change in the Agent class
> > compared to what the plugin was compiled against.
> >
> > The current code therefore relies on implementation dependent knowlege
> > of how virtual functions are laid out in the vtable, and puts
> > unnecessary constraints on the changes allowed in the classes
> > (e.g. it's not allowed to put anything before some member functions)
> >
> > 3. Major.minor numbering scheme
> >
> > The major.minor numbering scheme initially selected makes it harder
> > to fixes cases where an incompatibility was detected after release.
> >
> > For example, the major.minor version checking assumes that agent 1.21
> > is compatible with plugins 1.21, 1.13 or 1.03. If later testing
> > shows that 1.13 actually introduced an incompatiliy, you have to
> > special-case 1.13 in the compatibiliy check.
> >
> > An approach that does not have this problem is to rely on incremented
> > version numbers, with a "current" and "oldest compatible" version
> > number. This is used for example by libtools [1].
> >
> > Since the change required for #1 and #2 introduces an ABI break,
> > it is as good a time as any to also change the numbering scheme,
> > since changing it later would introduce another unnecessary ABI break.
>
> Great! AFAIK we haven't made a release yet so we don't need to worry
> about ABI breakage yet?
>
> > [1]
> > https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> >
> > Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> > ---
> > include/spice-streaming-agent/plugin.hpp | 50
> > +++++++++++++++++---------------
> > src/concrete-agent.cpp | 35 +++++++++++-----------
> > src/concrete-agent.hpp | 4 ---
> > src/mjpeg-fallback.cpp | 3 --
> > 4 files changed, 45 insertions(+), 47 deletions(-)
> >
> > diff --git a/include/spice-streaming-agent/plugin.hpp
> > b/include/spice-streaming-agent/plugin.hpp
> > index e08e3a6..0ec5040 100644
> > --- a/include/spice-streaming-agent/plugin.hpp
> > +++ b/include/spice-streaming-agent/plugin.hpp
> > @@ -23,11 +23,22 @@ namespace streaming_agent {
> > class FrameCapture;
> >
> > /*!
> > - * Plugin version, only using few bits, schema is 0xMMmm
> > - * where MM is major and mm is the minor, can be easily expanded
> > - * using more bits in the future
> > + * Plugins use a versioning system similar to that implemented by libtool
> > + *
> > http://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> > + * Update the version information as follows:
> > + * [ANY CHANGE] If any interfaces have been added, removed, or changed
> > since the last update,
> > + * increment PluginInterfaceVersion.
> > + * [COMPATIBLE CHANGE] If interfaces have only been added since the last
> > public release,
> > + * leave PluginInterfaceOldestCompatibleVersion identical.
> > + * [INCOMPATIBLE CHANGE] If any interfaces have been removed or changed
> > since the last release,
> > + * set PluginInterfaceOldestCompatibleVersion to PluginInterfaceVersion.
> > + * [DETECTED INCOMPATIBILITY]: If an incompatibility is detected after a
> > release,
> > + * set PluginInterfaceOldestCompatibleVersion to the last known compatible
> > version.
> > */
> > -enum Constants : unsigned { PluginVersion = 0x100u };
> > +enum Constants : unsigned {
> > + PluginInterfaceVersion = 1,
> > + PluginInterfaceOldestCompatibleVersion = 1
> > +};
>
> This is still not too pretty, consider at least renaming Constants to
> something better, or even use something like:
>
> struct PluginInterfaceVersion {
> static constexpr uint16_t current = 1;
> static constexpr uint16_t oldest_compatible = 1;
> };
>
> For the encapsulation?
>
> Cheers,
> Lukas
>
I agree with the rename.
Using a structure to define constants seems weird, an enumeration looks
more natural.
> > enum Ranks : unsigned {
> > /// this plugin should not be used
> > @@ -103,20 +114,6 @@ class Agent
> > {
> > public:
> > /*!
> > - * Get agent version.
> > - * Plugin should check the version for compatibility before doing
> > - * everything.
> > - * \return version specified like PluginVersion
> > - */
> > - virtual unsigned Version() const = 0;
> > -
> > - /*!
> > - * Check if a given plugin version is compatible with this agent
> > - * \return true is compatible
> > - */
> > - virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const =
> > 0;
> > -
> > - /*!
> > * Register a plugin in the system.
> > */
> > virtual void Register(Plugin& plugin) = 0;
> > @@ -136,18 +133,25 @@ typedef bool
> > PluginInitFunc(spice::streaming_agent::Agent* agent);
> >
> > #ifndef SPICE_STREAMING_AGENT_PROGRAM
> > /*!
> > + * Plugin interface version
> > + * Each plugin should define this variable and set it to
> > PluginInterfaceVersion
> > + * That version will be checked by the agent before executing any plugin
> > code
> > + */
> > +extern "C" unsigned spice_streaming_agent_plugin_interface_version;
> > +
> > +/*!
> > * Plugin main entry point.
> > - * Plugins should check if the version of the agent is compatible.
> > - * If is compatible should register itself to the agent and return
> > - * true.
> > - * If is not compatible can decide to stay in memory or not returning
> > - * true (do not unload) or false (safe to unload). This is necessary
> > + * This entry point is only called if the version check passed.
> > + * It should return true if it loaded and initialized successfully.
> > + * If the plugin does not initialize and does not want to be unloaded,
> > + * it may still return true on failure. This is necessary
> > * if the plugin uses some library which are not safe to be unloaded.
> > * This public interface is also designed to avoid exporting data from
> > * the plugin which could be a problem in some systems.
> > * \return true if plugin should stay loaded, false otherwise
> > */
> > extern "C" spice::streaming_agent::PluginInitFunc
> > spice_streaming_agent_plugin_init;
> > +
> > #endif
> >
> > #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP
> > diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> > index 4cf70e7..eb4f333 100644
> > --- a/src/concrete-agent.cpp
> > +++ b/src/concrete-agent.cpp
> > @@ -15,28 +15,11 @@
> >
> > using namespace spice::streaming_agent;
> >
> > -static inline unsigned MajorVersion(unsigned version)
> > -{
> > - return version >> 8;
> > -}
> > -
> > -static inline unsigned MinorVersion(unsigned version)
> > -{
> > - return version & 0xffu;
> > -}
> > -
> > ConcreteAgent::ConcreteAgent()
> > {
> > options.push_back(ConcreteConfigureOption(nullptr, nullptr));
> > }
> >
> > -bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion)
> > const
> > -{
> > - unsigned version = Version();
> > - return MajorVersion(version) == MajorVersion(pluginVersion) &&
> > - MinorVersion(version) >= MinorVersion(pluginVersion);
> > -}
> > -
> > void ConcreteAgent::Register(Plugin& plugin)
> > {
> > plugins.push_back(std::shared_ptr<Plugin>(&plugin));
> > @@ -83,6 +66,24 @@ void ConcreteAgent::LoadPlugin(const std::string
> > &plugin_filename)
> > return;
> > }
> >
> > + unsigned *version =
> > + (unsigned *) dlsym(dl,
> > "spice_streaming_agent_plugin_interface_version");
> > + if (!version) {
> > + syslog(LOG_ERR, "error loading plugin %s: no version information",
> > + plugin_filename.c_str());
> > + return;
> > + }
> > + if (*version < PluginInterfaceOldestCompatibleVersion ||
> > + *version > PluginInterfaceVersion) {
> > + syslog(LOG_ERR,
> > + "error loading plugin %s: plugin interface version %u, "
> > + "agent accepts version %u...%u",
> > + plugin_filename.c_str(), *version,
> > + PluginInterfaceOldestCompatibleVersion,
> > + PluginInterfaceVersion);
> > + return;
> > + }
> > +
> > try {
> > PluginInitFunc* init_func =
> > (PluginInitFunc *) dlsym(dl,
> > "spice_streaming_agent_plugin_init");
> > diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> > index 5bca23b..c631916 100644
> > --- a/src/concrete-agent.hpp
> > +++ b/src/concrete-agent.hpp
> > @@ -27,16 +27,12 @@ class ConcreteAgent final : public Agent
> > {
> > public:
> > ConcreteAgent();
> > - unsigned Version() const override {
> > - return PluginVersion;
> > - }
> > void Register(Plugin& plugin) override;
> > const ConfigureOption* Options() const override;
> > void LoadPlugins(const std::string &directory);
> > // pointer must remain valid
> > void AddOption(const char *name, const char *value);
> > FrameCapture *GetBestFrameCapture(const std::set<SpiceVideoCodecType>&
> > codecs);
> > - bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
> > private:
> > void LoadPlugin(const std::string &plugin_filename);
> > std::vector<std::shared_ptr<Plugin>> plugins;
> > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > index 68c282f..605a4b3 100644
> > --- a/src/mjpeg-fallback.cpp
> > +++ b/src/mjpeg-fallback.cpp
> > @@ -180,9 +180,6 @@ SpiceVideoCodecType MjpegPlugin::VideoCodecType() const
> > {
> >
> > bool MjpegPlugin::Register(Agent* agent)
> > {
> > - if (agent->Version() != PluginVersion)
> > - return false;
> > -
> > std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin());
> >
> > try {
Frediano
More information about the Spice-devel
mailing list