Skip to contentSkip to conversation

Fix potential 1-byte overread in TCP option parsing.

PATCH In review Gert Doering 2 participants 4 messages Jun 18, 2017

lore
Share & tools
lore

Gert Doering

author
#0 lore raw

PATCH 1/1 Fix potential 1-byte overread in TCP option parsing.

A malformed TCP header could lead to a one-byte overread when searching for the MSS option (but as far as we know, with no adverse consequences).

Change outer loop to always ensure there's one extra byte available in the buffer examined.

Technically, this would cause OpenVPN to ignore the only single-byte TCP option available, 'NOP', if it ends up being the very last option in the buffer - so what, it's a NOP anyway, and all we are interested is MSS, which needs 4 bytes.
(https://www.iana.org/assignments/tcp-parameters/tcp-parameters.xhtml)

Found and reported by Guido Vranken <[email protected]...>.

Trac: #745

Signed-off-by Gert Doering <[email protected]...>
---
 src/openvpn/mss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
src/openvpn/mss.c +1 -1
index ff24068..7c596d7 100644
@@ -159,7 +159,7 @@ mss_fixup_dowork(struct buffer *buf, uint16_t maxmss)
 
     for (olen = hlen - sizeof(struct openvpn_tcphdr),
          opt = (uint8_t *)(tc + 1);
-         olen > 0;
+         olen > 1;
          olen -= optlen, opt += optlen)
     {
         if (*opt == OPENVPN_TCPOPT_EOL)
Signature · 1 line
2.10.2
#1 lore raw

Am 18.06.17 um 21:41 schrieb Gert Doering:

Quoted 28 lines

A malformed TCP header could lead to a one-byte overread when searching for the MSS option (but as far as we know, with no adverse consequences).

Change outer loop to always ensure there's one extra byte available in the buffer examined.

Technically, this would cause OpenVPN to ignore the only single-byte TCP option available, 'NOP', if it ends up being the very last option in the buffer - so what, it's a NOP anyway, and all we are interested is MSS, which needs 4 bytes.
(https://www.iana.org/assignments/tcp-parameters/tcp-parameters.xhtml)

Found and reported by Guido Vranken <[email protected]...>.

Trac: #745

Signed-off-by Gert Doering <[email protected]...>
---
 src/openvpn/mss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
src/openvpn/mss.c +0 -0
index ff24068..7c596d7 100644

src/openvpn/mss.c mss_fixup_dowork(struct buffer *buf, uint16_t maxmss) · line 165

Quoted 8 lines
src/openvpn/mss.c +1 -1
@@ -159,7 +159,7 @@ mss_fixup_dowork(struct buffer *buf, uint16_t maxmss)
 
     for (olen = hlen - sizeof(struct openvpn_tcphdr),
          opt = (uint8_t *)(tc + 1);
-         olen > 0;
+         olen > 1;
          olen -= optlen, opt += optlen)
     {
         if (*opt == OPENVPN_TCPOPT_EOL)

ACK. That for is strangely formatted. I have to look very hard to figure out that.

Arne

author
#2 lore raw
In reply to Gert Doering

Patch has been applied to the master, release/2.4, 2.3 and 2.2 branch.

commit 22046a88342878cf43a9a553c83470eeaf97f000 (master) commit 529de430ce07d0c3210a4636b1cb4c89cc6c8fc1 (master) commit 4d343fbe9166e14187775567db00c0a91017df83 (release/2.3) commit 2cc3583ddbc7ce98d859475422cb4e1c217c8083 (release/2.2)

Author: Gert Doering
Date:   Sun Jun 18 21:41:04 2017 +0200
     Fix potential 1-byte overread in TCP option parsing.

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

author
#3 lore raw

Hi,

Quoted 6 lines · Gert Doering

Patch has been applied to the master, release/2.4, 2.3 and 2.2 branch.

commit 22046a88342878cf43a9a553c83470eeaf97f000 (master) commit 529de430ce07d0c3210a4636b1cb4c89cc6c8fc1 (master) commit 4d343fbe9166e14187775567db00c0a91017df83 (release/2.3) commit 2cc3583ddbc7ce98d859475422cb4e1c217c8083 (release/2.2)

The second "master" up there is obviously a "release/2.4" - sorry, and thanks to Arne for noticing. Too many numbers (try opening up all the sub-levels in wireshark for a single TCP SYN, then you understand :-) ).

Here's the correct block:

commit 22046a88342878cf43a9a553c83470eeaf97f000 (master) commit 529de430ce07d0c3210a4636b1cb4c89cc6c8fc1 (release/2.4) commit 4d343fbe9166e14187775567db00c0a91017df83 (release/2.3) commit 2cc3583ddbc7ce98d859475422cb4e1c217c8083 (release/2.2)

(verified against "git branch -v")

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...