Problem with 802.1Q marked frames with OpenVPN Client

[stepman0]

Found an issue regarding the handling of 802.1Q (VLAN) tagged frames in the latest version of OpenVPN in Windows.

My setup:

• A Linux-VM with OpenVPN server configured in Layer 2 bridge mode
• A Windows VM with OpenVPN client configured in Layer 2 bridge mode

If the Linux send a frame with VLAN tags into the tunnel, this frame is corrupted (Source MAC is wrong) when the packet leaves the TAP interface in Windows. In detail, the last 4 bytes of the destination MAC (bytes 8..11 in frame) are identical to bytes 5..7 in the frame.

Maybe this is related to this merge request: https://github.com/OpenVPN/tap-windows6/pull/81

I observed the problem with OpenVPN 2.4.9, with OpenVPN 2.4.7 everything works fine.

[svkers]

Additional note: It seems that the bug is in the function TapStrip8021Q() in file src/rxpath.c line 376:

// Copy the first part of the ethernet header up and over the protocol and 802.1Q data
// Don't copy the ethernet header's protocol, leave the inner value from the 802.1Q header.
NdisMoveMemory(buffer+4, buffer, ETHERNET_HEADER_SIZE-2);

After copying/moving the first 4 bytes, the already overwritten bytes are read and copied again. Please see also the figure below illustrating the issue:

Quick fix:

for (int i = (ETHERNET_HEADER_SIZE-2)-1; i >= 0; i--)
{
    buffer[i+4] = buffer[i];
}

which is the same as:

buffer[15] = buffer[11];
buffer[14] = buffer[10];
buffer[13] = buffer[ 9];
buffer[12] = buffer[ 8];
buffer[11] = buffer[ 7];
buffer[10] = buffer[ 6];
buffer[ 9] = buffer[ 5];
buffer[ 8] = buffer[ 4];
buffer[ 7] = buffer[ 3];
buffer[ 6] = buffer[ 2];
buffer[ 5] = buffer[ 1];
buffer[ 4] = buffer[ 0];

Edit: The above code has not been tested. Edit: According to MSDN NdisMoveMemory documentation the source and destination memory areas are not allowed to overlap:

The range specified by Source and Length cannot overlap the Destination range.

[selvanair]

The use of NdisMoveMemory() for shifting the buffer by 4 is definitely buggy. Note that its essentially RtlCopyMemory() which works like memcpy() not memmove().

The proposed loop looks correct, but RtlMoveMemory() may be better?

[svkers]

Yes, indeed, RtlMoveMemory() is the better approach to avoid the for loop. It allows for overlapping source and destination memory blocks. Thus the fix looks as follows:

// Copy the first part of the ethernet header up and over the protocol and 802.1Q data
// Don't copy the ethernet header's protocol, leave the inner value from the 802.1Q header.
RtlMoveMemory(buffer+4, buffer, ETHERNET_HEADER_SIZE-2);

instead of

// Copy the first part of the ethernet header up and over the protocol and 802.1Q data
// Don't copy the ethernet header's protocol, leave the inner value from the 802.1Q header.
NdisMoveMemory(buffer+4, buffer, ETHERNET_HEADER_SIZE-2);

[cron2]

Hi,

On Tue, Jul 21, 2020 at 10:05:34AM -0700, Sven Kerschbaum wrote:

Yes, indeed, RtlMoveMemory() is the better approach to avoid the for loop. It allows for overlapping source and destination memory blocks. Thus the fix looks as follows:

Thanks. This makes sense. (I assume that sgstair just assumed that NdisMoveMemory() would handle this correctly... - this 802.1q handling was rewritten/added for the test suite, that's why it works on older versions)

We'll have to wait for Samuli to return from vacation to roll and sign an updated tapv9 driver.

Well, and for me to actually turn the issue into a PR and submit that as PR :-) (tapv9 does PRs for patch submission).

gert

-- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany gert@greenie.muc.de

[selvanair]

@svkers: you may want to submit a PR with this fix.

[svkers]

@selvanair Ok, I created the PR as you suggested. Thanks.

[cron2]

So the PR has been merged, and the fix should be seen in a signed driver "real soon now". Please test and report :-)

[selvanair]

I tested the merged PR using https://build.openvpn.net/downloads/releases/OpenVPN-2.5-beta2-I601-amd64.msi The old tap driver 9.24.3.601 does mangle the ~destination~ source MAC while stripping VLAN id. The new one, 9.24.4.601 dated Aug 27, behaves correctly. I only checked the headers by capturing some packets, not the correct operation or performance of real traffic.

[cron2]

@stepman0 can you confirm that this is now fixed, and we can close the issue?

[svkers]

@cron2 and @stepman0 I can confirm that the merged PR works. I used the build https://build.openvpn.net/downloads/releases/OpenVPN-2.5-beta2-I601-amd64.msi. Please close the issue.

[cron2]

Great, thanks for testing & feedback.