Re: [PATCH v3 58/60] loader: sanity check entity filenames when loading

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

 



On Mon, Oct 12, 2015 at 06:11:19PM +0100, Daniel P. Berrange wrote:
> When we have loaded the id attribute of the entity, check it
> against the entity filename to ensure its XML document was
> held in a file with the modern naming.
> 
> This merely prints out a warning for now to avoid breaking
> existing users, but in the future, files with non-compliant
> names will not be loaded at all.
> 
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
>  osinfo/osinfo_loader.c | 105 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 97 insertions(+), 8 deletions(-)
> 
> diff --git a/osinfo/osinfo_loader.c b/osinfo/osinfo_loader.c
> index f7b841d..8a21a1b 100644
> --- a/osinfo/osinfo_loader.c
> +++ b/osinfo/osinfo_loader.c
> @@ -30,6 +30,7 @@
>  #include <gio/gio.h>
>  
>  #include <string.h>
> +#include <ctype.h>
>  #include <libxml/parser.h>
>  #include <libxml/tree.h>
>  #include <libxml/xpath.h>
> @@ -487,7 +488,49 @@ static OsinfoInstallScript *osinfo_loader_get_install_script(OsinfoLoader *loade
>      return script;
>  }
>  
> +
> +static gboolean osinfo_loader_check_id(const gchar *relpath,
> +                                       const gchar *type,
> +                                       const gchar *id)
> +{
> +    gchar *name;
> +    gchar *suffix;
> +    gboolean sep = FALSE;
> +    gsize i;
> +    if (g_str_has_prefix(id, "http://";)) {
> +        suffix = g_strdup(id + strlen("http://";));
> +    } else {
> +        suffix = g_strdup(id);
> +    }
> +    for (i = 0; suffix[i]; i++) {
> +        if (suffix[i] == '/' && !sep) {
> +            sep = TRUE;
> +        } else if (!isalnum(suffix[i]) &&
> +                   suffix[i] != '-' &&
> +                   suffix[i] != '.' &&
> +                   suffix[i] != '_') {
> +                suffix[i] = '-';
> +        }
> +    }
> +    name = g_strdup_printf("/%s/%s.xml", type, suffix);
> +    g_free(suffix);
> +
> +    if (!g_str_equal(relpath, name)) {
> +        g_warning("Entity %s should be in file %s not %s",
> +                  id, name, relpath);

name is leaked in this codepath.

> +        return TRUE; /* In future switch to FALSE to refuse
> +                      * to load non-compliant named files.
> +                      * Need a period of grace for backcompat
> +                      * first though... Switch ETA Jan 2017
> +                      */
> +    }
> +    g_free(name);
> +    return TRUE;
> +}
> +
> +
>  static void osinfo_loader_device(OsinfoLoader *loader,
> +                                 const gchar *relpath,
>                                   xmlXPathContextPtr ctxt,
>                                   xmlNodePtr root,
>                                   GError **err)
> @@ -508,6 +551,10 @@ static void osinfo_loader_device(OsinfoLoader *loader,
>          OSINFO_ERROR(err, _("Missing device id property"));
>          return;
>      }
> +    if (!osinfo_loader_check_id(relpath, "device", id)) {
> +        xmlFree(id);
> +        return;
> +    }
>  
>      OsinfoDevice *device = osinfo_loader_get_device(loader, id);
>      g_hash_table_remove(loader->priv->entity_refs, id);
> @@ -652,6 +699,7 @@ static void osinfo_loader_product(OsinfoLoader *loader,
>  }
>  
>  static void osinfo_loader_platform(OsinfoLoader *loader,
> +                                   const gchar *relpath,
>                                     xmlXPathContextPtr ctxt,
>                                     xmlNodePtr root,
>                                     GError **err)
> @@ -661,6 +709,10 @@ static void osinfo_loader_platform(OsinfoLoader *loader,
>          OSINFO_ERROR(err, _("Missing platform id property"));
>          return;
>      }
> +    if (!osinfo_loader_check_id(relpath, "platform", id)) {
> +        xmlFree(id);
> +        return;
> +    }
>  
>      OsinfoPlatform *platform = osinfo_loader_get_platform(loader, id);
>      g_hash_table_remove(loader->priv->entity_refs, id);
> @@ -681,6 +733,7 @@ static void osinfo_loader_platform(OsinfoLoader *loader,
>  }
>  
>  static void osinfo_loader_deployment(OsinfoLoader *loader,
> +                                     const gchar *relpath,
>                                       xmlXPathContextPtr ctxt,
>                                       xmlNodePtr root,
>                                       GError **err)
> @@ -690,6 +743,10 @@ static void osinfo_loader_deployment(OsinfoLoader *loader,
>          OSINFO_ERROR(err, _("Missing deployment id property"));
>          return;
>      }
> +    if (!osinfo_loader_check_id(relpath, "deployment", id)) {
> +        xmlFree(id);
> +        return;
> +    }
>  
>      gchar *osid = osinfo_loader_string("string(./os/@id)", loader, ctxt, err);
>      if (!osid && 0) {
> @@ -730,6 +787,7 @@ static void osinfo_loader_deployment(OsinfoLoader *loader,
>  }
>  
>  static void osinfo_loader_datamap(OsinfoLoader *loader,
> +                                  const gchar *relpath,
>                                    xmlXPathContextPtr ctxt,
>                                    xmlNodePtr root,
>                                    GError **err)
> @@ -744,6 +802,10 @@ static void osinfo_loader_datamap(OsinfoLoader *loader,
>          OSINFO_ERROR(err, _("Missing os id property"));
>          return;
>      }
> +    if (!osinfo_loader_check_id(relpath, "datamap", id)) {
> +        xmlFree(id);
> +        return;
> +    }
>  
>      OsinfoDatamap *map = osinfo_loader_get_datamap(loader, id);
>      g_hash_table_remove(loader->priv->entity_refs, id);
> @@ -836,6 +898,7 @@ static OsinfoAvatarFormat *osinfo_loader_avatar_format(OsinfoLoader *loader,
>  }
>  
>  static void osinfo_loader_install_script(OsinfoLoader *loader,
> +                                         const gchar *relpath,
>                                           xmlXPathContextPtr ctxt,
>                                           xmlNodePtr root,
>                                           GError **err)
> @@ -864,6 +927,10 @@ static void osinfo_loader_install_script(OsinfoLoader *loader,
>          return;
>      }
>  
> +    if (!osinfo_loader_check_id(relpath, "install-script", id)) {
> +        xmlFree(id);
> +        return;
> +    }
>      OsinfoInstallScript *installScript = osinfo_loader_get_install_script(loader,
>                                                                            id);
>      g_hash_table_remove(loader->priv->entity_refs, id);
> @@ -1291,6 +1358,7 @@ static OsinfoDeviceDriver *osinfo_loader_driver(OsinfoLoader *loader,
>  
>  
>  static void osinfo_loader_os(OsinfoLoader *loader,
> +                             const gchar *relpath,
>                               xmlXPathContextPtr ctxt,
>                               xmlNodePtr root,
>                               GError **err)
> @@ -1311,6 +1379,10 @@ static void osinfo_loader_os(OsinfoLoader *loader,
>          OSINFO_ERROR(err, _("Missing os id property"));
>          return;
>      }
> +    if (!osinfo_loader_check_id(relpath, "os", id)) {
> +        xmlFree(id);
> +        return;
> +    }
>  
>      OsinfoOs *os = osinfo_loader_get_os(loader, id);
>      g_hash_table_remove(loader->priv->entity_refs, id);
> @@ -1459,6 +1531,7 @@ cleanup:
>  }
>  
>  static void osinfo_loader_root(OsinfoLoader *loader,
> +                               const gchar *relpath,
>                                 xmlXPathContextPtr ctxt,
>                                 xmlNodePtr root,
>                                 GError **err)
> @@ -1494,22 +1567,22 @@ static void osinfo_loader_root(OsinfoLoader *loader,
>          ctxt->node = it;
>  
>          if (xmlStrEqual(it->name, BAD_CAST "device"))
> -            osinfo_loader_device(loader, ctxt, it, err);
> +            osinfo_loader_device(loader, relpath, ctxt, it, err);
>  
>          else if (xmlStrEqual(it->name, BAD_CAST "platform"))
> -            osinfo_loader_platform(loader, ctxt, it, err);
> +            osinfo_loader_platform(loader, relpath, ctxt, it, err);
>  
>          else if (xmlStrEqual(it->name, BAD_CAST "os"))
> -            osinfo_loader_os(loader, ctxt, it, err);
> +            osinfo_loader_os(loader, relpath, ctxt, it, err);
>  
>          else if (xmlStrEqual(it->name, BAD_CAST "deployment"))
> -            osinfo_loader_deployment(loader, ctxt, it, err);
> +            osinfo_loader_deployment(loader, relpath, ctxt, it, err);
>  
>          else if (xmlStrEqual(it->name, BAD_CAST "install-script"))
> -            osinfo_loader_install_script(loader, ctxt, it, err);
> +            osinfo_loader_install_script(loader, relpath, ctxt, it, err);
>  
>          else if (xmlStrEqual(it->name, BAD_CAST "datamap"))
> -            osinfo_loader_datamap(loader, ctxt, it, err);
> +            osinfo_loader_datamap(loader, relpath, ctxt, it, err);
>  
>          ctxt->node = saved;
>  
> @@ -1536,6 +1609,7 @@ catchXMLError(void *ctx, const char *msg ATTRIBUTE_UNUSED, ...)
>  }
>  
>  static void osinfo_loader_process_xml(OsinfoLoader *loader,
> +                                      const gchar *relpath,
>                                        const gchar *xmlStr,
>                                        const gchar *src,
>                                        GError **err)
> @@ -1580,7 +1654,7 @@ static void osinfo_loader_process_xml(OsinfoLoader *loader,
>  
>      ctxt->node = root;
>  
> -    osinfo_loader_root(loader, ctxt, root, err);
> +    osinfo_loader_root(loader, relpath, ctxt, root, err);
>  
>   cleanup:
>      xmlXPathFreeContext(ctxt);
> @@ -1741,22 +1815,37 @@ osinfo_loader_process_file_reg_pci(OsinfoLoader *loader,
>  
>  static void
>  osinfo_loader_process_file_reg_xml(OsinfoLoader *loader,
> +                                   GFile *base,
>                                     GFile *file,
>                                     GError **err)
>  {
>      gchar *xml = NULL;
>      gsize xmlLen;
> +    gchar *basepath, *filepath;
> +    const gchar *relpath;
> +
>      g_file_load_contents(file, NULL, &xml, &xmlLen, NULL, err);
>      if (error_is_set(err))
>          return;
>  
> +    basepath = g_file_get_path(base);
> +    filepath = g_file_get_path(file);
> +    if (g_str_has_prefix(filepath, basepath)) {
> +        relpath = filepath + strlen(basepath);
> +    } else {
> +        relpath = filepath;
> +        g_warning("File %s does not have prefix %s", filepath, basepath);
> +    }

g_file_get_relative_path() is going to do the hunk above for you.

>      gchar *uri = g_file_get_uri(file);
>      osinfo_loader_process_xml(loader,
> +                              relpath,
>                                xml,
>                                uri,
>                                err);
>      g_free(uri);
>      g_free(xml);
> +    g_free(filepath);
> +    g_free(basepath);
>  }
>  
>  
> @@ -1838,7 +1927,7 @@ static void osinfo_loader_process_list(OsinfoLoader *loader,
>  
>              tmp = files;
>              while (tmp) {
> -                osinfo_loader_process_file_reg_xml(loader, tmp->data, &lerr);
> +                osinfo_loader_process_file_reg_xml(loader, *dirs, tmp->data, &lerr);
>                  if (lerr) {
>                      g_propagate_error(err, lerr);
>                      break;
> -- 
> 2.4.3
> 
> _______________________________________________
> Libosinfo mailing list
> Libosinfo@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libosinfo

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Libosinfo mailing list
Libosinfo@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libosinfo

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Fedora Users]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]

  Powered by Linux