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

[David Woodhouse]

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


-- 
David Woodhouse                            Open Source Technology Centre
address@hidden                              Intel Corporation

smime.p7s
Description: S/MIME cryptographic signature