Hi list,
I just noticed I sent my previous reply only to James, instead of to
the list. See below.
On Sat, Mar 5, 2016 at 10:23 AM, Steffan Karger <stef...@karger.me> wrote:
> Hi James,
>
> On Thu, Mar 3, 2016 at 9:19 AM, James Yonan <ja...@openvpn.net> wrote:
>> Signed-off-by: James Yonan <ja...@openvpn.net>
>> ---
>> src/openvpn/ssl_verify_polarssl.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/openvpn/ssl_verify_polarssl.c
>> b/src/openvpn/ssl_verify_polarssl.c
>> index e87d2e2..9d0d086 100644
>> --- a/src/openvpn/ssl_verify_polarssl.c
>> +++ b/src/openvpn/ssl_verify_polarssl.c
>> @@ -176,7 +176,7 @@ unsigned char *
>> x509_get_sha1_hash (x509_crt *cert, struct gc_arena *gc)
>> {
>> unsigned char *sha1_hash = gc_malloc(SHA_DIGEST_LENGTH, false, gc);
>> - sha1(cert->tbs.p, cert->tbs.len, sha1_hash);
>> + sha1(cert->raw.p, cert->raw.len, sha1_hash);
>> return sha1_hash;
>> }
>
> I agree with the change, but this needs a bit more explanation in the
> commit msg and a note in Changes.rst, because it changes
> externally-visible behaviour for PolarSSL/mbedTLS builds (at least
> for --verify-hash, maybe more?).
>
> It took me a bit of time to figure out *why* this is the correct, so
> I'll jot down my findings here.
>
> At first, this didn't not look correct to me. A certificate digest is
> calculated over the TBSCertificate/cert.tbs (TBS = To Be Signed), not
> the Certificate/cert.raw field. This must be, because the Certificate
> field contains the TBSCertificate and the signatureValue (which
> depends on the hash). Also see section 4.1 of RFC5280
> (https://tools.ietf.org/html/rfc5280#page-16).
>
> However, a certificate *fingerprint* (i.e. the output op openssl x509
> -fingerprint) is calculated over the (contents of the) Certificate
> field. And this is what this function should be doing. To make this
> more clear I suggest renaming this function to
> x509_get_sha1_fingerprint(), and updating the doxygen.
>
> -Steffan
So, since Gert is looking into applying these, ACK to the code, and
attached an updated patch with suggestions for the commit msg and
Changes.rst.
-Steffan
From 6f372be44926f555ec9f860746dda8c5e00640f8 Mon Sep 17 00:00:00 2001
From: Steffan Karger <stef...@karger.me>
Date: Mon, 4 Apr 2016 21:59:38 +0200
Subject: [PATCH] PolarSSL x509_get_sha1_hash now returns correct SHA1
fingerprint.
509_get_sha1_hash() is supposed to return the certificate fingerprint,
which is the hash of the entire certificate - including the signature -
and not just the 'to be signed' data (cert->tbs in polarssl).
This changes externally visible behavior for polarssl builds: it will
change the value of the tls_digest_N values exported to the environment
for scripts.
v2 Steffan Karger: added commit message and Changes.rst entry.
Code unchanged from v1 by James.
Signed-off-by: James Yonan <ja...@openvpn.net>
Signed-off-by: Steffan Karger <stef...@karger.me>
---
Changes.rst | 6 +++++-
src/openvpn/ssl_verify_polarssl.c | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/Changes.rst b/Changes.rst
index af70d14..cb900dc 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -63,7 +63,6 @@ User-visible Changes
In --static mode connect-timeout specifies the timeout for TCP and
proxy connection establishment
-
- connect-retry now specifies the maximum number of unsucessfully
trying all remote/connection entries before exiting.
@@ -86,6 +85,11 @@ User-visible Changes
- Removed --enable-password-save from configure. This option is now
always enabled.
+- PolarSSL builds: changed the tls_digest_N values exported to the script
+ environment to be equal to the ones exported by OpenSSL builds, namely
+ the certificate fingerprint (was the hash of the 'to be signed' data).
+
+
Maintainer-visible changes
--------------------------
- OpenVPN no longer supports building with crypto support, but without TLS
diff --git a/src/openvpn/ssl_verify_polarssl.c b/src/openvpn/ssl_verify_polarssl.c
index a2e6a8e..bf1a6ec 100644
--- a/src/openvpn/ssl_verify_polarssl.c
+++ b/src/openvpn/ssl_verify_polarssl.c
@@ -175,7 +175,7 @@ unsigned char *
x509_get_sha1_hash (x509_crt *cert, struct gc_arena *gc)
{
unsigned char *sha1_hash = gc_malloc(SHA_DIGEST_LENGTH, false, gc);
- sha1(cert->tbs.p, cert->tbs.len, sha1_hash);
+ sha1(cert->raw.p, cert->raw.len, sha1_hash);
return sha1_hash;
}
--
2.5.0
