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
>Closed-Date:    
>Last-Modified:  Thu Sep 12 19:45: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;
 }


>Release-Note:

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

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Tue, 23 Jul 2024 22:11:15 +0000
State-Changed-Why:
needs pullup-10, maybe pullup-9 too


State-Changed-From-To: needs-pullups->open
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sun, 18 Aug 2024 17:30:32 +0000
State-Changed-Why:
This is not fixed -- it has no test case and there's still undefined
behaviour (referring to a pointer after it has been freed), requiring
another patch:

--- gettext.c
+++ gettext.c
@@ -174,10 +174,13 @@

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

-	if (translation == msgctxt_id)
+	if (translation == msgctxt_id) {
+		free(msgctxt_id);
 		return msgid1;
+	}
+
+	free(msgctxt_id);

 	p = strchr(translation, '\004');
 	if (p)

(or something to that effect)


From: Paul Ripke <stix@stix.id.au>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/58136 (Use after free in libintl pgettext)
Date: Mon, 19 Aug 2024 21:45:04 +1000

 From my reading of C's behaviour, dereferencing a freed pointer is undefined
 behaviour, obviously. But using the pointer value should not be? Indeed, when
 compiled with clang/llvm, your code results in only one call to free.

 Agreed on the missing test, though. This code is old and crufty, does it have
 any coverage at all? I didn't see any after a brief look.

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

From: Rob Whitlock <rwhitlock22@gmail.com>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 stix@stix.id.au
Subject: Re: lib/58136 (Use after free in libintl pgettext)
Date: Mon, 19 Aug 2024 18:01:26 -0400

 > On Aug 19, 2024, at 7:50 AM, gnats-admin@netbsd.org wrote:
 >=20
 > The following reply was made to PR lib/58136; it has been noted by =
 GNATS.
 >=20
 > From: Paul Ripke <stix@stix.id.au>
 > To: gnats-bugs@netbsd.org
 > Cc:=20
 > Subject: Re: lib/58136 (Use after free in libintl pgettext)
 > Date: Mon, 19 Aug 2024 21:45:04 +1000
 >=20
 > =46rom my reading of C's behaviour, dereferencing a freed pointer is =
 undefined
 > behaviour, obviously. But using the pointer value should not be? =
 Indeed, when
 > compiled with clang/llvm, your code results in only one call to free.

 In N1256 (a C99 draft) section 6.2.4p2, it says

   The /lifetime/ of an object is the portion of program execution during =
 which
   storage is guaranteed to be reserved for it. An object exists, has a =
 constant
   address,^25) and retains its last-stored value throughout its =
 lifetime.^26) If
   an object is referred to outside of its lifetime, the behavior is =
 undefined. The
   value of a pointer becomes indeterminate when the object it points to
   reaches the end of its lifetime.

 Section 7.20.3 describes the malloc, calloc, realloc, and free =
 functions. Section
 7.20.3p1 contains the sentence

   The lifetime of an allocated object extends from the allocation until =
 the
   deallocation.

 Section 3.17.2 defines "indeterminate value" as

   either an unspecified value or a trap representation

 Section 3.17.3 defines "unspecified value" as

   valid value of the relevant type where this International Standard =
 imposes
   no requirements on which value is chosen in any instance

   NOTE An unspecified value cannot be a trap representation

 Putting this together, we can see that accessing the value of a pointer =
 after
 it has been freed can result in a meaningless value or a trap.

 ANSI C says something slightly different. Section 6.1.2.4 of ANSI C says

     An object has a /storage duration/ that determines its lifetime. =
 There are
   two storage durations: static and automatic.

     An object whose identifier is declared with external or internal =
 linkage, or
   with the storage-class specifier static has /static storage duration/. =
 For such
   an object, storage is reserved and its stored value is initialized =
 only once,
   prior to program startup. The object exists and retains its =
 last-stored value
   throughout the execution of the program.^15

     An object whose identifier is declared with no linkage and without =
 the
   storage-class specifier static has /automatic storage duration/. =
 Storage is
   guaranteed to be reserved for a new instance of such an object on each
   normal entry into the block with which it is associated, or on a jump =
 from
   outside the block to a labeled statement in the block or in an =
 enclosed
   block. In an initialization is specified for the value stored in the =
 object, it is
   performed on each normal entry, but not if the block is entered by a =
 jump
   to a labeled statement. Storage for the object is no longer guaranteed =
 to
   be reserved when execution of a block ends in any way. (Entering an
   enclosed block suspends but does not end execution of the enclosing
   block. Calling a function suspends but does not end execution of the
   block containing the call.) The value of a pointer that referred to an =
 object
   with automatic storage duration that is no longer guaranteed to be
   reserved is indeterminate.

 Section 7.10.3 addresses malloc, calloc, realloc and free, and contains
 the sentence

   The value of a pointer that refers to freed space is indeterminate.

 Section 3.16 defines "undefined behavior" as

   Behavior, upon use of a nonportable or erroneous program construct,
   of erroneous data, or of indeterminately valued objects, for which =
 this
   International Standard imposes no requirements. Permissible undefined
   behavior ranges from ignoring the situation completely with =
 unpredictable
   results, to behaving during translation or program execution in a
   documented manner characteristic of the environment (with or without
   issuance of a diagnostic message), to terminating a translation or
   execution (with the issuance of a diagnostic message).

     If a "shall" or "shall not" requirement that appears outside of a =
 constraint
   is violated, the behavior is undefined. Undefined behavior is =
 otherwise
   indicated in this International Standard by the words "undefined =
 behavior"
   or by the omission of any explicit definition of behavior. There is no
   difference in emphasis among these three; they all describe "behavior
   that is undefined."

 Putting these together, we see that accessing a pointer after is has =
 been freed is undefined behavior in ANSI C.

 > Agreed on the missing test, though. This code is old and crufty, does =
 it have
 > any coverage at all? I didn't see any after a brief look.
 >=20
 > --=20
 > Paul Ripke
 > "Great minds discuss ideas, average minds discuss events, small minds
 >  discuss people."
 > -- Disputed: Often attributed to Eleanor Roosevelt. 1948.
 >=20

From: Paul Ripke <stix@stix.id.au>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/58136 (Use after free in libintl pgettext)
Date: Tue, 20 Aug 2024 22:01:59 +1000

 Ack, thanks for the clue bat, I should've checked those sources myself,
 rather than relying on poor summaries.

 Concur, that makes it clear that accessing the value of a freed ptr
 is UD, regardless of implementation details.

 And, thanks to christos@, I see this has been fixed.

 Can we now pull this 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.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58136 CVS commit: [netbsd-10] src/lib/libintl
Date: Thu, 12 Sep 2024 19:41:09 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Sep 12 19:41:09 UTC 2024

 Modified Files:
 	src/lib/libintl [netbsd-10]: gettext.c

 Log Message:
 Pull up following revision(s) (requested by rin in ticket #842):

 	lib/libintl/gettext.c: revision 1.32

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


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

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

>Unformatted:

Home	PR Database Search