| Message ID | 20221115122940.1947284-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Fix logic error in checking early negotiation support check | expand |
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 */
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
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
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 --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 */
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(-)