openssh bugs - Aug 2016 - [Bug 2608] New: Signed overflow in openbsd-compat/strlcpy.c

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

            Bug ID: 2608
           Summary: Signed overflow in openbsd-compat/strlcpy.c
           Product: Portable OpenSSH
           Version: 7.3p1
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: Miscellaneous
          Assignee: unassigned-bugs at mindrot.org
          Reporter: yuanjie.huang at windriver.com

Created attachment 2866
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2866&action=edit
test driver of strlcpy to demo the bug.

Pointer arithmatic results in implementation defined signed integer
type, so that 's - src' in strlcpy and others may trigger signed
overflow.

When the code is compiled by gcc or clang with -ftrapv option, the
overflow would lead to program abort.                                   

$ gcc test.c strlcpy.c -o test -m32 -ftrapv
$ ./test 
Allocating src starting from 0x7fffff40
Trying to copy 0x400 from 0x7fffff40 to 0x9630008
Aborted (core dumped)

The proposed fix is also attached.

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

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

--- Comment #1 from Yuanjie Huang <yuanjie.huang at windriver.com> ---
Created attachment 2867
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2867&action=edit
proposed fix.

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

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

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

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

--- Comment #2 from Darren Tucker <dtucker at zip.com.au> ---
Thanks.  Looking (these files are sourced from OpenBSD, so any changes
will need to go upstream first).


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 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.

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2594                        |

--- Comment #3 from Darren Tucker <dtucker at zip.com.au> ---
I applied then, after some discussion with some folks, reverted your
change.

The rationale is that C11 6.5.6.9 says:
"""
When two pointers are subtracted, both shall point to elements of the
same array object, or one past the last element of the array object;
the
result is the difference of the subscripts of the two array elements.
"""

In these cases the objects are arrays of char so the result is defined,
and we believe that the compiler incorrectly trapping on defined
behaviour.

I also found https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63303
("Pointer subtraction is broken when using -fsanitize=undefined")
which
seems to support this position.


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 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=2608

Hongxu Jia <hongxu.jia at windriver.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hongxu.jia at windriver.com

--- Comment #4 from Hongxu Jia <hongxu.jia at windriver.com> ---
Created attachment 3293
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3293&action=edit
proposed fix v2

-- 
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=2608

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |WONTFIX
                 CC|                            |djm at mindrot.org
             Status|NEW                         |RESOLVED

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
I agree with Darren's analysis - the existing implementation is
compliant with ISO C.

-- 
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=2608

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 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.