NetBSD Problem Report #58136

From stix@stix.id.au  Wed Apr 10 07:24:17 2024
Return-Path: <stix@stix.id.au>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 8214D1A9239
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 10 Apr 2024 07:24:17 +0000 (UTC)
Message-Id: <20240410072410.2D48E19E51@stix.id.au>
Date: Wed, 10 Apr 2024 17:24:10 +1000 (AEST)
From: stix@stix.id.au
Reply-To: stix@stix.id.au
To: gnats-bugs@NetBSD.org
Subject: Use after free in libintl pgettext
X-Send-Pr-Version: 3.95

>Number:         58136
>Category:       lib
>Synopsis:       Use after free in libintl pgettext
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Apr 10 07:25:00 +0000 2024
>Last-Modified:  Mon Apr 15 06:25:01 +0000 2024
>Originator:     Paul Ripke
>Release:        NetBSD 10.0_RC6
>Organization:
Paul Ripke
"Great minds discuss ideas, average minds discuss events, small minds
 discuss people."
-- Disputed: Often attributed to Eleanor Roosevelt. 1948.
>Environment:
System: NetBSD slave 10.0_RC6 NetBSD 10.0_RC6 (SLAVE) #8: Sun Mar 17 09:38:38 AEDT 2024 stix@slave:/home/netbsd/netbsd-10/obj.amd64/home/netbsd/netbsd-10/src/sys/arch/amd64/compile/SLAVE amd64
Architecture: x86_64
Machine: amd64
>Description:
While attempting to port the latest release of games/widelands, running under
ASAN generated the following:

==1020==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000271f30 at pc 0x7f7ff6e4c10f bp 0x7f7fffff5d80 sp 0x7f7fffff5530
READ of size 9 at 0x603000271f30 thread T0
    #0 0x7f7ff6e4c10e  (/usr/lib/libasan.so.5+0x4c10e)
    #1 0x7f7ff2604e80  (/usr/lib/libintl.so.1+0x4e80)
    #2 0x1db76d1 in pgettext_wrapper (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x1bb76d1)
    #3 0xd7f351 in UI::BaseDropdown::BaseDropdown(UI::Panel*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, int, int, unsigned int, unsigned int, int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, UI::DropdownType, UI::PanelStyle, UI::ButtonStyle) (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0xb7f351)
    #4 0xe7f4e7 in FsMenu::MainMenu::MainMenu(bool) (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0xc7f4e7)
    #5 0x89c749 in WLApplication::run() (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x69c749)
    #6 0x1fba7b9 in main (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x1dba7b9)
    #7 0x86020c in ___start (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x66020c)

0x603000271f30 is located 0 bytes inside of 21-byte region [0x603000271f30,0x603000271f45)
freed by thread T0 here:
    #0 0x7f7ff6e433e7 in __interceptor_free (/usr/lib/libasan.so.5+0x433e7)
    #1 0x7f7ff2604e73  (/usr/lib/libintl.so.1+0x4e73)
    #2 0x1db76d1 in pgettext_wrapper (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x1bb76d1)
    #3 0xd7f351 in UI::BaseDropdown::BaseDropdown(UI::Panel*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, int, int, unsigned int, unsigned int, int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, UI::DropdownType, UI::PanelStyle, UI::ButtonStyle) (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0xb7f351)
    #4 0xe7f4e7 in FsMenu::MainMenu::MainMenu(bool) (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0xc7f4e7)
    #5 0x89c749 in WLApplication::run() (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x69c749)
    #6 0x1fba7b9 in main (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x1dba7b9)
    #7 0x86020c in ___start (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x66020c)
    #8 0x7f7ff7c0bbb7  (/usr/libexec/ld.elf_so+0xbbb7)

previously allocated by thread T0 here:
    #0 0x7f7ff6e43a5a in __interceptor_realloc (/usr/lib/libasan.so.5+0x43a5a)
    #1 0x7f7ff073afc5 in vasprintf_l (/lib/libc.so.12+0x13afc5)
    #2 0x7f7ff6eb1e17 in __interceptor_vasprintf (/usr/lib/libasan.so.5+0xb1e17)
    #3 0x7f7ff6eb25c2 in __interceptor_asprintf (/usr/lib/libasan.so.5+0xb25c2)
    #4 0x7f7ff2604e3e  (/usr/lib/libintl.so.1+0x4e3e)
    #5 0x1db76d1 in pgettext_wrapper (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x1bb76d1)
    #6 0xd7f351 in UI::BaseDropdown::BaseDropdown(UI::Panel*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, int, int, unsigned int, unsigned int, int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, UI::DropdownType, UI::PanelStyle, UI::ButtonStyle) (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0xb7f351)
    #7 0xe7f4e7 in FsMenu::MainMenu::MainMenu(bool) (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0xc7f4e7)
    #8 0x89c749 in WLApplication::run() (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x69c749)
    #9 0x1fba7b9 in main (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x1dba7b9)
    #10 0x86020c in ___start (/home/tmp/pkgwrk/games/widelands/work/widelands/./build/src/widelands+0x66020c)
    #11 0x7f7ff7c0bbb7  (/usr/libexec/ld.elf_so+0xbbb7)

SUMMARY: AddressSanitizer: heap-use-after-free (/usr/lib/libasan.so.5+0x4c10e) 
Shadow bytes around the buggy address:
  0x0c0680046390: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800463a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800463b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800463c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fd fd
  0x0c06800463d0: fd fd fa fa fd fd fd fa fa fa fd fd fd fd fa fa
=>0x0c06800463e0: fd fd fd fa fa fa[fd]fd fd fa fa fa fd fd fd fa
  0x0c06800463f0: fa fa fd fd fd fd fa fa fd fd fd fa fa fa fd fd
  0x0c0680046400: fd fd fa fa 00 00 00 fa fa fa 00 00 04 fa fa fa
  0x0c0680046410: 00 00 00 fa fa fa fd fd fd fa fa fa fd fd fd fd
  0x0c0680046420: fa fa fd fd fd fa fa fa fd fd fd fa fa fa fd fd
  0x0c0680046430: fd fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==1020==ABORTING

Looking at the code, concatenate_ctxt_id allocates a new string in msgctxt_id.
dcngettext may then return a pointer back to msgctxt_id, which we free, and
then optionally return back to the caller.

>How-To-Repeat:

I'm no expert here, but I think any call to pgettext that doesn't find a
translation may return a pointer to freed memory.

>Fix:

The following patch seems to work, and allowed running games/widelands under
ASAN:

diff --git a/lib/libintl/gettext.c b/lib/libintl/gettext.c
index 424abbd2f567..1bd62a31d4dd 100644
--- a/lib/libintl/gettext.c
+++ b/lib/libintl/gettext.c
@@ -174,11 +174,13 @@ pgettext_impl(const char *domainname, const char *msgctxt, const char *msgid1,

 	translation = dcngettext(domainname, msgctxt_id,
 		msgid2, n, category);
-	free(msgctxt_id);

 	p = strchr(translation, '\004');
-	if (p)
-		return p + 1;
+	if (p) {
+		free(msgctxt_id);
+		return msgid1;
+	}
+	free(msgctxt_id);
 	return translation;
 }


>Audit-Trail:
From: Thomas Klausner <wiz@NetBSD.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/58136: Use after free in libintl pgettext
Date: Wed, 10 Apr 2024 10:08:52 +0200

 Is this still a problem in upstream or is just because our version is so old?
  Thomas

From: Joerg Sonnenberger <joerg@bec.de>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/58136: Use after free in libintl pgettext
Date: Wed, 10 Apr 2024 13:44:33 +0200

 On Wednesday, April 10, 2024 10:10:01=E2=80=AFAM CEST you wrote:
 > The following reply was made to PR lib/58136; it has been noted by GNATS.
 >=20
 > From: Thomas Klausner <wiz@NetBSD.org>
 > To: gnats-bugs@netbsd.org
 > Cc:=20
 > Subject: Re: lib/58136: Use after free in libintl pgettext
 > Date: Wed, 10 Apr 2024 10:08:52 +0200
 >=20
 >  Is this still a problem in upstream or is just because our version is so=
  old?

 Neither, this is our libintl implementation.

 Joerg


From: Paul Ripke <stix@stix.id.au>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/58136: Use after free in libintl pgettext
Date: Thu, 11 Apr 2024 13:31:47 +1000

 Yeah, I looked around for other implementations and came up empty; looking
 at blame on the source, I came to the conclusion it's ours.

 After sleeping on it, I've amended my patch; I think this is more correct,
 although still rather ugly:

 diff --git a/lib/libintl/gettext.c b/lib/libintl/gettext.c
 index 424abbd2f567..0469e6512fdc 100644
 --- a/lib/libintl/gettext.c
 +++ b/lib/libintl/gettext.c
 @@ -176,6 +176,9 @@ pgettext_impl(const char *domainname, const char *msgctxt, const char *msgid1,
  		msgid2, n, category);
  	free(msgctxt_id);

 +	if (translation == msgctxt_id)
 +		return msgid1;
 +
  	p = strchr(translation, '\004');
  	if (p)
  		return p + 1;

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58136 CVS commit: src/lib/libintl
Date: Fri, 12 Apr 2024 22:01:38 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Sat Apr 13 02:01:38 UTC 2024

 Modified Files:
 	src/lib/libintl: gettext.c

 Log Message:
 PR/58136: Paul Ripke: Fix use after free.


 To generate a diff of this commit:
 cvs rdiff -u -r1.31 -r1.32 src/lib/libintl/gettext.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Paul Ripke <stix@stix.id.au>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: PR/58136 CVS commit: src/lib/libintl
Date: Mon, 15 Apr 2024 16:22:18 +1000

 Thanks, Christos!

 Is this worth pulling up to netbsd-10?

 -- 
 Paul Ripke
 "Great minds discuss ideas, average minds discuss events, small minds
  discuss people."
 -- Disputed: Often attributed to Eleanor Roosevelt. 1948.

Home	PR Database Search