From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9046 Path: news.gmane.org!not-for-mail From: Alexander Cherepanov Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] fix use of pointer after free in unsetenv Date: Mon, 4 Jan 2016 13:52:02 +0300 Message-ID: <568A4ED2.9020609@openwall.com> References: <5689AA38.60108@openwall.com> <20160104030558.GT238@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1451904739 19972 80.91.229.3 (4 Jan 2016 10:52:19 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 4 Jan 2016 10:52:19 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9059-gllmg-musl=m.gmane.org@lists.openwall.com Mon Jan 04 11:52:19 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1aG2kL-0007l2-Od for gllmg-musl@m.gmane.org; Mon, 04 Jan 2016 11:52:18 +0100 Original-Received: (qmail 32143 invoked by uid 550); 4 Jan 2016 10:52:14 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 32117 invoked from network); 4 Jan 2016 10:52:14 -0000 X-Enigmail-Draft-Status: N1110 In-Reply-To: <20160104030558.GT238@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:9046 Archived-At: On 2016-01-04 06:05, Rich Felker wrote: > On Mon, Jan 04, 2016 at 02:09:44AM +0300, Alexander Cherepanov wrote: >> The code in [1] uses a pointer which was freed and hence has an >> indeterminate value. Patch attached. >> >> [1] http://git.musl-libc.org/cgit/musl/tree/src/env/unsetenv.c#n23 > > The bug sounds a lot scarier than it actually is. Sure. I have tried at least not to use the term "use after free" but mentioning directly that the pointer is not dereferenced would be even better. > I don't think any > compilers will break this yet but it is indeed UB. I think so too. >> >From f446b5811a8abc08bcc8202aa241dce82d4c917d Mon Sep 17 00:00:00 2001 >> From: Alexander Cherepanov >> Date: Mon, 4 Jan 2016 01:40:03 +0300 >> Subject: [PATCH] fix use of pointer after free in unsetenv >> >> the value of a pointer becomes indeterminate after free() so delay free() >> until the pointer is not needed anymore. >> >> --- >> src/env/unsetenv.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/env/unsetenv.c b/src/env/unsetenv.c >> index 3569335..b5d8b19 100644 >> --- a/src/env/unsetenv.c >> +++ b/src/env/unsetenv.c >> @@ -19,9 +19,10 @@ again: >> if (__environ[i]) { >> if (__env_map) { >> for (j=0; __env_map[j] && __env_map[j] != __environ[i]; j++); >> - free (__env_map[j]); >> + char *t =__env_map[j]; >> for (; __env_map[j]; j++) >> __env_map[j] = __env_map[j+1]; >> + free (t); > > Wouldn't something like this be simpler: > > do __env_map[j] = __env_map[j+1]; > while (__env_map[++j]); This depends on whether our __env_map[j] could be 0. The condition "__env_map[j]" in the previous loop hints that it could. Then it should be something like this: if (__env_map[j]) { free (__env_map[j]); do __env_map[j] = __env_map[j+1]; while (__env_map[++j]); } -- Alexander Cherepanov