Re: [Openvpn-devel] [PATCH v2] Add Windows DNS Leak fix using WFP ('block-outside-dns')

[ValdikSS]

On 16.11.2015 09:17, Selva Nair wrote:
> Hi,
>
> Here are some comments on the code -- there is one apparent memory leak (see 
> below).
>
>
> the word "external" is not required nor appropriate as it blocks all, isn't 
> it?

I'm not a native speaker. If you say that, probably you're right.

>  
>
> This error/warning is unnecessary and confusing. If the user does not want to 
> block-outside-dns, why say "can't block"? And why the "without configured DNS
> server" remark?

That's a garbage. There used to be a check for configured 'dhcp-option DNS' and 
I forgot to remove that. Thanks.

>
> in win32.c
> +
> +ret = ConvertInterfaceIndexToLuid(index, );
> +if (ret == NO_ERROR)
> +dmsg (D_LOW, "Tap Luid: %I64d", tapluid.Value);
> +
>
> Is it safe to ignore the ret != NO_ERROR case? May be it is...
>
>
> openvpnblob is allocated here, but not freed anywhere. Need to free it before 
> return.
>
> Finally, to nitpick,
> There are a couple of unused variables
> Some local vars could be eliminated (rpcStatus, dwFwAPIRetCode, ret)
> FIREWALL_NAME defined as const, but passed to non-const
> ZeroMemory --> CLEAR
> CopyMemory --> memcpy
>
> Regards,
>
> Selva




signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH v3] Notify clients about server's exit/restart

[Lev Stipakov]

Hi,


Since the new server side code does not actually *do* OCC any more we
are just #ifdef'ing it to access options->ce.explicit_exit_notify
because that one is only compiled in #ifdef ENABLE_OCC ... so we're
coupling this new functionality to an #ifdef which is not really
relevant.

No good suggestion yet, just expressing thoughts...



Since connection_entry->explicit_exit_notification is now used also 
outside of OCC context, I suggest to:


1) Remove #ifdef ENABLE_OCC around it's definition.

2) Fix comment there:

/* Explicitly tell peer when we are exiting via OCC_EXIT or control 
channel [RESTART] message */


3) Remove (unneeded anymore) #ifdef in multi_process_signal around 
accessing explicit_exit_notification.


What do you think?

-Lev

Re: [Openvpn-devel] [PATCH v2] Add Windows DNS Leak fix using WFP ('block-outside-dns')

[Selva Nair]

Hi,

Here are some comments on the code -- there is one apparent memory leak
(see below).

 .\"*
>  .TP
> +.B \-\-block\-outside\-dns
> +Block external DNS servers on other network adapters to prevent
>

the word "external" is not required nor appropriate as it blocks all, isn't
it?


> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -1468,6 +1468,22 @@ do_open_tun (struct context *c)
>"up",
>c->c2.es);
>  +#if _WIN32_WINNT >= 0x0600
> +  if (c->options.block_outside_dns)
> +  {
> +  if (!win_wfp_init())
> +msg (M_NONFATAL, "Initialising WFP failed!");
> +else
> +{
> +dmsg (D_LOW, "Blocking outside DNS");
> +if (!win_wfp_block_dns(c->c1.tuntap->adapter_index))
> +msg (M_NONFATAL, "Blocking DNS failed!");
> +}
> +  }
> +  else
> +   msg (M_NONFATAL, "Can't block outside DNS without configured
> DNS server");
>

This error/warning is unnecessary and confusing. If the user does not want
to block-outside-dns, why say "can't block"? And why the "without
configured DNS server" remark?

in win32.c
+
+ret = ConvertInterfaceIndexToLuid(index, );
+if (ret == NO_ERROR)
+dmsg (D_LOW, "Tap Luid: %I64d", tapluid.Value);
+

Is it safe to ignore the ret != NO_ERROR case? May be it is...

+
> +/* Get OpenVPN path. */
> +GetModuleFileNameW(NULL, openvpnpath, MAX_PATH);
> +
> +if (FwpmGetAppIdFromFileName0(openvpnpath, ) !=
> ERROR_SUCCESS)
>

openvpnblob is allocated here, but not freed anywhere. Need to free it
before return.

Finally, to nitpick,
There are a couple of unused variables
Some local vars could be eliminated (rpcStatus, dwFwAPIRetCode, ret)
FIREWALL_NAME defined as const, but passed to non-const
ZeroMemory --> CLEAR
CopyMemory --> memcpy

Regards,

Selva