Re: [OATH-Toolkit-help] pskc_build_xml() use-after-free

[Simon Josefsson]

Thanks for the bug report!

I'm a little uncertain how well the proposed patch works.  What happens
if you call pskc_build_xml() multiple times?  Wouldn't there be many
xmlDoc pointers around?  And consequently potentially stale pointers if
any of them would be free'd?

/Simon

David Woodhouse <address@hidden> writes:

> When we parse a PSKC file with pskc_parse_from_memory() and subsequently
> process the xmlDoc with parse_keycontainer(), we build up a bunch of
> pointers from the container to the 'static' strings within the xmlDoc.
>
> Then we call pskc_build_xml() which builds a *new* xmlDoc and frees the
> old one. Leaving lots and lots of stale pointers to the memory we just
> freed.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1129491
>
> The best answer is to fix those stale pointers. We could possibly do
> that by calling parse_keycontainer() again in a new 'reparse' mode so
> that it doesn't reallocate things (and so that existing pointers remain
> valid). There's half an attempt at that in
> https://bugzilla.redhat.com/show_bug.cgi?id=1129491#c1
>
> Alternatively, we could set the fields in the data structures *as* we
> build up the new xmlDoc. But that has issues if we get a failure when
> building the new xmlDoc and we'd need to roll back. At least with the
> former option we could be fairly sure that parse_keycontainer() really
> *shouldn't* fail on the xmlDoc that we just created.
>
> The third and by far the simplest approach, although less efficient, is
> just to keep the original xmlDoc around for the entire lifetime of the
> container. That's what this patch does:
>
> diff --git a/libpskc/build.c b/libpskc/build.c
> index 8f0840f..c3e84ad 100644
> --- a/libpskc/build.c
> +++ b/libpskc/build.c
> @@ -510,7 +510,7 @@ pskc_build_xml (pskc_t * container, char **out, size_t * 
> len)
>  
>    xmlDocSetRootElement (doc, keycont);
>  
> -  if (container->xmldoc)
> +  if (container->xmldoc && container->xmldoc != container->original_xmldoc)
>      xmlFreeDoc (container->xmldoc);
>    container->xmldoc = doc;
>    doc = NULL;
> diff --git a/libpskc/internal.h b/libpskc/internal.h
> index 674625d..9b36e28 100644
> --- a/libpskc/internal.h
> +++ b/libpskc/internal.h
> @@ -103,7 +103,7 @@ struct pskc_key
>  struct pskc
>  {
>    /* raw XML */
> -  xmlDocPtr xmldoc;
> +  xmlDocPtr xmldoc, original_xmldoc;
>    /* Is there a Signature element in xmldoc? */
>    int signed_p;
>  
> diff --git a/libpskc/parser.c b/libpskc/parser.c
> index 47f8bd6..d75e5ac 100644
> --- a/libpskc/parser.c
> +++ b/libpskc/parser.c
> @@ -677,6 +677,8 @@ pskc_done (pskc_t * container)
>      return;
>  
>    xmlFreeDoc (container->xmldoc);
> +  if (container->original_xmldoc != container->xmldoc)
> +       xmlFreeDoc (container->original_xmldoc);
>  
>    for (i = 0; i < container->nkeypackages; i++)
>      {
> @@ -717,7 +719,7 @@ pskc_parse_from_memory (pskc_t * container, size_t len, 
> const char *buffer)
>    if (xmldoc == NULL)
>      return PSKC_XML_ERROR;
>  
> -  container->xmldoc = xmldoc;
> +  container->original_xmldoc = container->xmldoc = xmldoc;
>  
>    root = xmlDocGetRootElement (xmldoc);
>    parse_keycontainer (container, root, &rc);