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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Apr 18, 2018 at 12:47:41PM +0100, Frediano Ziglio wrote:
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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;
> +

This probably belongs to another commit.

>  #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;
> -

This bit is not really related to this change, the plugin is static,
agent->Version and PluginVersion will always be the same iirc, so the
check can be dropped because of this.

Apart from this, this is very similar from my split attempt a few weeks
ago.

Christophe

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]