openssh bugs - Jul 2017 - [Bug 2735] New: Wrong address family handling for tun devices

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

            Bug ID: 2735
           Summary: Wrong address family handling for tun devices
           Product: Portable OpenSSH
           Version: 7.5p1
          Hardware: Other
                OS: Other
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: stepe at centaurus.uberspace.de

Created attachment 3005
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3005&action=edit
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

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

--- Comment #1 from stepe at centaurus.uberspace.de ---
Created attachment 3006
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3006&action=edit
Patch to fix address family handling for sys_tun_outfilter()

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au
             Blocks|                            |2698

--- Comment #2 from Darren Tucker <dtucker at zip.com.au> ---
I'm not familiar with this code but it sounds like something that
should be looked at for 7.6p1.


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2698
[Bug 2698] Tracking bug for OpenSSH 7.6 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Created attachment 3011
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3011&action=edit
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?

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3005|0                           |1
        is obsolete|                            |
   Attachment #3006|0                           |1
        is obsolete|                            |
   Attachment #3011|0                           |1
        is obsolete|                            |
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
             Status|NEW                         |ASSIGNED
   Attachment #3016|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Created attachment 3016
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3016&action=edit
revised again

re-revised; this one should makes the endian swizzling less confusing
and more correct

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3016|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3016|0                           |1
        is obsolete|                            |
   Attachment #3017|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Created attachment 3017
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3017&action=edit
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

--- Comment #6 from stepe at centaurus.uberspace.de ---
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

--- Comment #7 from stepe at centaurus.uberspace.de ---
Created attachment 3019
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3019&action=edit
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3017|0                           |1
        is obsolete|                            |
   Attachment #3017|ok?(dtucker at zip.com.au)     |
              Flags|                            |
   Attachment #3019|0                           |1
        is obsolete|                            |
   Attachment #3020|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
Created attachment 3020
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3020&action=edit
final (?) diff

Well, that's embarrassing. I wrote those macros, so I should have
remembered how they work.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3020|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
Applied - thanks for the help. This will be in openssh-7.6

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

--- Comment #10 from stepe at centaurus.uberspace.de ---
Cool, looking forward for the new version. Thank you!

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2735

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #11 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 release

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.