From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9047 Path: news.gmane.org!not-for-mail From: Alexander Monakov Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] fix use of pointer after free in unsetenv Date: Mon, 4 Jan 2016 14:56:26 +0300 (MSK) Message-ID: References: <5689AA38.60108@openwall.com> <20160104030558.GT238@brightrain.aerifal.cx> <568A4ED2.9020609@openwall.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Trace: ger.gmane.org 1451908604 16469 80.91.229.3 (4 Jan 2016 11:56:44 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 4 Jan 2016 11:56:44 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9060-gllmg-musl=m.gmane.org@lists.openwall.com Mon Jan 04 12:56:44 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 1aG3ke-0000G8-1w for gllmg-musl@m.gmane.org; Mon, 04 Jan 2016 12:56:40 +0100 Original-Received: (qmail 15802 invoked by uid 550); 4 Jan 2016 11:56:38 -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 15784 invoked from network); 4 Jan 2016 11:56:38 -0000 In-Reply-To: <568A4ED2.9020609@openwall.com> User-Agent: Alpine 2.20 (LNX 67 2015-01-07) Xref: news.gmane.org gmane.linux.lib.musl.general:9047 Archived-At: On Mon, 4 Jan 2016, Alexander Cherepanov wrote: > 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]); > } True, but it wouldn't solve the problem in its entirety. There's a similar issue further down (line 26 atm), where __environ[i] is tested in a loop. However, if we executed free(__env_map[j]), then __env_map[j]==__environ[i] had held. Thus, entering the loop invokes the same UB. To me the implementation looks weird due to how it restarts scanning __environ with 'goto again' from position 0 instead of current position. I can propose the following rewrite (untested): for (i=0; __environ[i]; i++) { char *e = __environ[i]; if (!memcmp(name, e, l) && e[l] == '=') { for (j=i--; __environ[j]; j++) __environ[j] = __environ[j+1]; if (__env_map) { for (j=0; __env_map[j] && __env_map[j] != e; j++); if (__env_map[j]) { free(__env_map[j]); do __env_map[j] = __env_map[j+1]; while (__env_map[++j]); } } } } Alexadner