openvpn-devel Mailing List for OpenVPN

As reported by Bill Parker in trac #600, strdup() return values are not always correctly checked for failed allocations. This patch adds missing checks. Note that in misc.c and options.c, the check is after the dirname() or basename() call, because these can deal with NULL params and we need to keep MSVC happy with its ancient no-declaration-after-statement policy. Signed-off-by: Steffan Karger <st...@ka...> --- src/openvpn/cryptoapi.c | 1 + src/openvpn/init.c | 1 + src/openvpn/misc.c | 1 + src/openvpn/options.c | 1 + src/openvpn/ssl_polarssl.c | 1 + 5 files changed, 5 insertions(+) diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index b7fc11e..62f30be 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -127,6 +127,7 @@ static char *ms_error_text(DWORD ms_err) break; } } + check_malloc_return(rv); return rv; } diff --git a/src/openvpn/init.c b/src/openvpn/init.c index b7c153b..0809cc7 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -817,6 +817,7 @@ init_options_dev (struct options *options) { if (!options->dev && options->dev_node) { char *dev_node = strdup(options->dev_node); /* POSIX basename() implementaions may modify its arguments */ + check_malloc_return(dev_node); options->dev = basename (dev_node); } } diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c index a5bad0d..b543b65 100644 --- a/src/openvpn/misc.c +++ b/src/openvpn/misc.c @@ -1608,6 +1608,7 @@ argv_extract_cmd_name (const char *path) { char *path_cp = strdup(path); /* POSIX basename() implementaions may modify its arguments */ const char *bn = basename (path_cp); + check_malloc_return(path_cp); if (bn)	{ char *ret = string_alloc (bn, NULL); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 2784580..dba7c9d 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -2588,6 +2588,7 @@ check_file_access(const int type, const char *file, const int mode, const char * { char *fullpath = strdup(file); /* POSIX dirname() implementaion may modify its arguments */ char *dirpath = dirname(fullpath); + check_malloc_return(fullpath); if (platform_access (dirpath, mode|X_OK) != 0) errcode = errno; diff --git a/src/openvpn/ssl_polarssl.c b/src/openvpn/ssl_polarssl.c index 3fc811e..673dbbe 100644 --- a/src/openvpn/ssl_polarssl.c +++ b/src/openvpn/ssl_polarssl.c @@ -198,6 +198,7 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers) /* Parse allowed ciphers, getting IDs */ i = 0; tmp_ciphers_orig = tmp_ciphers = strdup(ciphers); + check_malloc_return(tmp_ciphers); token = strtok (tmp_ciphers, ":"); while(token) -- 2.1.4 

Hi, On Tue, Aug 25, 2015 at 02:38:20AM -0400, grarpamp wrote: > Cert serial numbers found in the wild are hardly unique (witness They are not "in the wild" in this context, as the issueing CA in OpenVPN contexts is typically in-house - and serial numbers for certs issued by a single CA(!) *are* unique. If the cert cannot be verified at all (not revoked, but "issued by another CA" or "not signed at all"), I agree, the serial number does not carry meaningful information - but this is not the case this patch is about. gert -- USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany ge...@gr... fax: +49-89-35655025 ge...@ne... 

Hi. I disagree. And openssl crl disagrees with you too. There are no sha1 (or other) fingerprints there, serial numbers are stored there :) As far as I understand in most of the cases where X509 is used for OpenVPN, single (mostly probable self-signed) CA is used for authentication so serial number should be sane enough to understand which is which. Anyway, feel free to add sha1 (you name it) fingerprint additionally to my patch - more information on the cause of denial of connection the better. On 25.08.2015 9:38, grarpamp wrote: > On Mon, Aug 24, 2015 at 12:54 PM, Boris Lytochkin > <lyt...@ya...> wrote: >> Log serial number of revoked certificate >> In most of situations admin of OpenVPN server needs to know which particular >> certificate is used by client. > Cert serial numbers found in the wild are hardly unique (witness > the Mozilla CA bundle), thus no one with a sane mind refers to them > as identifiers, nor do libraries/apps use them for things like cert pinning, > nor should people be encouraged to think they are unique (even though > there may now be some spec for that, but history precedes). The only place > they'd have meaning is as text string for the local issuer, but it's really just > duplication of work. > The sha1 (or better) fingerprint of the cert should be used instead. > > ------------------------------------------------------------------------------ > _______________________________________________ > Openvpn-devel mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openvpn-devel -- Boris Lytochkin Yandex NOC +7 (495) 739 70 00 ext. 7671 

On Mon, Aug 24, 2015 at 12:54 PM, Boris Lytochkin <lyt...@ya...> wrote: > Log serial number of revoked certificate > In most of situations admin of OpenVPN server needs to know which particular > certificate is used by client. Cert serial numbers found in the wild are hardly unique (witness the Mozilla CA bundle), thus no one with a sane mind refers to them as identifiers, nor do libraries/apps use them for things like cert pinning, nor should people be encouraged to think they are unique (even though there may now be some spec for that, but history precedes). The only place they'd have meaning is as text string for the local issuer, but it's really just duplication of work. The sha1 (or better) fingerprint of the cert should be used instead. 

Hi Gert, thanks for clarifying this. Cheers Jirka H. On 08/24/2015 11:10 PM, Gert Doering wrote: > Hi, > > On Mon, Aug 24, 2015 at 09:17:44PM +0200, Jiri Horky wrote: >> thanks for the information. I see you effectively increased the size to >> 1.2k which, as far as I know, will not work for people with dial ups. > If that wouldn't work, openvpn data transfers would not work either - but > surprisingly, they do :-) - data packets are 1500+ bytes in size, unless > specifically told (--link-mtu) to be smaller. We do cap the control channel > MTU to a smaller value to give particularily broken setups (like, 1400 MTU > *and* broken fragments) the chance to push smaller --link-mtu from the > server. > > "Dial-Up", per se, does not imply a smaller-than-1500 MTU - some folks > do that, for "performance reasons" (reducing MTU actually reduces > performance, as it increases TCP overhead, but what do I know against > glossy computer magazine advice...), but even a 574 byte MTU will still > work *if* fragmentation works. > > Which it normally does in this combination - fragmentation tends to break > if stupid NAT routers are involved, but "dialup with modem links and > 574 mtu" is usually not connected to a NAT router but modem->PC... > > But as I said - I'm waiting for feedback. > > gert >