bugGNU Screen - Bugs: bug #50197, out of bounds write when...

 
 

bug #50197: out of bounds write when dimensions are still 0, 0

Submitter:  None
Submitted:  Tue 31 Jan 2017 01:59:11 PM UTC
   
 
Category:  Crash/Freeze/Infloop Severity:  3 - Normal
Priority:  * 5 - Normal Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Release:  4.4.0
Fixed Release:  4.5.1 Planned Release:  None
Work Required:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Wed 01 Feb 2017 03:02:32 PM UTC, comment #2: 

Thanks for detailed bug report and analysis.

Apparently it was already "fixed" on master branch by making Z0width and Z1width really constant.

cherry-picked to screen-v4 branch.

http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4&id=a8c0d68f669eb17a2bf9d8d97f834f5cabb1dad5


Write string already has dimension check
  if (cols > 0 && rows > 0)                                                                                            
    {

Amadeusz Sławiński <amade>
Group administrator
Wed 01 Feb 2017 08:47:02 AM UTC, comment #1: 

More investigation yielded some more insights:
- rows/cols is zero because of an earlier escape sequence (\E[?1;3;4;6l) which is "column mode" and is supposed to initialize to either columns of 80 or 132, see e.g. http://vt100.net/docs/vt220-rm/chapter4.html, source code snippet:

            case 3:     /* COLM: column mode */
              i = (i ? Z0width : Z1width);
              ClearScreen();
              curr->w_x = 0;
              curr->w_y = 0;
              WChangeSize(curr, i, curr->w_height);
              cols = curr->w_width;
              rows = curr->w_height;
              break;

- the numbers 80 and 132 are stored in the "constants" Z0width and Z1width which are to be set by InitTermcap, source code snippet:

  /* Termcap fields Z0 & Z1 contain width-changing sequences. */
  if (D_CZ1 == 0)
    D_CZ0 = 0;
  Z0width = 132;
  Z1width = 80;

- since we are starting the screen in detached mode, display is 0 and InitTermcap is never called:

  if (display)
    {
      if (InitTermcap(0, 0))
        {
          debug("Could not init termcap - exiting\n");

So in summary I think there are at least two issues:
- when started in detached mode, some initialization is missing (Z0width, Z1width)
- WriteString should check for 0 dimensions and not call low-level functions which would do an out of bounds array access

Anonymous
Tue 31 Jan 2017 01:59:11 PM UTC, original submission:  

Program received signal SIGSEGV, Segmentation fault.
0x080510da in MFixLine (p=p@entry=0x80cb2d8, y=y@entry=-1, mc=mc@entry=0x80cd830) at ansi.c:2371
2371    ansi.c: No such file or directory.
(gdb) bt
#0  0x080510da in MFixLine (p=p@entry=0x80cb2d8, y=y@entry=-1, mc=mc@entry=0x80cd830) at ansi.c:2371
#1  0x08051a7a in MPutChar (p=0x80cb2d8, c=0x80cd830, x=-1, y=-1) at ansi.c:2723
#2  0x08057e61 in WriteString (wp=0x0, buf=0xffb8c453 "\033[1;27H\r\n", len=175) at ansi.c:869
#3  0x08067868 in win_readev_fn (ev=0x80cb2e4, data=0x80cb2d8 "") at window.c:1942
#4  0x08090630 in sched () at sched.c:237
#5  0x0804c463 in main (ac=<optimized out>, av=<optimized out>) at screen.c:1487
(gdb) frame 0
#0  0x080510da in MFixLine (p=p@entry=0x80cb2d8, y=y@entry=-1, mc=mc@entry=0x80cd830) at ansi.c:2371
2371      if (mc->attr && ml->attr == null)
(gdb) up
#1  0x08051a7a in MPutChar (p=0x80cb2d8, c=0x80cd830, x=-1, y=-1) at ansi.c:2723
2723      MFixLine(p, y, c);
(gdb) up
#2  0x08057e61 in WriteString (wp=0x0, buf=0xffb8c453 "\033[1;27H\r\n", len=175) at ansi.c:869
869                       MPutChar(curr, &curr->w_rend, curr->w_x, curr->w_y);
(gdb) l
864                           curr->w_x++;
865                         }
866                     }
867                   else if (curr->w_x == cols - 1)
868                     {
869                       MPutChar(curr, &curr->w_rend, curr->w_x, curr->w_y);
870                       LPutChar(&curr->w_layer, &curr->w_rend, curr->w_x, curr->w_y);
871                       if (curr->w_wrap)
872                         curr->w_x++;
873                     }
(gdb) p cols
$3 = 0
(gdb) p rows
$4 = 0

As one can see rows/cols are both 0 valued but are used for range index computations. It seems when a screen is started in detached mode those variables are never set to reasonable values (like 25, 80).
Above source code is actually from 4.2.1 as it was easier in Debian to retrieve the source from that version, yet the error also occurs in 4.4.0.

Reproduce:
Terminal 1:
$ xxd bad_sequence
00000000: 1b5b 721b 5b6d 1b5b 324a 1b5b 481b 5b3f  .[r.[m.[2J.[H.[?
00000010: 3768 1b5b 3f31 3b33 3b34 3b36 6c1b 5b3b  7h.[?1;3;4;6l.[;
00000020: 481b 5b32 4a00 0000 0000 0000 0000 0000  H.[2J...........
00000030: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000040: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000050: 0000 0000 0000 001b 5b3b 481b 5b32 4a00  ........[;H.[2J.
00000060: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000070: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000080: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000090: 001b 5b3b 376d 1b28 301b 5b31 3b31 3048  ..[;7m.(0.[1;10H
000000a0: 0000 0000 0078 1b5b 313b 3237 480a       .....x.[1;27H.
$ screen -d -m bash -c 'sleep 10; cat bad_sequence; sleep 999'
Terminal 2 ("continue" in gdb and wait for the crash in less than 10 seconds)
$ gdb -p $(pgrep  screen)

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #39625:  bad_sequence added by None (174B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by amade (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-06-29 amade Open/ClosedOpen Closed
        Fixed ReleaseNone 4.5.1
    2017-02-01 amade StatusNone Fixed
    2017-01-31 None Attached File- Added bad_sequence, #39625

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code