[Spice-devel] [PATCH spice-streaming-agent 1/3] Ensure that plugins cannot bypass version check

[Frediano Ziglio]

From: Christophe de Dinechin <dinechin at redhat.com>

This change addresses two 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,

[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)

Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
---
 include/spice-streaming-agent/plugin.hpp | 31 ++++++++++++-------------------
 src/concrete-agent.cpp                   | 17 ++++++++++++++++-
 src/concrete-agent.hpp                   |  5 +----
 src/mjpeg-fallback.cpp                   |  3 ---
 4 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
index e08e3a6..90651dd 100644
--- a/include/spice-streaming-agent/plugin.hpp
+++ b/include/spice-streaming-agent/plugin.hpp
@@ -102,20 +102,6 @@ public:
 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.
      */
@@ -135,19 +121,26 @@ typedef bool PluginInitFunc(spice::streaming_agent::Agent* agent);
 }} // namespace spice::streaming_agent
 
 #ifndef SPICE_STREAMING_AGENT_PROGRAM
+/*!
+ * Plugin interface version
+ * Each plugin should define this variable and set it to PluginVersion
+ * 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..58ce9c4 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -32,7 +32,7 @@ ConcreteAgent::ConcreteAgent()
 
 bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const
 {
-    unsigned version = Version();
+    unsigned version = PluginVersion;
     return MajorVersion(version) == MajorVersion(pluginVersion) &&
         MinorVersion(version) >= MinorVersion(pluginVersion);
 }
@@ -83,6 +83,21 @@ 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 (!PluginVersionIsCompatible(*version)) {
+        syslog(LOG_ERR,
+               "error loading plugin %s: plugin interface version %u.%u not accepted",
+               plugin_filename.c_str(),
+               MajorVersion(*version), MinorVersion(*version));
+        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..c0cf9ba 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -27,17 +27,14 @@ 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:
+    bool PluginVersionIsCompatible(unsigned pluginVersion) const;
     void LoadPlugin(const std::string &plugin_filename);
     std::vector<std::shared_ptr<Plugin>> plugins;
     std::vector<ConcreteConfigureOption> options;
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 {
-- 
2.14.3