[Swan-dev] Pluto crash with expired certificates

[Matt Rogers]

On 02/05, Paul Wouters wrote:
> 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

That will work. This function will change to its NSS version, and I
addressed that in the same way:

 799                 /* check validity of cert */                                                                                                                                                                                                                                
 800                 if (CERT_CheckCertValidTimes(cert.u.nss_cert,                                                                                                                                                                                                               
 801                                              PR_Now(),FALSE) !=                                                                                                                                                                                                             
 802                                                 secCertTimeValid) {                                                                                                                                                                                                         
 803                         loglog(RC_LOG_SERIOUS,"certificate time is expired/invalid");                                                                                                                                                                                       
 804                         CERT_DestroyCertificate(cert.u.nss_cert);                                                                                                                                                                                                           
 805                         return FALSE;                                                                    

Thanks,
Matt