Bug 2735 - Wrong address family handling for tun devices
Summary: Wrong address family handling for tun devices
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: 7.5p1
Hardware: Other Other
: P5 normal
Assignee: Damien Miller
URL:
Keywords:
Depends on:
Blocks: V_7_6
  Show dependency treegraph
 
Reported: 2017-07-03 08:27 AEST by stepe
Modified: 2021-04-23 15:10 AEST (History)
2 users (show)

See Also:


Attachments
Patch to fix address family handling for sys_tun_infilter() (696 bytes, patch)
2017-07-03 08:27 AEST, stepe
no flags Details | Diff
Patch to fix address family handling for sys_tun_outfilter() (766 bytes, patch)
2017-07-03 08:28 AEST, stepe
no flags Details | Diff
revised diff (1.41 KB, patch)
2017-07-07 14:10 AEST, Damien Miller
no flags Details | Diff
revised again (1.54 KB, patch)
2017-07-21 14:39 AEST, Damien Miller
dtucker: ok+
Details | Diff
re-re-revised diff (3.19 KB, patch)
2017-07-24 15:06 AEST, Damien Miller
no flags Details | Diff
Modified re-re-revised diff to not use htonl()/ntohl() anymore (1.64 KB, patch)
2017-07-28 05:13 AEST, stepe
no flags Details | Diff
final (?) diff (3.15 KB, patch)
2017-07-28 11:06 AEST, Damien Miller
dtucker: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description stepe 2017-07-03 08:27:42 AEST
Created attachment 3005 [details]
Patch to fix address family handling for sys_tun_infilter()

[Also affects sshd.
Affected OSes depend on SSH_TUN_COMPAT_AF and SSH_TUN_PREPEND_AF.]

Hello OpenSSH developers,

I noticed issues with the address family handling for tun devices in the sys_tun_infilter() and sys_tun_outfilter() functions.

An example symptom is that when using tuns with IPv6 on Linux (SSH_TUN_COMPAT_AF and SSH_TUN_PREPEND_AF defined), the client sends tunneled packets with the (fallback) v4 family.

In sys_tun_infilter(), the AF is not always converted to network byte
order. Please see the first attached patch.

In sys_tun_outfilter(), the af pointer is assigned from the integer return value of ntohl() and then later dereferenced. Please see the
second patch for a proposed fix.
I have/could not test this second patch as I do not have a platform with SSH_TUN_COMPAT_AF, but not SSH_TUN_PREPEND_AF (at least not that I know).

Have a nice day,
Peter

PS: Thank you for developing and maintaining OpenSSH and OpenBSD
Comment 1 stepe 2017-07-03 08:28:40 AEST
Created attachment 3006 [details]
Patch to fix address family handling for sys_tun_outfilter()
Comment 2 Darren Tucker 2017-07-04 11:41:06 AEST
I'm not familiar with this code but it sounds like something that should be looked at for 7.6p1.
Comment 3 Damien Miller 2017-07-07 14:10:12 AEST
Created attachment 3011 [details]
revised diff

I think there are a couple of other problems in the output filter: it wasn't testing the address family header against the host-order expected value. Also, it was using potentially unaligned word access.

Could you give this diff a try?
Comment 4 Damien Miller 2017-07-21 14:39:01 AEST
Created attachment 3016 [details]
revised again

re-revised; this one should makes the endian swizzling less confusing and more correct
Comment 5 Damien Miller 2017-07-24 15:06:56 AEST
Created attachment 3017 [details]
re-re-revised diff

Reading over the diff before I committed it, I noticed a few more problems so here's a third attempt.

This simplifies the input filter code a bit, making the COMPAT_AF path almost completely separate to the PREPEND_AF one, removes potential unaligned accesses and adds some needed comments.
Comment 6 stepe 2017-07-26 07:10:57 AEST
I (finally) tried your (latest) revised patch, but with it applied it seems that no data is arriving on the tun device of the server (which is running OpenBSD, so no patch there). tcpdump on the client however, reports some outgoing packets.

To rule out configuration issues, I tried my original patch afterwards again and it still worked.

I will have a closer look tomorrow evening or the day after.
Comment 7 stepe 2017-07-28 05:13:14 AEST
Created attachment 3019 [details]
Modified re-re-revised diff to not use htonl()/ntohl() anymore

From what I understand, data in the ssh buffer should live there in network byte order (big endian) and the preferred/canonical way to ensure this is with the PEEK_*()/POKE_*() macros.
However, the original code used ntohl()/htonl() for this purpose instead.
Doing both messes up the byte order again, so I tried your diff again without ntohl()/htonl() (see attachment) and it worked fine.
Comment 8 Damien Miller 2017-07-28 11:06:59 AEST
Created attachment 3020 [details]
final (?) diff

Well, that's embarrassing. I wrote those macros, so I should have remembered how they work.
Comment 9 Damien Miller 2017-07-28 13:23:25 AEST
Applied - thanks for the help. This will be in openssh-7.6
Comment 10 stepe 2017-07-29 05:54:48 AEST
Cool, looking forward for the new version. Thank you!
Comment 11 Damien Miller 2021-04-23 15:10:01 AEST
closing resolved bugs as of 8.6p1 release