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
_______________________________________________
Swan-dev mailing list
[email protected]
https://lists.libreswan.org/mailman/listinfo/swan-dev
