[Swan-dev] Pluto crash with expired certificates

[Paul Wouters]

On Thu, 5 Feb 2015, Wolfgang Nothdurft wrote:

> With commit aac20299b27be6c401cb5d45262a559994e52431 a bug was introduced 
> that causes pluto to crash if an end user certificate is expired.

> The attached patch added the missing return false statement to fix this 
> problem.

I don't understand the patch:

diff --git a/programs/pluto/connections.c b/programs/pluto/connections.c
index 82f6bac..32defad 100644
--- a/programs/pluto/connections.c
+++ b/programs/pluto/connections.c
@@ -823,7 +823,7 @@ static bool load_end_certificate(const char *name,
struct end *dst)
                         if (dst->ca.ptr == NULL)
                                 dst->ca = dst->cert.u.x509->issuer;
                 }
-               break;
+       return FALSE;
         default:
                 bad_case(cert.ty);
         }

If I look at the code:

                 valid_until = cert.u.x509->notAfter;
                 ugh = check_validity(cert.u.x509, &valid_until /* IN/OUT
*/);
                 if (ugh != NULL) {
                         loglog(RC_LOG_SERIOUS,"  %s", ugh);
                         free_x509cert(cert.u.x509);
                 } else {
                         DBG(DBG_CONTROL,
                                 DBG_log("certificate is valid"));
                         add_x509_public_key(&dst->id, cert.u.x509, valid_until,
                                         DAL_LOCAL);
                         dst->cert.ty = cert.ty;
                         dst->cert.u.x509 = add_x509cert(cert.u.x509);

                         /* if no CA is defined, use issuer as default */
                         if (dst->ca.ptr == NULL)
                                 dst->ca = dst->cert.u.x509->issuer;
                 }
                 break;
         default:
                 bad_case(cert.ty);
         }

         return TRUE;
}

if ugh is non-null we have a serious error. So I can see that the return
FALSE goes in there and we currently return TRUE. But with your patch,
regardless of whether ugh returns NULL or not, you return FALSE. The
break seems fine for the "good case" because it will leave the switch,
skipping over the default bad_case() and return TRUE? So I think the
patch should be:

diff --git a/programs/pluto/connections.c b/programs/pluto/connections.c
index 1446849..72600b9 100644
--- a/programs/pluto/connections.c
+++ b/programs/pluto/connections.c
@@ -811,6 +811,7 @@ static bool load_end_certificate(const char *name,
struct end *dst)
                 if (ugh != NULL) {
                         loglog(RC_LOG_SERIOUS,"  %s", ugh);
                         free_x509cert(cert.u.x509);
+                       return FALSE;
                 } else {
                         DBG(DBG_CONTROL,
                                 DBG_log("certificate is valid"));

Paul