| Message ID | 20230128223421.2207802-3-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Improvements for cryptoapi.c | expand |
Acked-by: Gert Doering <gert@greenie.muc.de> I think this is a useful addition. Code looks good according to the documentation for CertGetNameStringW() and WideCharToMultiByte(). Tested on a MinGW compile (yes, compiles :-) ). Not actually tested on a life windows system, as my "have p12 certs imported, reference them from .ovpn" project is still pending. The 3 new gc_free() are a bit ugly, but unavoidable without either having "gc_free() in the middle of the function" (which we don't do) or restructure more & add "ret = 1 ; goto end" code... so it is what it is. Your patch has been applied to the master and release/2.6 branch. commit ddffcea2922905ec13e0e39239d106e0edbea5de (master) commit 0ccfce24b470d924d4184c8a3de08a50154c5e4c (release/2.6) Author: Selva Nair Date: Sat Jan 28 17:34:19 2023 -0500 cyryptapi.c: log the selected certificate's name Signed-off-by: Selva Nair <selva.nair@gmail.com> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20230128223421.2207802-3-selva.nair@gmail.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26093.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
Hi, ... and apologies for not fixing the Subject: - I saw the typo, it was on my "must fix" list, and then I read too much Microsoft documentation and was distracted... gert
Hi, On Tue, Feb 14, 2023 at 03:00:33PM +0100, Gert Doering wrote: > The 3 new gc_free() are a bit ugly, but unavoidable without either > having "gc_free() in the middle of the function" (which we don't do) or > restructure more & add "ret = 1 ; goto end" code... so it is what it is. I did have a vague dislike for these gc_free(), but it turns out that one of them is actually *wrong*... https://github.com/OpenVPN/openvpn/actions/runs/4174551447/jobs/7228274450 cryptoapi.c: In function `SSL_CTX_use_CryptoAPI_certificate': 426 cryptoapi.c:1072:13: error: incompatible type for argument 1 of `gc_free' 427 1072 | gc_free(gc); 428 | ^~ 429 | | 430 | struct gc_arena 431 In file included from cryptoapi.c:52: 432 buffer.h:1019:26: note: expected `struct gc_arena *' but argument is of type `struct gc_arena' 433 1019 | gc_free(struct gc_arena *a) 434 | ~~~~~~~~~~~~~~~~~^ Sorry for not spotting this before merging - my MinGW build did not excercise the "right" code paths. This is in the "no HAVE_XKEY_PROVIDER" branch, and I build with 3.0.7, so the test build never saw this. Seems I need to merge the "remove pre-3.0 support" patch quickly, so this becomes moot ;-) gert
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index 39eeec1b..e3c0bc99 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -939,12 +939,31 @@ xkey_cng_sign(void *handle, unsigned char *sig, size_t *siglen, const unsigned c #endif /* HAVE_XKEY_PROVIDER */ +static char * +get_cert_name(const CERT_CONTEXT *cc, struct gc_arena *gc) +{ + DWORD len = CertGetNameStringW(cc, CERT_NAME_FRIENDLY_DISPLAY_TYPE, 0, NULL, NULL, 0); + char *name = NULL; + if (len) + { + wchar_t *wname = gc_malloc(len*sizeof(wchar_t), false, gc); + if (!wname + || CertGetNameStringW(cc, CERT_NAME_FRIENDLY_DISPLAY_TYPE, 0, NULL, wname, len) == 0) + { + return NULL; + } + name = utf16to8(wname, gc); + } + return name; +} + int SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) { HCERTSTORE cs; X509 *cert = NULL; CAPI_DATA *cd = calloc(1, sizeof(*cd)); + struct gc_arena gc = gc_new(); if (cd == NULL) { @@ -979,6 +998,13 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) } } + /* try to log the "name" of the selected certificate */ + char *cert_name = get_cert_name(cd->cert_context, &gc); + if (cert_name) + { + msg(D_LOW, "cryptapicert: using certificate with name <%s>", cert_name); + } + /* cert_context->pbCertEncoded is the cert X509 DER encoded. */ cert = d2i_X509(NULL, (const unsigned char **) &cd->cert_context->pbCertEncoded, cd->cert_context->cbCertEncoded); @@ -1022,6 +1048,7 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) EVP_PKEY *privkey = xkey_load_generic_key(tls_libctx, cd, pkey, xkey_cng_sign, (XKEY_PRIVKEY_FREE_fn *) CAPI_DATA_free); SSL_CTX_use_PrivateKey(ssl_ctx, privkey); + gc_free(&gc); return 1; /* do not free cd -- its kept by xkey provider */ #else /* ifdef HAVE_XKEY_PROVIDER */ @@ -1047,12 +1074,14 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) goto err; } CAPI_DATA_free(cd); /* this will do a ref_count-- */ + gc_free(gc); return 1; #endif /* HAVE_XKEY_PROVIDER */ err: CAPI_DATA_free(cd); + gc_free(&gc); return 0; } #endif /* _WIN32 */ diff --git a/src/openvpn/win32-util.c b/src/openvpn/win32-util.c index 35f2a311..32f7a00b 100644 --- a/src/openvpn/win32-util.c +++ b/src/openvpn/win32-util.c @@ -48,6 +48,21 @@ wide_string(const char *utf8, struct gc_arena *gc) return ucs16; } +char * +utf16to8(const wchar_t *utf16, struct gc_arena *gc) +{ + char *utf8 = NULL; + int n = WideCharToMultiByte(CP_UTF8, 0, utf16, -1, NULL, 0, NULL, NULL); + if (n > 0) + { + utf8 = gc_malloc(n, true, gc); + if (utf8) + { + WideCharToMultiByte(CP_UTF8, 0, utf16, -1, utf8, n, NULL, NULL); + } + } + return utf8; +} /* * Return true if filename is safe to be used on Windows, diff --git a/src/openvpn/win32-util.h b/src/openvpn/win32-util.h index b24242c8..ac37979f 100644 --- a/src/openvpn/win32-util.h +++ b/src/openvpn/win32-util.h @@ -34,6 +34,9 @@ /* Convert a string from UTF-8 to UCS-2 */ WCHAR *wide_string(const char *utf8, struct gc_arena *gc); +/* Convert a string from UTF-16 to UTF-8 */ +char *utf16to8(const wchar_t *utf16, struct gc_arena *gc); + /* return true if filename is safe to be used on Windows */ bool win_safe_filename(const char *fn);