Re: LibreSSL legacy verifier regression
On Wed, Feb 24, 2021 at 09:21:56PM +0100, Theo Buehler wrote:
> On Wed, Feb 24, 2021 at 09:00:05PM +0100, Theo Buehler wrote:
> > On Wed, Feb 24, 2021 at 06:47:00AM +0100, Jan Klemkow wrote:
> > > another co-worker of mine has found an other regress in the LibreSSL
> > > legacy verifier. I took his diff and made a test for our regression
> > > framework.
> > >
> > > The legacy verifier seems not to check the certificate if no root CA was
> > > given. The following test creates an expired certificate and tries to
> > > verify it. In one case it found the expected error in another, it does
> > > not.
> >
> > Thanks for the report and the test case, that's very helpful. The diff
> > at the end addresses this.
> >
> > The verifier does not find the expected error because it now bails out
> > earlier. This is a consequence of a refactoring of X509_verify_cert()
> > (x509_vfy.c r1.75) that was done to integrate the new verifier.
> >
> > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libcrypto/x509/x509_vfy.c.diff?r1=1.74&r2=1.75
> >
> > What happens is that x509_legacy_verify_build_chain() returns ok == 0 in
> > your test case. The safety net at the end of x509_verify_cert_legacy()
> > sets ctx->error to X509_V_ERR_UNSPECIFIED (so the unchecked call to
> > X509_verify_cert() in your regress test actually indicates verification
> > failure).
> >
> >
> > The diff below restores the previous behavior and fixes a bug.
> >
> > Prior to the the refactoring, each 'goto end' in the code that is now in
> > x509_legacy_verify_build_chain() would stop validation, while in other
> > cases validation would have carried on. So indicate this via the return
> > value and return ok via a pointer.
> >
> > The bug is that the return value check of x509_legacy_verify_build_chain()
> > should have been if (ok <= 0), not if (!ok).
> >
> > Regarding your regress diff: I don't think we want to land it as it is.
Ok.
> > The verifier lives in libcrypto/x509, so the regress test belongs in
> > there.
You are right, its the better place. At least I want to send you a bug
report with concrete code to test.
> > We really need to come up with an extensible design that can check a
> > number of such cases (and ideally includes the bulk of your openssl/x509
> > regress tests). I don't want to add a directory for every bug in the
> > verifier or legacy verifier. As jsing already mentioned, I expect that
> > we want to commit the test certs so we don't need perl modules from
> > ports to run the regress. Then we want to add generating scripts and a
> > README that gives instructions on how to regenerate the certs if needed.
> >
> > I would like to have one C program that runs all tests in a loop (or
> > perhaps one C program per family of regressions). It would be nice if
> > this C program could also be compiled against OpenSSL 1.1.1 so we can
> > easily check for differences of behavior (see x509/bettertls/Makefile
> > for an example that does this). For your test program this just means:
> > don't access csc->blah, but use X509_STORE_CTX_get_blah(csc) instead.
> >
> > Why does the test set TRUSTED_FIRST?
I just forget to remove the this line, from the original version.
> > The code also needs a bit of cleaning. There are a number of unchecked
> > return values, for example strdup and sk_*_push, and csc is leaked
> > after X509_verify_cert().
> >
> > It would also be nice to run this test against the new verifier.
The test passes with the new verifier in current, but not in 6.8.
> Missed an obvious simplification.
The diff looks fine to me and it fixes our regressions.
I would give you an OK jan, fwiw.
Thanks,
Jan
> Index: x509/x509_vfy.c
> ===
> RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 x509_vfy.c
> --- x509/x509_vfy.c 11 Feb 2021 04:56:43 - 1.85
> +++ x509/x509_vfy.c 24 Feb 2021 20:19:34 -
> @@ -240,12 +240,13 @@ x509_vfy_check_id(X509_STORE_CTX *ctx) {
> * Oooh..
> */
> static int
> -X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad)
> +X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad, int
> *out_ok)
> {
> X509 *x, *xtmp, *xtmp2, *chain_ss = NULL;
> int bad_chain = 0;
> X509_VERIFY_PARAM *param = ctx->param;
> - int depth, i, ok = 0;
> + int ok = 0, ret = 0;
> + int depth, i;
> int num, j, retry, trust;
> int (*cb) (int xok, X509_STORE_CTX *xctx);
> STACK_OF(X509) *sktmp = NULL;
> @@ -517,11 +518,15 @@ X509_verify_cert_legacy_build_chain(X509
> if (!ok)
> goto end;
> }
> +
> + ret = 1;
> end:
> sk_X509_free(sktmp);
> X509_free(chain_ss);
> *bad = bad_chain;
> - return ok;
> + *out_ok = ok;
> +
> + return ret;
> }
>
> static int
> @@ -531,8 +536,7 @@ X509_verify_cert_legacy(X509_STORE_CTX *
>
> ct
Re: LibreSSL legacy verifier regression
On Wed, Feb 24, 2021 at 09:00:05PM +0100, Theo Buehler wrote:
> On Wed, Feb 24, 2021 at 06:47:00AM +0100, Jan Klemkow wrote:
> > Hi,
> >
> > another co-worker of mine has found an other regress in the LibreSSL
> > legacy verifier. I took his diff and made a test for our regression
> > framework.
> >
> > The legacy verifier seems not to check the certificate if no root CA was
> > given. The following test creates an expired certificate and tries to
> > verify it. In one case it found the expected error in another, it does
> > not.
>
> Thanks for the report and the test case, that's very helpful. The diff
> at the end addresses this.
>
> The verifier does not find the expected error because it now bails out
> earlier. This is a consequence of a refactoring of X509_verify_cert()
> (x509_vfy.c r1.75) that was done to integrate the new verifier.
>
> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libcrypto/x509/x509_vfy.c.diff?r1=1.74&r2=1.75
>
> What happens is that x509_legacy_verify_build_chain() returns ok == 0 in
> your test case. The safety net at the end of x509_verify_cert_legacy()
> sets ctx->error to X509_V_ERR_UNSPECIFIED (so the unchecked call to
> X509_verify_cert() in your regress test actually indicates verification
> failure).
>
>
> The diff below restores the previous behavior and fixes a bug.
>
> Prior to the the refactoring, each 'goto end' in the code that is now in
> x509_legacy_verify_build_chain() would stop validation, while in other
> cases validation would have carried on. So indicate this via the return
> value and return ok via a pointer.
>
> The bug is that the return value check of x509_legacy_verify_build_chain()
> should have been if (ok <= 0), not if (!ok).
>
>
> Regarding your regress diff: I don't think we want to land it as it is.
>
> The verifier lives in libcrypto/x509, so the regress test belongs in
> there.
>
> We really need to come up with an extensible design that can check a
> number of such cases (and ideally includes the bulk of your openssl/x509
> regress tests). I don't want to add a directory for every bug in the
> verifier or legacy verifier. As jsing already mentioned, I expect that
> we want to commit the test certs so we don't need perl modules from
> ports to run the regress. Then we want to add generating scripts and a
> README that gives instructions on how to regenerate the certs if needed.
>
> I would like to have one C program that runs all tests in a loop (or
> perhaps one C program per family of regressions). It would be nice if
> this C program could also be compiled against OpenSSL 1.1.1 so we can
> easily check for differences of behavior (see x509/bettertls/Makefile
> for an example that does this). For your test program this just means:
> don't access csc->blah, but use X509_STORE_CTX_get_blah(csc) instead.
>
> Why does the test set TRUSTED_FIRST?
>
> The code also needs a bit of cleaning. There are a number of unchecked
> return values, for example strdup and sk_*_push, and csc is leaked
> after X509_verify_cert().
>
> It would also be nice to run this test against the new verifier.
Missed an obvious simplification.
Index: x509/x509_vfy.c
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
retrieving revision 1.85
diff -u -p -r1.85 x509_vfy.c
--- x509/x509_vfy.c 11 Feb 2021 04:56:43 - 1.85
+++ x509/x509_vfy.c 24 Feb 2021 20:19:34 -
@@ -240,12 +240,13 @@ x509_vfy_check_id(X509_STORE_CTX *ctx) {
* Oooh..
*/
static int
-X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad)
+X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad, int *out_ok)
{
X509 *x, *xtmp, *xtmp2, *chain_ss = NULL;
int bad_chain = 0;
X509_VERIFY_PARAM *param = ctx->param;
- int depth, i, ok = 0;
+ int ok = 0, ret = 0;
+ int depth, i;
int num, j, retry, trust;
int (*cb) (int xok, X509_STORE_CTX *xctx);
STACK_OF(X509) *sktmp = NULL;
@@ -517,11 +518,15 @@ X509_verify_cert_legacy_build_chain(X509
if (!ok)
goto end;
}
+
+ ret = 1;
end:
sk_X509_free(sktmp);
X509_free(chain_ss);
*bad = bad_chain;
- return ok;
+ *out_ok = ok;
+
+ return ret;
}
static int
@@ -531,8 +536,7 @@ X509_verify_cert_legacy(X509_STORE_CTX *
ctx->error = X509_V_OK; /* Initialize to OK */
- ok = X509_verify_cert_legacy_build_chain(ctx, &bad_chain);
- if (!ok)
+ if (!X509_verify_cert_legacy_build_chain(ctx, &bad_chain, &ok))
goto end;
/* We have the chain complete: now we need to check its purpose */
Re: LibreSSL legacy verifier regression
On Wed, Feb 24, 2021 at 06:47:00AM +0100, Jan Klemkow wrote:
> Hi,
>
> another co-worker of mine has found an other regress in the LibreSSL
> legacy verifier. I took his diff and made a test for our regression
> framework.
>
> The legacy verifier seems not to check the certificate if no root CA was
> given. The following test creates an expired certificate and tries to
> verify it. In one case it found the expected error in another, it does
> not.
Thanks for the report and the test case, that's very helpful. The diff
at the end addresses this.
The verifier does not find the expected error because it now bails out
earlier. This is a consequence of a refactoring of X509_verify_cert()
(x509_vfy.c r1.75) that was done to integrate the new verifier.
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libcrypto/x509/x509_vfy.c.diff?r1=1.74&r2=1.75
What happens is that x509_legacy_verify_build_chain() returns ok == 0 in
your test case. The safety net at the end of x509_verify_cert_legacy()
sets ctx->error to X509_V_ERR_UNSPECIFIED (so the unchecked call to
X509_verify_cert() in your regress test actually indicates verification
failure).
The diff below restores the previous behavior and fixes a bug.
Prior to the the refactoring, each 'goto end' in the code that is now in
x509_legacy_verify_build_chain() would stop validation, while in other
cases validation would have carried on. So indicate this via the return
value and return ok via a pointer.
The bug is that the return value check of x509_legacy_verify_build_chain()
should have been if (ok <= 0), not if (!ok).
Regarding your regress diff: I don't think we want to land it as it is.
The verifier lives in libcrypto/x509, so the regress test belongs in
there.
We really need to come up with an extensible design that can check a
number of such cases (and ideally includes the bulk of your openssl/x509
regress tests). I don't want to add a directory for every bug in the
verifier or legacy verifier. As jsing already mentioned, I expect that
we want to commit the test certs so we don't need perl modules from
ports to run the regress. Then we want to add generating scripts and a
README that gives instructions on how to regenerate the certs if needed.
I would like to have one C program that runs all tests in a loop (or
perhaps one C program per family of regressions). It would be nice if
this C program could also be compiled against OpenSSL 1.1.1 so we can
easily check for differences of behavior (see x509/bettertls/Makefile
for an example that does this). For your test program this just means:
don't access csc->blah, but use X509_STORE_CTX_get_blah(csc) instead.
Why does the test set TRUSTED_FIRST?
The code also needs a bit of cleaning. There are a number of unchecked
return values, for example strdup and sk_*_push, and csc is leaked
after X509_verify_cert().
It would also be nice to run this test against the new verifier.
Index: x509/x509_vfy.c
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
retrieving revision 1.85
diff -u -p -r1.85 x509_vfy.c
--- x509/x509_vfy.c 11 Feb 2021 04:56:43 - 1.85
+++ x509/x509_vfy.c 24 Feb 2021 19:09:03 -
@@ -240,7 +240,7 @@ x509_vfy_check_id(X509_STORE_CTX *ctx) {
* Oooh..
*/
static int
-X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad)
+X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad, int *out_ok)
{
X509 *x, *xtmp, *xtmp2, *chain_ss = NULL;
int bad_chain = 0;
@@ -517,11 +517,21 @@ X509_verify_cert_legacy_build_chain(X509
if (!ok)
goto end;
}
+
+ sk_X509_free(sktmp);
+ X509_free(chain_ss);
+ *bad = bad_chain;
+ *out_ok = ok;
+
+ return 1;
+
end:
sk_X509_free(sktmp);
X509_free(chain_ss);
*bad = bad_chain;
- return ok;
+ *out_ok = ok;
+
+ return 0;
}
static int
@@ -531,8 +541,7 @@ X509_verify_cert_legacy(X509_STORE_CTX *
ctx->error = X509_V_OK; /* Initialize to OK */
- ok = X509_verify_cert_legacy_build_chain(ctx, &bad_chain);
- if (!ok)
+ if (!X509_verify_cert_legacy_build_chain(ctx, &bad_chain, &ok))
goto end;
/* We have the chain complete: now we need to check its purpose */
LibreSSL legacy verifier regression
Hi,
another co-worker of mine has found an other regress in the LibreSSL
legacy verifier. I took his diff and made a test for our regression
framework.
The legacy verifier seems not to check the certificate if no root CA was
given. The following test creates an expired certificate and tries to
verify it. In one case it found the expected error in another, it does
not.
OK?
bye,
Jan
Index: lib/libcrypto/Makefile
===
RCS file: /cvs/src/regress/lib/libcrypto/Makefile,v
retrieving revision 1.41
diff -u -p -r1.41 Makefile
--- lib/libcrypto/Makefile 26 Dec 2020 00:48:56 - 1.41
+++ lib/libcrypto/Makefile 24 Feb 2021 05:29:51 -
@@ -23,6 +23,7 @@ SUBDIR += ecdsa
SUBDIR += engine
SUBDIR += evp
SUBDIR += exp
+SUBDIR += expcert
SUBDIR += free
SUBDIR += gcm128
SUBDIR += gost
Index: lib/libcrypto/expcert/Makefile
===
RCS file: lib/libcrypto/expcert/Makefile
diff -N lib/libcrypto/expcert/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ lib/libcrypto/expcert/Makefile 24 Feb 2021 05:39:38 -
@@ -0,0 +1,29 @@
+# $OpenBSD$
+
+LDFLAGS += -lcrypto
+
+PROG = expcrt
+
+PKG_REQUIRE != pkg_info -e 'p5-IO-Socket-SSL-*'
+.if empty (PKG_REQUIRE)
+regress:
+ @echo "missing package p5-IO-Socket-SSL"
+ @echo SKIPPED
+.endif
+
+REGRESS_TARGETS = test-chain-with-root-CA
+REGRESS_TARGETS += test-chain-without-root-CA
+REGRESS_SETUP_ONCE = create-certs
+
+REGRESS_EXPECTED_FAILURES = test-chain-without-root-CA
+
+create-certs: create-certs.pl ${PROG}
+ perl ${.CURDIR}/create-certs.pl
+
+test-chain-with-root-CA:
+ ./expcrt -e 10 -r
+
+test-chain-without-root-CA:
+ ./expcrt -e 10
+
+.include
Index: lib/libcrypto/expcert/create-certs.pl
===
RCS file: lib/libcrypto/expcert/create-certs.pl
diff -N lib/libcrypto/expcert/create-certs.pl
--- /dev/null 1 Jan 1970 00:00:00 -
+++ lib/libcrypto/expcert/create-certs.pl 24 Feb 2021 05:27:46 -
@@ -0,0 +1,46 @@
+#!/usr/bin/perl
+
+# Copyright (c) 2021 Anton Borowka
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+use strict;
+use warnings;
+
+use IO::Socket::SSL::Utils;
+
+my %certs;
+
+@{$certs{root}}{qw/cert key/} = CERT_create(
+CA => 1,
+not_after => time() + 31536,
+subject => { commonName => 'Root CA' },
+);
+
+@{$certs{intermediate}}{qw/cert key/} = CERT_create(
+CA => 1,
+issuer => [@{$certs{root}}{qw/cert key/}],
+not_after => time() + 31536,
+subject => { commonName => 'Intermediate CA' },
+);
+
+@{$certs{expired}}{qw/cert key/} = CERT_create(
+issuer => [@{$certs{intermediate}}{qw/cert key/}],
+not_before => time() - 7200,
+not_after => time() - 3600,
+subject => { commonName => 'Expired' },
+);
+
+for (sort keys %certs) {
+PEM_cert2file($certs{$_}{cert}, "$_.crt");
+}
Index: lib/libcrypto/expcert/expcrt.c
===
RCS file: lib/libcrypto/expcert/expcrt.c
diff -N lib/libcrypto/expcert/expcrt.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ lib/libcrypto/expcert/expcrt.c 24 Feb 2021 05:27:46 -
@@ -0,0 +1,218 @@
+/* $OpenBSD$ */
+/*
+ * Copyright (c) 2021 Jan Klemkow
+ * Copyright (c) 2021 Anton Borowka
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+int error_found = 0;
+int expected_error = 0;
+
+static int
+x509verify_error_handle
