bugGNU Screen - Bugs: bug #66147, screen crashes in attacher.c:465...

 
 

bug #66147: screen crashes in attacher.c:465 when compiled with FORTIFY_SOURCE=3

Submitter:  None
Submitted:  Fri 30 Aug 2024 10:00:55 AM UTC
Votes: 101
 
Category:  Crash/Freeze/Infloop Severity:  3 - Normal
Priority:  * 5 - Normal Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Release:  5.0.0
Fixed Release:  None Planned Release:  None
Work Required:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Thu 05 Sep 2024 08:27:28 PM UTC, comment #3: 

Good point! As you mentioned, the ARRAY_SIZE macro will try to find the size of the pointer to the array of unknown length, and will devolve to simply the size of the pointer over the size of a char (8/1).

Upon further examination, the code change I suggested is simply limiting the copy to 7 characters, which fixes the contrived test case of "screen -S test -X stuff "ls" but does not allow strings larger than 7 now...

I can confirm that your suggestion of using strcpy instead of strncpy seems to make the runtime buffer overflow go away, but may be undermining the point of using FORTIFY_SOURCE=3, but I confess I'm not an expert in FORTIFY_SOURCE.

-Al C.

Anonymous
Thu 05 Sep 2024 08:27:58 AM UTC, comment #2: 

strncpy(p, av, ARRAY_SIZE(p) - 1) doesn't do what you're expecting. For char p, the value of ARRAY_SIZE(p) is constant, i.e. on 64 bit platforms it is 8.

An alternate fix would be to replace the offending strncpy(p, *av, MAXPATHLEN); with strcpy(p, *av); since it is guaranteed to fit because  of
if (p + len >= m.m.command.cmd + ARRAY_SIZE(m.m.command.cmd) - 1)
in https://git.savannah.gnu.org/cgit/screen.git/tree/src/attacher.c#n463.

Dan M.

Anonymous
Tue 03 Sep 2024 02:55:26 PM UTC, comment #1: 

I can confirm that changing line 465 in attacher.c from:

    strncpy(p, *av, MAXPATHLEN);

to:

    strncpy(p, *av, ARRAY_SIZE(p) - 1);

...fixes this specfic crash on 5.0.0 clean build, with FORTIFY_SOURCE=3 build flags still enabled.

There are many similar strncpy (and likely other) operations using #defined MAX*** sizes that would also likely need to be changed to address similar FORTIFY-induced buffer overruns in other parts of the code.

-Al C.

Anonymous
Fri 30 Aug 2024 10:00:55 AM UTC, original submission:  

Hello,

we have been recently made aware about a bug in screen which makes it crash when being compiled with FORTIFY_SOURCE=3. This usually indicates a bug in the source code.

It can be triggered like so:

    $ screen -v
    Screen version 5.0.0 (build on 2024-08-29 01:08:49)
    $ screen -dmS test
    $ screen -S test -X stuff "ls"
    * buffer overflow detected *: terminated
    Aborted (core dumped)

When examined with gdb this gives the following backtrace:

    (gdb) bt
    #0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
    #1  0x00007f4858e5b463 in __pthread_kill_internal (threadid=<optimized out>, signo=6) at pthread_kill.c:78
    #2  0x00007f4858e02120 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
    #3  0x00007f4858de94c3 in __GI_abort () at abort.c:79
    #4  0x00007f4858dea354 in __libc_message_impl (fmt=fmt@entry=0x7f4858f7816c "*** %s *: terminated\n") at ../sysdeps/posix/libc_fatal.c:132
    #5  0x00007f4858eea799 in __GI___fortify_fail (msg=msg@entry=0x7f4858f78153 "buffer overflow detected") at fortify_fail.c:24
    #6  0x00007f4858eea124 in __GI___chk_fail () at chk_fail.c:28
    #7  0x00007f4858eebd39 in __GI___strncpy_chk (s1=s1@entry=0x7ffed1859156 "", s2=s2@entry=0x7ffed185c111 "ls", n=n@entry=4096, s1len=<optimized out>)
        at strncpy_chk.c:26
    #8  0x000055fea93f5190 in strncpy (__dest=<optimized out>, __src=<optimized out>, __len=<optimized out>, __dest=<optimized out>, __src=<optimized out>,
        __len=<optimized out>) at /usr/include/bits/string_fortified.h:100
    #9  SendCmdMessage (sty=<optimized out>, match=<optimized out>, av=0x7ffed185b300, query=...) at /usr/src/debug/screen/screen-5.0.0/attacher.c:465
    #10 main (argc=<optimized out>, argv=0x7ffed185b2f8) at /usr/src/debug/screen/screen-5.0.0/screen.c:905

Indicating that https://git.savannah.gnu.org/cgit/screen.git/tree/src/attacher.c#n465 causes the crash.

This issue has first been reported on the Arch Linux Bugtracker by Albert Chaharbakhshi: https://gitlab.archlinux.org/archlinux/packaging/packages/screen/-/issues/2#note_206734

Anonymous

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jed (Voted in favor of this item)
  • -email is unavailable- added by ckujau (Voted in favor of this item)
  •  

    There are 101 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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-09-21 jed Carbon-Copy- Added jed
    2024-09-02 ckujau Carbon-Copy- Added ckujau

    Back to the top

    Powered by Savane 3.14-573e.
    Corresponding source code