Bug#831672: screen: segfault on digraph

[Axel Beckert]

Hi,

Thorsten Glaser wrote:
> On Thu, 4 Aug 2016, Jan Nordholz wrote:
> (I’ll test it once XTaran’s updated package hits sid).

Works for me on amd64 at least.

Expect the updated package within the next few days.

I've also cherry-picked another crash fix from upstream which is
related to the new bumpleft/bumpright commands.

Both fixes are already on
https://anonscm.debian.org/cgit/collab-maint/screen.git

Regards, Axel
-- 
 ,''`.  |  Axel Beckert , http://people.debian.org/~abe/
: :' :  |  Debian Developer, ftp.ch.debian.org Admin
`. `'   |  4096R: 2517 B724 C5F6 CA99 5329  6E61 2FF9 CD59 6126 16B5
  `-|  1024D: F067 EA27 26B9 C3FC 1486  202E C09E 1D89 9593 0EDE

Bug#831672: screen: segfault on digraph

[Thorsten Glaser]

On Thu, 4 Aug 2016, Jan Nordholz wrote:

> Quick fix for users: initialize argl[1] by instead doing

Thanks, this indeed works around it on amd64:

| bind u digraph 'U+' ''

> Suggested code fix: RC_DIGRAPH should probably check that args[1] !=
> NULL before checking argl[1] > 0 (because the latter is meaningless
> without the former, as discussed).

Interesting bug cause. Thanks for the quick analysis, workaround,
and fix (I’ll test it once XTaran’s updated package hits sid).

bye,
//mirabilos
-- 
tarent solutions GmbH
Rochusstraße 2-4, D-53123 Bonn • http://www.tarent.de/
Tel: +49 228 54881-393 • Fax: +49 228 54881-235
HRB 5168 (AG Bonn) • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

Bug#831672: screen: segfault on digraph

[Axel Beckert]

Hi Jan,

Jan Nordholz wrote:
> Adding a patch suggestion for completeness...

Thanks a lot for the patch. Will prepare an updated package soon.

Regards, Axel
-- 
 ,''`.  |  Axel Beckert , http://people.debian.org/~abe/
: :' :  |  Debian Developer, ftp.ch.debian.org Admin
`. `'   |  4096R: 2517 B724 C5F6 CA99 5329  6E61 2FF9 CD59 6126 16B5
  `-|  1024D: F067 EA27 26B9 C3FC 1486  202E C09E 1D89 9593 0EDE

Bug#831672: screen: segfault on digraph

[Jan Nordholz]

tags 831672 +patch
thankyou

Adding a patch suggestion for completeness...


Jan
Description: Fixes broken handling of "bind u digraph U+", resulting in a
 SIGSEGV instead of prompting for the remainder. Also fixes an allocation
 inaccuracy I found while debugging this, even though that one looks innocuous.
 See BTS #831672.
Author: Jan C. Nordholz 

--- a/process.c	2016-08-04 21:41:27.0 +0200
+++ b/process.c	2016-08-04 21:42:37.916299040 +0200
@@ -3855,7 +3855,7 @@
   break;
 
 case RC_DIGRAPH:
-  if (argl && argl[0] > 0 && argl[1] > 0)
+  if (argl && argl[0] > 0 && args[1] && argl[1] > 0)
 	{
 	  if (argl[0] != 2)
 	{
@@ -4691,9 +4691,9 @@
   act->argl = 0;
   return;
 }
-  if ((pp = (char **)malloc((unsigned)(argc + 1) * sizeof(char **))) == 0)
+  if ((pp = (char **)malloc((unsigned)(argc + 1) * sizeof(char *))) == 0)
 Panic(0, "%s", strnomem);
-  if ((lp = (int *)malloc((unsigned)(argc) * sizeof(int *))) == 0)
+  if ((lp = (int *)malloc((unsigned)(argc) * sizeof(int))) == 0)
 Panic(0, "%s", strnomem);
   act->nr = nr;
   act->args = pp;

Bug#831672: screen: segfault on digraph

[Jan Nordholz]

Hi,

the bug exists because screen's "struct action" provides no explicit way
to store the number of arguments, and doing

bind u digraph U+

results in act->args being allocated for two "char *"s (including a
terminating NULL), but act->argl only for one int... by the way, both
allocations I'm talking about (process.c:4698):

  if ((pp = (char **)malloc((unsigned)(argc + 1) * sizeof(char **))) == 0)
Panic(0, "%s", strnomem);
  if ((lp = (int *)malloc((unsigned)(argc) * sizeof(int *))) == 0)
Panic(0, "%s", strnomem);

should have a '*' less in the sizeof(), we're allocating room for ints, not
for pointers.

Anyway, DoAction() finally checks in the RC_DIGRAPH case (process.c:3858):

  if (argl && argl[0] > 0 && argl[1] > 0)

And as argl[] is only one member wide, the final "argl[1]" could be anything;
and apparently on x86 we're just lucky that it's indeed 0. (If it's not, then
the following code will try to dereference args[1], which is the terminating
NULL that was placed earlier.)

Quick fix for users: initialize argl[1] by instead doing

bind u digraph U+ ""

Suggested code fix: RC_DIGRAPH should probably check that args[1] != NULL
before checking argl[1] > 0 (because the latter is meaningless without the
former, as discussed).


Jan
-- 
Jan Nordholz 
Security in Telecommunications 
TU Berlin / Telekom Innovation Laboratories
Ernst-Reuter-Platz 7, Sekr TEL 17 / D - 10587 Berlin, Germany
phone: +49 30 8353 58663

Bug#831672: screen: segfault on digraph

[Thorsten Glaser]

Oh, fun.

This works now on x32, but still segfaults on amd64.
Both sid as of about one hour ago.

bye,
//mirabilos
-- 
tarent solutions GmbH
Rochusstraße 2-4, D-53123 Bonn • http://www.tarent.de/
Tel: +49 228 54881-393 • Fax: +49 228 54881-235
HRB 5168 (AG Bonn) • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

Bug#831672: screen: segfault on digraph

[Thorsten Glaser]

Package: screen
Version: 4.4.0-3
Severity: normal

cat >.screenrc
bind u digraph U+
^D
screen
^Au

… leads to…

[screen caught signal 11. (core dumped)]

Getting a backtrace is unfortunately not as easy, but
with sudo gdb screen I can at least get it interactively.

(gdb) bt
#0  0x7fe709acde10 in __pause_nocancel () at 
../sysdeps/unix/syscall-template.S:84
#1  0x556d56004524 in Attacher () at ../attacher.c:628
#2  0x556d55fdf2d0 in main (ac=0, av=) at ../screen.c:1275


-- System Information:
Debian Release: stretch/sid
  APT prefers unstable
  APT policy: (500, 'unstable')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 4.6.0-1-amd64 (SMP w/2 CPU cores)
Locale: LANG=C, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/lksh
Init: sysvinit (via /sbin/init)

Versions of packages screen depends on:
ii  libc6  2.23-1
ii  libpam0g   1.1.8-3.3
ii  libtinfo5  6.0+20160625-1

screen recommends no packages.

Versions of packages screen suggests:
pn  byobu | screenie | iselect  
pn  ncurses-term

-- debconf information:
* screen/410-upgrade:
  screen/403-copy-failed: