From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9056 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] fix use of pointer after free in unsetenv Date: Mon, 4 Jan 2016 16:53:15 -0500 Message-ID: <20160104215315.GY238@brightrain.aerifal.cx> References: <5689AA38.60108@openwall.com> <20160104030558.GT238@brightrain.aerifal.cx> <568A4ED2.9020609@openwall.com> <20160104210528.GX238@brightrain.aerifal.cx> 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 1451944410 24187 80.91.229.3 (4 Jan 2016 21:53:30 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 4 Jan 2016 21:53:30 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9069-gllmg-musl=m.gmane.org@lists.openwall.com Mon Jan 04 22:53:30 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 1aGD4D-0000jM-Ig for gllmg-musl@m.gmane.org; Mon, 04 Jan 2016 22:53:29 +0100 Original-Received: (qmail 24429 invoked by uid 550); 4 Jan 2016 21:53:27 -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 24411 invoked from network); 4 Jan 2016 21:53:27 -0000 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:9056 Archived-At: On Tue, Jan 05, 2016 at 12:28:12AM +0300, Alexander Monakov wrote: > On Mon, 4 Jan 2016, Rich Felker wrote: > > On Mon, Jan 04, 2016 at 06:47:36PM +0300, Alexander Monakov wrote: > > > On Mon, 4 Jan 2016, Alexander Monakov wrote: > > > > 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): > > > > The "goto again" is for the rare (generally malicious) case of > > duplicate definitions, to ensure that unsetenv removes them all. > > Yes, but my point was that rewinding all the way back to i=0 looks odd -- I > understood the need to scan all entries. Oh. In that case I guess it's unnecessary to rewind, yes. BTW what might be best to do is something like: char *tmp = __environ[i]; for (j=i ; __environ[j]; j++) __environ[j] = __environ[j+1]; __env_free(tmp); where __env_free has a weak def as a nop and gets its strong def from setenv.c or putenv.c. This refactoring would make it possible for unsetenv not to pull in free, and the reordering might make it less likely for dangerous things to happen in a broken program that reads the environment concurrently with unsetenv. > > > Hm, there's no need to preserve relative order of env entries, is there? > > > > Yes, there is. If FOO=x and FOO=y both appear in environ[], > > unsetenv("BAR") must not cause getenv("FOO") to change from "x" to > > "y". > > Thanks, I did not consider that. I'm curious, is that just from QoI > perspective, or also required somewhere? It is a requirement, and it's simply a consequence of the fact that functions cannot have random side effects they're not specified to have. unsetenv can't change the value of FOO from x to y any more than calling strlen can. The fact that the issue arises at all when environ[] was not setup manually by the program is a consequence of the kernel not enforcing uniqueness of env var names, which (AFAIK) is an implementation detail anyway. Rich