Re: [Openvpn-devel] Log cert serial no if it is revoked

[Boris Lytochkin]

Hi.

v2 patch is attached.

Thanks for comments!

On 20.09.2015 14:28, Steffan Karger wrote:

On 15-09-15 23:49, Steffan Karger wrote:

I agree that the functionality makes, but need to look at the code.  I'm
currently on a long holiday and haven't had enough spare cycles to spend

on openvpn. After I get back (next week), this will be part of my backlog :)

I looked at the code this morning, and tested with both OpenSSL and PolarSSL. The functionality makes sense. The serial formatting is a bit confusing ("serial 12" is actually serial 0x12, or 18 in dec), but this matches 'openssl crl -text' output, and putting 0x in front will make the longer AA:BB:CC formatting look weird... So I guess I'm fine with this.

Regarding
<cut>

backend_x509_get_serial_hex() may return NULL, and passing NULL as a string format specifier to printf() results in undefined behaviour. So please add a NULL check before passing the string to msg().

Finally, when you send a v2 of this patch, could you use 'git format-patch' to create a patch file with a clear commit message explaining why this change is useful? (Your original email probably suffices).

-Steffan

--
Boris Lytochkin
Yandex NOC
+7 (495) 739 70 00 ext. 7671


From e8368ef1b7d18e188dc66c017dc81c7f15f66ab0 Mon Sep 17 00:00:00 2001
From: Boris Lytochkin <lytbo...@yandex-team.ru>
Date: Sun, 20 Sep 2015 17:05:22 +0300
Subject: [PATCH] Log serial number of revoked certificate

In most of situations admin of OpenVPN server needs to know which particular 
certificate is used by client.
In the case when certificate is OK, environment variable can be used for that 
but once it is revoked
no user scripts are invoked so there is no way to get serial number: only 
subject is printed in logs.

So we log certificate serial in case it is revoked.

Sponsored-by: Yandex LLC

Signed-off-by: Boris Lytochkin <lytbo...@yandex-team.ru>
---
 src/openvpn/ssl_verify_openssl.c  | 6 +++++-
 src/openvpn/ssl_verify_polarssl.c | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 81b2e38..bf53522 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -585,6 +585,8 @@ x509_verify_crl(const char *crl_file, X509 *peer_cert, 
const char *subject)
   BIO *in=NULL;
   int n,i;
   result_t retval = FAILURE;
+  struct gc_arena gc = gc_new();
+  char *serial;
 
   in = BIO_new_file (crl_file, "r");
 
@@ -609,7 +611,8 @@ x509_verify_crl(const char *crl_file, X509 *peer_cert, 
const char *subject)
   for (i = 0; i < n; i++) {
     revoked = (X509_REVOKED *)sk_X509_REVOKED_value(X509_CRL_get_REVOKED(crl), 
i);
     if (ASN1_INTEGER_cmp(revoked->serialNumber, 
X509_get_serialNumber(peer_cert)) == 0) {
-      msg (D_HANDSHAKE, "CRL CHECK FAILED: %s is REVOKED",subject);
+      serial = backend_x509_get_serial_hex(peer_cert, &gc);
+      msg (D_HANDSHAKE, "CRL CHECK FAILED: %s (serial %s) is REVOKED", 
subject, (serial ? serial : "NOT AVAILABLE"));
       goto end;
     }
   }
@@ -618,6 +621,7 @@ x509_verify_crl(const char *crl_file, X509 *peer_cert, 
const char *subject)
   msg (D_HANDSHAKE, "CRL CHECK OK: %s",subject);
 
 end:
+  gc_free(&gc);
   BIO_free(in);
   if (crl)
     X509_CRL_free (crl);
diff --git a/src/openvpn/ssl_verify_polarssl.c 
b/src/openvpn/ssl_verify_polarssl.c
index 2edf21d..4852243 100644
--- a/src/openvpn/ssl_verify_polarssl.c
+++ b/src/openvpn/ssl_verify_polarssl.c
@@ -373,6 +373,8 @@ x509_verify_crl(const char *crl_file, x509_crt *cert, const 
char *subject)
 {
   result_t retval = FAILURE;
   x509_crl crl = {0};
+  struct gc_arena gc = gc_new();
+  char *serial;
 
   int polar_retval = x509_crl_parse_file(&crl, crl_file);
   if (polar_retval != 0)
@@ -394,7 +396,8 @@ x509_verify_crl(const char *crl_file, x509_crt *cert, const 
char *subject)
 
   if (0 != x509_crt_revoked(cert, &crl))
     {
-      msg (D_HANDSHAKE, "CRL CHECK FAILED: %s is REVOKED", subject);
+      serial = backend_x509_get_serial_hex(cert, &gc);
+      msg (D_HANDSHAKE, "CRL CHECK FAILED: %s (serial %s) is REVOKED", 
subject, (serial ? serial : "NOT AVAILABLE"));
       goto end;
     }
 
@@ -402,6 +405,7 @@ x509_verify_crl(const char *crl_file, x509_crt *cert, const 
char *subject)
   msg (D_HANDSHAKE, "CRL CHECK OK: %s",subject);
 
 end:
+  gc_free(&gc);
   x509_crl_free(&crl);
   return retval;
 }
-- 
2.1.2