From b3f5b81dc46e7f893eab4f038d834abd2785b84f Mon Sep 17 00:00:00 2001
From: William Brown <firstyear@redhat.com>
Date: Tue, 12 Apr 2016 14:29:19 +1000
Subject: [PATCH 2/7] Ticket 10 - Use after free

Bug Description:  Coverity and ASAN detected use after frees related to the
reuse of the pin object.

Fix Description:  This corrects the behaviours that would cause the use
after free to occur

https://pagure.io/svrcore/issue/10

Author: wibrown

Review by: ???
---
 examples/svrcore_driver.c | 20 ++++++++++----------
 src/pin.c                 |  9 +++++++++
 src/pk11.c                |  8 +++++---
 src/std-systemd.c         |  5 +++--
 src/std.c                 |  4 +++-
 src/svrcore.h             |  7 +++++++
 6 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/examples/svrcore_driver.c b/examples/svrcore_driver.c
index 44a02d6..2b6703e 100644
--- a/examples/svrcore_driver.c
+++ b/examples/svrcore_driver.c
@@ -62,7 +62,7 @@ svrcore_systemd_get_token()
     // Should set a password into &pw
 
     // Cleanup
-    SVRCORE_DestroySystemdPinObj(StdPinObj);
+    SVRCORE_DestroyRegisteredPinObj();
 
     return 0;
 }
@@ -71,21 +71,21 @@ int
 svrcore_stdsystemd_setup()
 {
     PRErrorCode errorCode;
-    SVRCOREStdSystemdPinObj *StdPinObj;
+    SVRCOREStdSystemdPinObj *StdSysPinObj;
     char *filename = "/tmp/pin.txt";
 
-    StdPinObj = (SVRCOREStdSystemdPinObj *)SVRCORE_GetRegisteredPinObj();
+    StdSysPinObj = (SVRCOREStdSystemdPinObj *)SVRCORE_GetRegisteredPinObj();
 
-    if (StdPinObj) {
+    if (StdSysPinObj) {
         // This means it's already registered?
         return 0;
     }
-    if (SVRCORE_CreateStdSystemdPinObj(&StdPinObj, filename, PR_FALSE, PR_TRUE, 60) != SVRCORE_Success) {
+    if (SVRCORE_CreateStdSystemdPinObj(&StdSysPinObj, filename, PR_FALSE, PR_TRUE, 60) != SVRCORE_Success) {
         errorCode = PR_GetError();
         printf("Unable to create std systemd pin %d\n", errorCode);
         return -1;
     }
-    SVRCORE_RegisterPinObj((SVRCOREPinObj *)StdPinObj);
+    SVRCORE_RegisterPinObj((SVRCOREPinObj *)StdSysPinObj);
     return 0;
 }
 
@@ -94,17 +94,17 @@ svrcore_stdsystemd_get_token()
 {
     //Actually get the password
     // Get the pinobj
-    SVRCOREStdSystemdPinObj *StdPinObj;
+    SVRCOREStdSystemdPinObj *StdSysPinObj;
     char *pw = NULL;
     char *token = NULL;
     SVRCOREError err = SVRCORE_Success;
 
-    StdPinObj = (SVRCOREStdSystemdPinObj *)SVRCORE_GetRegisteredPinObj();
+    StdSysPinObj = (SVRCOREStdSystemdPinObj *)SVRCORE_GetRegisteredPinObj();
     // Are we interactive?
     // SVRCORE_SetStdPinInteractive((SVRCOREStdPinObj *) StdPinObj , PR_TRUE);
     // what is token?
     token = "internal (software)";
-    pw = SVRCORE_GetPin( (SVRCOREPinObj *)StdPinObj, token , PR_FALSE);
+    pw = SVRCORE_GetPin( (SVRCOREPinObj *)StdSysPinObj, token , PR_FALSE);
     if ( err != SVRCORE_Success || pw == NULL) {
         printf("Couldn't get pin %d \n", err);
     } else {
@@ -114,7 +114,7 @@ svrcore_stdsystemd_get_token()
     // Should set a password into &pw
 
     // Cleanup
-    SVRCORE_DestroyStdSystemdPinObj(StdPinObj);
+    SVRCORE_DestroyRegisteredPinObj();
 
     return 0;
 }
diff --git a/src/pin.c b/src/pin.c
index acfc151..718768e 100644
--- a/src/pin.c
+++ b/src/pin.c
@@ -68,3 +68,12 @@ SVRCORE_GetRegisteredPinObj(void)
 {
   return pinObj;
 }
+
+void
+SVRCORE_DestroyRegisteredPinObj(void)
+{
+  if (pinObj) {
+    pinObj->methods->destroyObj(pinObj);
+  }
+  pinObj = 0;
+}
diff --git a/src/pk11.c b/src/pk11.c
index ebc7f16..0634732 100644
--- a/src/pk11.c
+++ b/src/pk11.c
@@ -274,9 +274,11 @@ SVRCORE_Pk11StoreGetPin(char **out, SVRCOREPk11PinStore *store)
   if (rv)
   {
     err = SVRCORE_System_Error;
-    memset(plain, 0, store->length);
-    free(plain);
-    plain = 0;
+    if (plain) {
+      memset(plain, 0, store->length);
+      free(plain);
+      plain = 0;
+    }
   }
 
   *out = (char *)plain;
diff --git a/src/std-systemd.c b/src/std-systemd.c
index de62110..cffc11d 100644
--- a/src/std-systemd.c
+++ b/src/std-systemd.c
@@ -138,13 +138,14 @@ SVRCORE_CreateStdSystemdPinObj(
         obj->top = top;
     } while(0);
 
+    *out = obj;
+
     if (err != SVRCORE_Success)
     {
         SVRCORE_DestroyStdSystemdPinObj(obj);
+        *out = NULL;
     }
 
-    *out = obj;
-
     return err;
 #endif // win32
 #else // systemd
diff --git a/src/std.c b/src/std.c
index a9064ed..d1acfd8 100644
--- a/src/std.c
+++ b/src/std.c
@@ -89,12 +89,14 @@ SVRCORE_CreateStdPinObj(
     obj->top = top;
   } while(0);
 
+  *out = obj;
+
   if (err != SVRCORE_Success)
   {
     SVRCORE_DestroyStdPinObj(obj);
+    *out = NULL;
   }
 
-  *out = obj;
 
   return err;
 }
diff --git a/src/svrcore.h b/src/svrcore.h
index 1efdbdd..ec91174 100644
--- a/src/svrcore.h
+++ b/src/svrcore.h
@@ -104,6 +104,13 @@ SVRCORE_RegisterPinObj(SVRCOREPinObj *obj);
 SVRCOREPinObj *
 SVRCORE_GetRegisteredPinObj(void);
 
+/* 
+ *  SVRCORE_DestroyRegisteredPinObj - Destroys (frees) the currently registered
+ * pin object, and zeros the pointer. This way a new object can be created
+ */
+void
+SVRCORE_DestroyRegisteredPinObj(void);
+
 /* ------------------------------------------------------------ */
 /*
  * SVRCOREStdPinObj - implementation of SVRCOREPinObj that
-- 
2.5.5

