[Swan-dev] Pluto crash with expired certificates

Wolfgang Nothdurft wolfgang at linogate.de
Fri Feb 6 10:02:07 EET 2015


Am 06.02.2015 um 02:55 schrieb 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

Your right, sorry.

That was a hasty reaction right before leaving the office :(

Wolfgang


More information about the Swan-dev mailing list