openssh bugs - Feb 2023 - [Bug 3534] New: probable underflow calculating display width of file name

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

            Bug ID: 3534
           Summary: probable underflow calculating display width of file
                    name
           Product: Portable OpenSSH
           Version: -current
          Hardware: amd64
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P5
         Component: scp
          Assignee: unassigned-bugs at mindrot.org
          Reporter: programmerjake at gmail.com

I first found this on Termux on AArch64 Android, but am able to
replicate on x86-64 Ubuntu 20.04.

running:
touch
????????????????????????????????????????????????????????????????.txt
scp
????????????????????????????????????????????????????????????????.txt
jacob at 192.168.1.141:

gives:
FORTIFY: vsnprintf: size 18446744073709551610 > SSIZE_MAX
Aborted

afaict this is terminal-width-dependent, my terminal has $COLUMNS set
to 120, if i increase my terminal width to 205, then it completes
successfully.

Afaict this bug also occurs in sftp, i was able to crash it by running
the corresponding `put` command from interactive sftp.

I was able to reproduce the scp bug on Ubuntu 20.04.5 LTS on x86-64
where it apparently just prints garbage instead of aborting:
????????????????????????????????????????

Ubuntu's ssh version:
ssh -V
OpenSSH_8.2p1 Ubuntu-4ubuntu0.5, OpenSSL 1.1.1f  31 Mar 2020
Termux's ssh version:
OpenSSH_9.2p1, OpenSSL 3.0.7 1 Nov 2022

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

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

programmerjake at gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |programmerjake at gmail.com

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

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
I'm not able to replicate this. Are you able to get a stacktrace?

Failing that, what are your LOCALE and LC_* environment variables set
to?

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

--- Comment #2 from programmerjake at gmail.com ---
Created attachment 3665
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3665&action=edit
backtrace from snprintf call with invalid maxlen

(In reply to Damien Miller from comment #1)
> I'm not able to replicate this. Are you able to get a stacktrace?
yes. I recompiled from source on commit
6dfb65de949cdd0a5d198edee9a118f265924f33 and edited progressmeter.c,
changing can_output to always return 1, and editing setscreensize to
always set win_size to 121. line numbers should still match the
unedited version.

I built it with:
configure --prefix=/home/jacob/projects/openssh/dbg CFLAGS='-g -O0'
--with-privsep-path=/run/sshd --disable-strip

created a file:
touch ??????????????????????????????????????????????????
and debugged the built scp with args:
/home/jacob/projects/openssh/dbg/bin/scp
?????????????????????????????????????????????????? localhost:

Attached the backtrace
> 
> Failing that, what are your LOCALE and LC_* environment variables
> set to?
LANG=en_US.UTF-8
LC_COLLATE=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=3534

--- Comment #3 from programmerjake at gmail.com ---
Created attachment 3666
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3666&action=edit
temporary edits applied for debugging

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at dtucker.net

--- Comment #4 from Darren Tucker <dtucker at dtucker.net> ---
I also was not able to replicate it on x86-64 (Fedora) or aarch64
(Debian).  Could you put the repo case into a shell script?  I'm
wondering if we're losing something with the cut and paste (possibly
related to EscapeChar?).

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

--- Comment #5 from programmerjake at gmail.com ---
Created attachment 3667
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3667&action=edit
shell script for reproduction

the reproducer script doesn't need "temporary edits applied for
debugging" patch

note that I only ever got the abort on Termux, since Ubuntu's snprintf
doesn't check for nonsensical size inputs.

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

--- Comment #6 from programmerjake at gmail.com ---
I'm also able to reproduce on debian 10 x86-64 with:
OpenSSH_7.9p1 Debian-10+deb10u2, OpenSSL 1.1.1n  15 Mar 2022

$ ./reproducer.sh
Agent pid 1567
Enter passphrase for /home/jacob/.ssh/id_rsa:
Identity added: /home/jacob/.ssh/id_rsa (jacob at jacob-build-server)
?????????????????????????????????????????
$

oh, also, i have the computer i'm running scp from on the
authorized_keys list

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

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
Created attachment 3670
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3670&action=edit
Used snmprintf() for window width handling

Firstly, thanks a heap for the debugging and reproduction work you've
done. It makes our lives a lot easier.

It looks like what is happening is that snmprintf() (our UTF-8-aware
string formatting function) can write more than win_size characters to
the buffer. At first glance this shouldn't happen for two reasons: 

1. file_len, which is used as the column limit to snmprintf() is less
than win_size
2. The field with for the "%-*s" format string is passed as file_len
too.

Neither of these turns out to be an effective limit. For #2, this is
the width of the field (i.e how much to pad) rather than the precision
(where to chop).

For #1, the column limit applies to the visual width of the string as
displayed and not the number of characters used. These will differ if
the string contains UTF-8 multibyte characters as this reproducer does.

So strlen(buf) can easily exceed win_size for UTF-8 strings and this
will lead to the negative snprintf() size later when we calculate is as
"win_size - strlen(buf)".

IMO the best solution to this is to let snmprintf() do more of the work
in trying to hit win_size, as attempting to do it via strlen() is never
going to work for UTF-8. This patch implements this, adjusting the
first snmprintf() call to return how much padding we need to hit
file_len. The padded string should be visually correct, but I added a
final snmprintf() call to ensure that it's truncated in a UTF-8 safe
way at win_size.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3670|0                           |1
        is obsolete|                            |

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
Created attachment 3676
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3676&action=edit
Revised diff

Please try this one

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

--- Comment #11 from programmerjake at gmail.com ---
(In reply to Damien Miller from comment #10)
> Created attachment 3676 [details]
> Revised diff
> 
> Please try this one
tried it, looks good to me! Thanks for the extra effort to fix it on
tiny terminals too!

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3676|                            |ok?(dtucker at dtucker.net)
              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=3534

--- Comment #12 from Damien Miller <djm at mindrot.org> ---
Created attachment 3679
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3679&action=edit
different approach

Here's a different approach that uses asmprintf() and xextendf() to
build the string, avoiding all the strlen() math.

It's probably safer but makes a few more syscalls and heap operations
per progressmeter update. IMO safety wins here.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3679|                            |ok?(dtucker at dtucker.net)
              Flags|                            |

--- Comment #13 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 3679
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3679
different approach

Darren made me do this so he gets to look at it

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3679|ok?(dtucker at dtucker.net)    |ok+
              Flags|                            |

--- Comment #14 from Darren Tucker <dtucker at dtucker.net> ---
Comment on attachment 3679
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3679
different approach

Looks ok with a couple of nits.
>+	static char buf[((sizeof(off_t) * 8 * 4 * 2) / 10) + 16];
What's the reasoning behind the numbers?
>+		if (hours != 0) {
>+			xextendf(&buf, NULL, "%d:%02d:%02d",
>+			    hours, minutes, seconds);
>+		} else {
>+			xextendf(&buf, NULL, "  %02d:%02d", minutes, seconds);
>+		}
The rest of this function doesn't have brackets around single-line
if/else statements.

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

--- Comment #15 from Damien Miller <djm at mindrot.org> ---
(In reply to Darren Tucker from comment #14)
> Comment on attachment 3679 [details]
> different approach
> 
> Looks ok with a couple of nits.
> 
> >+	static char buf[((sizeof(off_t) * 8 * 4 * 2) / 10) + 16];
> 
> What's the reasoning behind the numbers?
calculating the length of integer rendered as a string using
nbits*log2(10) / 10

8 bits per byte
4 > 3.321 ~= log2(10)
2 because there are two variables being formatted

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3676|0                           |1
        is obsolete|                            |
   Attachment #3676|ok?(dtucker at dtucker.net)    |
              Flags|                            |
   Attachment #3679|0                           |1
        is obsolete|                            |
   Attachment #3680|                            |ok?(dtucker at dtucker.net)
              Flags|                            |

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

Updated the diff to move the math to a #define and fix the braces

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

--- Comment #17 from Darren Tucker <dtucker at dtucker.net> ---
(In reply to Damien Miller from comment #13)
> Darren made me do this so he gets to look at it
It's true, my main contribution to this project is nersniping Damien
into implementing things I'm too lazy or incompetent to do.

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3680|ok?(dtucker at dtucker.net)    |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=3534

Sam James <sam at gentoo.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at gentoo.org

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #19 from Damien Miller <djm at mindrot.org> ---
OpenSSH 9.3 has been released. Close resolved bugs

-- 
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.