diff mbox series

[Openvpn-devel] Do not error when md_kt_size() is called with mdname="none"

Message ID 20220121185752.14138-1-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] Do not error when md_kt_size() is called with mdname="none" | expand

Commit Message

Selva Nair Jan. 21, 2022, 7:57 a.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

An easy way to trigger this error is to run an otherwise working setup
(at say verb = 4) with increased verbosity of verb >= 7 and using a GCM 
cipher (e.g., AES-256-GCM). It will cause a fatal exit while printing the 
cipher and hmac in key2_print().

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
Its actually md_get("none") called by md_kt_size("none") that
causes the error and I'm not entirely sure whether we should
instead make md_get("none") to return NULL. But that would
require all its callers to check for NULL.

 src/openvpn/crypto_openssl.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Arne Schwabe Jan. 21, 2022, 3:38 p.m. UTC | #1 
Am 21.01.22 um 19:57 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
> 
> An easy way to trigger this error is to run an otherwise working setup
> (at say verb = 4) with increased verbosity of verb >= 7 and using a GCM
> cipher (e.g., AES-256-GCM). It will cause a fatal exit while printing the
> cipher and hmac in key2_print().
> 
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
> Its actually md_get("none") called by md_kt_size("none") that
> causes the error and I'm not entirely sure whether we should
> instead make md_get("none") to return NULL. But that would
> require all its callers to check for NULL.

Thanks for finding this.

Acked-By: Arne Schwabe <arne@rfc2549.org>
Antonio Quartulli Jan. 25, 2022, 5:27 a.m. UTC | #2 
Hi,

On 21/01/2022 19:57, selva.nair@gmail.com wrote:
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index 35fb0052..b93c680a 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -1073,6 +1073,10 @@ md_kt_name(const char *mdname)
>   unsigned char
>   md_kt_size(const char *mdname)
>   {
> +    if (!strcmp("none", mdname))

Since we have it implemented, can we use streq() here?

Cheers,
Antonio Quartulli Jan. 25, 2022, 5:34 a.m. UTC | #3 
Hi,

On 25/01/2022 17:30, Arne Schwabe wrote:
> Am 25.01.22 um 17:27 schrieb Antonio Quartulli:
>> Hi,
>>
>> On 21/01/2022 19:57, selva.nair@gmail.com wrote:
>>> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
>>> index 35fb0052..b93c680a 100644
>>> --- a/src/openvpn/crypto_openssl.c
>>> +++ b/src/openvpn/crypto_openssl.c
>>> @@ -1073,6 +1073,10 @@ md_kt_name(const char *mdname)
>>>   unsigned char
>>>   md_kt_size(const char *mdname)
>>>   {
>>> +    if (!strcmp("none", mdname))
>>
>> Since we have it implemented, can we use streq() here?
>>
>> Cheers,
>>
> 
> streq is mainly used in option.c only, almost all other codes uses 
> strcmp insteads, so not really that strong of an argument.

I thought options.c was part of this project too :-D
To be honest, I think streq() is not really useful, but then we should 
either use it consistently or just drop it.

I am in favour of dropping it because including options.h in other .c 
files can be quite a nightmare and you don't want that just to use streq().

So I guess we can live with this patch, but should we put on the agenda 
wiping streq() for good?

Cheers,

> 
> Arne
>
Selva Nair Jan. 25, 2022, 6:01 a.m. UTC | #4 
Hi,

On Tue, Jan 25, 2022 at 11:35 AM Antonio Quartulli <a@unstable.cc> wrote:

> Hi,
>
> On 25/01/2022 17:30, Arne Schwabe wrote:
> > Am 25.01.22 um 17:27 schrieb Antonio Quartulli:
> >> Hi,
> >>
> >> On 21/01/2022 19:57, selva.nair@gmail.com wrote:
> >>> diff --git a/src/openvpn/crypto_openssl.c
> b/src/openvpn/crypto_openssl.c
> >>> index 35fb0052..b93c680a 100644
> >>> --- a/src/openvpn/crypto_openssl.c
> >>> +++ b/src/openvpn/crypto_openssl.c
> >>> @@ -1073,6 +1073,10 @@ md_kt_name(const char *mdname)
> >>>   unsigned char
> >>>   md_kt_size(const char *mdname)
> >>>   {
> >>> +    if (!strcmp("none", mdname))
> >>
> >> Since we have it implemented, can we use streq() here?
> >>
> >> Cheers,
> >>
> >
> > streq is mainly used in option.c only, almost all other codes uses
> > strcmp insteads, so not really that strong of an argument.
>
> I thought options.c was part of this project too :-D
> To be honest, I think streq() is not really useful, but then we should
> either use it consistently or just drop it.
>
> I am in favour of dropping it because including options.h in other .c
> files can be quite a nightmare and you don't want that just to use streq().
>
> So I guess we can live with this patch, but should we put on the agenda
> wiping streq() for good?
>

"live with" ? When did strcmp() become a pariah?

Though strcmp()'s naming and return value are arguably not intuitive, we
are all used to it. That said, we have 472 uses of streq(), so let it be.
Just do not require that one should be preferred over the other.

Consistency is a good goal, but let's not be pedantic about it.

Selva
<div dir="ltr"><div>Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 25, 2022 at 11:35 AM Antonio Quartulli &lt;<a href="mailto:a@unstable.cc" target="_blank">a@unstable.cc</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On 25/01/2022 17:30, Arne Schwabe wrote:<br>
&gt; Am 25.01.22 um 17:27 schrieb Antonio Quartulli:<br>
&gt;&gt; Hi,<br>
&gt;&gt;<br>
&gt;&gt; On 21/01/2022 19:57, <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a> wrote:<br>
&gt;&gt;&gt; diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c<br>
&gt;&gt;&gt; index 35fb0052..b93c680a 100644<br>
&gt;&gt;&gt; --- a/src/openvpn/crypto_openssl.c<br>
&gt;&gt;&gt; +++ b/src/openvpn/crypto_openssl.c<br>
&gt;&gt;&gt; @@ -1073,6 +1073,10 @@ md_kt_name(const char *mdname)<br>
&gt;&gt;&gt;   unsigned char<br>
&gt;&gt;&gt;   md_kt_size(const char *mdname)<br>
&gt;&gt;&gt;   {<br>
&gt;&gt;&gt; +    if (!strcmp(&quot;none&quot;, mdname))<br>
&gt;&gt;<br>
&gt;&gt; Since we have it implemented, can we use streq() here?<br>
&gt;&gt;<br>
&gt;&gt; Cheers,<br>
&gt;&gt;<br>
&gt; <br>
&gt; streq is mainly used in option.c only, almost all other codes uses <br>
&gt; strcmp insteads, so not really that strong of an argument.<br>
<br>
I thought options.c was part of this project too :-D<br>
To be honest, I think streq() is not really useful, but then we should <br>
either use it consistently or just drop it.<br>
<br>
I am in favour of dropping it because including options.h in other .c <br>
files can be quite a nightmare and you don&#39;t want that just to use streq().<br>
<br>
So I guess we can live with this patch, but should we put on the agenda <br>
wiping streq() for good?<br></blockquote><div><br></div><div>&quot;live with&quot; ? When did strcmp() become a pariah? </div><div><br></div><div>Though strcmp()&#39;s naming and return value are arguably not intuitive, we are all used to it. That said, we have 472 uses of streq(), so let it be. Just do not require that one should be preferred over the other.</div><div><br></div><div>Consistency is a good goal, but let&#39;s not be pedantic about it.</div><div><br></div><div>Selva</div></div></div>
Antonio Quartulli Jan. 25, 2022, 9:34 a.m. UTC | #5 
Hi,

On 25/01/2022 18:01, Selva Nair wrote:
> Hi,
> 
> On Tue, Jan 25, 2022 at 11:35 AM Antonio Quartulli <a@unstable.cc 
> <mailto:a@unstable.cc>> wrote:
> 
>     Hi,
> 
>     On 25/01/2022 17:30, Arne Schwabe wrote:
>      > Am 25.01.22 um 17:27 schrieb Antonio Quartulli:
>      >> Hi,
>      >>
>      >> On 21/01/2022 19:57, selva.nair@gmail.com
>     <mailto:selva.nair@gmail.com> wrote:
>      >>> diff --git a/src/openvpn/crypto_openssl.c
>     b/src/openvpn/crypto_openssl.c
>      >>> index 35fb0052..b93c680a 100644
>      >>> --- a/src/openvpn/crypto_openssl.c
>      >>> +++ b/src/openvpn/crypto_openssl.c
>      >>> @@ -1073,6 +1073,10 @@ md_kt_name(const char *mdname)
>      >>>   unsigned char
>      >>>   md_kt_size(const char *mdname)
>      >>>   {
>      >>> +    if (!strcmp("none", mdname))
>      >>
>      >> Since we have it implemented, can we use streq() here?
>      >>
>      >> Cheers,
>      >>
>      >
>      > streq is mainly used in option.c only, almost all other codes uses
>      > strcmp insteads, so not really that strong of an argument.
> 
>     I thought options.c was part of this project too :-D
>     To be honest, I think streq() is not really useful, but then we should
>     either use it consistently or just drop it.
> 
>     I am in favour of dropping it because including options.h in other .c
>     files can be quite a nightmare and you don't want that just to use
>     streq().
> 
>     So I guess we can live with this patch, but should we put on the agenda
>     wiping streq() for good?
> 
> 
> "live with" ? When did strcmp() become a pariah?

Sorry, I didn't mean to have an offensive behaviour. Maybe I chose the 
wrong language.

What I meant was: since we already have a mix of strcmp and streq in the 
code and since Arne sounded reluctant to suggest changing this patch, I 
basically wanted to agree with keeping this patch as is. (read "live 
with" as "accept" or "ack").

> 
> Though strcmp()'s naming and return value are arguably not intuitive, we 
> are all used to it. That said, we have 472 uses of streq(), so let it 
> be. Just do not require that one should be preferred over the other.
> 

I am all in favour of simply using strcmp. It's just that our code is a 
partial mix up and that is not nice (imho). This is why I suggested that 
we should put on the agenda wiping streq entirely and be done with it.

> Consistency is a good goal, but let's not be pedantic about it.

True, but, if not pursued, consistency easily become inconsistency and 
at some point we are forced to clean things up.

Now, the streq/strcmp dilemma is just a marginal thing compared to other 
inconsistencies we have and I did not want to make a big deal about it.
Sorry if it sounded so.


Cheers,
Gert Doering Jan. 26, 2022, 2:47 a.m. UTC | #6 
Patch + explanation make sense and Arne indeed ACKed this.  Thanks.

Reproduced the problem (GCM + verb7):

   2022-01-26 14:40:34 us=15705 Data Channel: using negotiated cipher 'AES-256-GCM'
   ...
   2022-01-26 14:40:34 us=15829 Message hash algorithm 'none' not found
   2022-01-26 14:40:34 us=15849 Exiting due to fatal error

and can see the fix work

   2022-01-26 14:42:38 us=355685 Data Channel: using negotiated cipher 'AES-256-GCM'
   ...
   2022-01-26 14:42:38 us=355782 Master Encrypt (cipher): 0ba0d2b3 83f3f160 3ff8fe87 11c22db7 dc67c1fd 7f9ba3f0 9077f9e9 7ee77cf2
   2022-01-26 14:42:38 us=355791 Master Encrypt (hmac): 
   ...
   2022-01-26 14:42:38 us=355897 Incoming Data Channel: CIPHER block_size=16 iv_size=12
   2022-01-26 14:42:38 us=356020 ROUTE_GATEWAY 193.149.48.190/255.255.255.192 IFACE=bge0 HWADDR=3c:a8:2a:9f:70:90


Your patch has been applied to the master branch.

commit 9076d90684c06c8f0ac0ba4c099b00ddbc4ab29d
Author: Selva Nair
Date:   Fri Jan 21 13:57:52 2022 -0500

     Do not error when md_kt_size() is called with mdname=none

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20220121185752.14138-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23631.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 35fb0052..b93c680a 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -1073,6 +1073,10 @@  md_kt_name(const char *mdname)
 unsigned char
 md_kt_size(const char *mdname)
 {
+    if (!strcmp("none", mdname))
+    {
+        return 0;
+    }
     evp_md_type *kt = md_get(mdname);
     unsigned char size =  (unsigned char)EVP_MD_size(kt);
     EVP_MD_free(kt);