diff mbox series

[Openvpn-devel] Fix logic error in checking early negotiation support check

Message ID 20221115122940.1947284-1-arne@rfc2549.org
State Accepted
Headers
Series [Openvpn-devel] Fix logic error in checking early negotiation support check |

Commit Message

Arne Schwabe Nov. 15, 2022, 12:29 p.m. UTC 
  We want to check if EARLY_NEG_START is set and reserve the other bits
for future expansions. Right now we also check if all reserved bits are
zero. oops.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/mudp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Antonio Quartulli Nov. 15, 2022, 12:36 p.m. UTC | #1 
Hi,

On 15/11/2022 13:29, Arne Schwabe wrote:
> We want to check if EARLY_NEG_START is set and reserve the other bits
> for future expansions. Right now we also check if all reserved bits are
> zero. oops.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>   src/openvpn/mudp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
> index 7c6fc816e..bdf35a8ba 100644
> --- a/src/openvpn/mudp.c
> +++ b/src/openvpn/mudp.c
> @@ -92,7 +92,7 @@ do_pre_decrypt_check(struct multi_context *m,
>           ASSERT(packet_id_read(&pin, &tmp, true));
>   
>           /* The most significant byte is 0x0f if early negotiation is supported */
> -        bool early_neg_support = (pin.id & EARLY_NEG_MASK) == EARLY_NEG_START;
> +        bool early_neg_support = ((pin.id & EARLY_NEG_MASK) & EARLY_NEG_START) == EARLY_NEG_START;

The "== EARLY_NEG_START" is not needed.

On top of that, my brain parses the expression easier without that part, 
because the question is "after filtering using the mask, is 
EARLY_NEG_START set?".
The "==" imho adds logic which is not needed (both at the code and at 
the brain level).

Cheers,

>   
>           /* All clients that support early negotiation and tls-crypt are assumed
>            * to also support resending the WKc in the 2nd packet */
Arne Schwabe Nov. 16, 2022, 12:54 a.m. UTC | #2 
Am 15.11.2022 um 13:36 schrieb Antonio Quartulli:
> Hi,
>
> On 15/11/2022 13:29, Arne Schwabe wrote:
>> We want to check if EARLY_NEG_START is set and reserve the other bits
>> for future expansions. Right now we also check if all reserved bits are
>> zero. oops.
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>> ---
>>   src/openvpn/mudp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
>> index 7c6fc816e..bdf35a8ba 100644
>> --- a/src/openvpn/mudp.c
>> +++ b/src/openvpn/mudp.c
>> @@ -92,7 +92,7 @@ do_pre_decrypt_check(struct multi_context *m,
>>           ASSERT(packet_id_read(&pin, &tmp, true));
>>             /* The most significant byte is 0x0f if early negotiation 
>> is supported */
>> -        bool early_neg_support = (pin.id & EARLY_NEG_MASK) == 
>> EARLY_NEG_START;
>> +        bool early_neg_support = ((pin.id & EARLY_NEG_MASK) & 
>> EARLY_NEG_START) == EARLY_NEG_START;
>
> The "== EARLY_NEG_START" is not needed.


>
> On top of that, my brain parses the expression easier without that 
> part, because the question is "after filtering using the mask, is 
> EARLY_NEG_START set?".
> The "==" imho adds logic which is not needed (both at the code and at 
> the brain level).

Without the == it is enough if any of the bits EARLY_NEG_START is set 
(0xf00000), we want them all to be set. If EARLY_NEG_START were a 
flag/single bit, you would be right.

Arne
Gert Doering Nov. 16, 2022, 12:30 p.m. UTC | #3 
Acked-by: Gert Doering <gert@greenie.muc.de>

The discussion in the mail thread and on IRC explains why we need
to check the full EARLY_NEG_START value (because it's "0x0f" in the
topmost byte, not "just one bit set").  This is because it was done
that way initially, and now it is what it is...  so what this patch
adds is "ignore the uppermost 4 bits in comparison, should we want
to use these 4 bits for protocol extention in the future".

Arguably one could just do "(pin.id & EARLY_NEG_START) == EARLY_NEG_START"
here, but maybe this way it's clear what is being looked at.

Only compile tested.

Your patch has been applied to the master branch.

commit 543f709f13bca9887cabd4545554539f18346e3c
Author: Arne Schwabe
Date:   Tue Nov 15 13:29:40 2022 +0100

     Fix logic error in checking early negotiation support check

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20221115122940.1947284-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25519.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Antonio Quartulli Nov. 16, 2022, 12:35 p.m. UTC | #4 
Hi,

On 16/11/2022 01:54, Arne Schwabe wrote:
> Without the == it is enough if any of the bits EARLY_NEG_START is set 
> (0xf00000), we want them all to be set. If EARLY_NEG_START were a 
> flag/single bit, you would be right.

Ouch, I indeed assumed it was 1 bit only..

Cheers,
diff mbox series

Patch

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 7c6fc816e..bdf35a8ba 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -92,7 +92,7 @@  do_pre_decrypt_check(struct multi_context *m,
         ASSERT(packet_id_read(&pin, &tmp, true));
 
         /* The most significant byte is 0x0f if early negotiation is supported */
-        bool early_neg_support = (pin.id & EARLY_NEG_MASK) == EARLY_NEG_START;
+        bool early_neg_support = ((pin.id & EARLY_NEG_MASK) & EARLY_NEG_START) == EARLY_NEG_START;
 
         /* All clients that support early negotiation and tls-crypt are assumed
          * to also support resending the WKc in the 2nd packet */