1 byte overread in mss_fixup_dowork

[gvranken]

make openvpn-2.3.12 like this:

CFLAGS="-fsanitize=address" ./configure && make

Call mss_fixup_ipv4 directly:

    unsigned char payload[] = {
        0x3e, 0x28, 0x00, 0x54, 0x3a, 0x51, 0x60, 0x00, 0x21, 0x06, 0x89, 0x27,
        0xde, 0x21, 0x27, 0xf1, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x09, 0x01,
        0x01, 0x01, 0x01, 0x01, 0x11, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
        0x01, 0x01, 0x01, 0x01, 0x01, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e,
        0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e,
        0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e,
        0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x33
    };
    unsigned int payload_len = 84;
    struct gc_arena gc = gc_new ();
    struct buffer b = alloc_buf_gc (payload_len, &gc);
    memcpy(b.data, payload, payload_len);
    b.len += payload_len;
    mss_fixup_ipv4(&b, 100);
    gc_free (&gc);

This should trigger ASAN. I haven't attempted to trigger this remotely with an actual OpenVPN instance, but it's probably possible?

Proposed patch:

diff --git a/mss.c b/mss.c
index 7298c7b..715d837 100644
--- a/mss.c
+++ b/mss.c
@@ -153,6 +153,9 @@ mss_fixup_dowork (struct buffer *buf, uint16_t maxmss)
     else if (*opt == OPENVPN_TCPOPT_NOP)
       optlen = 1;
     else {
+        if ( optlen == 1 ) {
+            break;
+        }
       optlen = *(opt + 1);
       if (optlen <= 0 || optlen > olen)
         break;

[Gert Döring]

"should" it trigger ASAN or *does* it trigger ASAN?

Your change to the code does not make much sense - reaching that else clause, the code looks at the length of the *previous* TCP option, which has no relevance at this point.

What this code does is "walk the TCP option chain to find the TCP MSS option" (TCPOPT_MAXSEG) - so with your change, it would stop the walk if it hits an TCPOPT_NOP, and ignore all further options. So this might be safe, but it will also just no longer work (also, you access an uninitialized variable if the first option is not a NOP).

Which read exactly is overflowing? From a cursory glance, I see that the header chain walking is carefully checked, and if we overrun "olen" (end of buffer) we immediately exit. So I think the only bit that might be overreading by one is the optlen = *(opt+1) part - a fix for that which would not break option chains with a NOP in front would be to change the olen > 0 if() condition to olen > 1 .

[gvranken]

The given payload triggers ASAN, because of the overread, which occurs exactly here:

optlen = *(opt + 1);

And you're right, checking olen is the way to go.

[Gert Döring]

Oh, wow, this really got stuck. And you came back with more good findings this time... :-)

Anyway. Have sent a patch to the openvpn-devel list (what we agreed already, 7 months ago)...

--- a/src/openvpn/mss.c
+++ b/src/openvpn/mss.c
@@ -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)

​https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14874.html

[Gert Döring]

And merged!

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.

(Arne was really quick with that ACK)

Guido, can you confirm? Then we can close this, after way too long...

[gvranken]

Hereby confirmed that your patch resolves the issue.