From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9048 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 14:58:28 +0300 Message-ID: <568A5E64.2020105@openwall.com> References: <5689AA38.60108@openwall.com> <20160104074208.GA2016@debian> 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 1451908729 18843 80.91.229.3 (4 Jan 2016 11:58:49 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 4 Jan 2016 11:58:49 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9061-gllmg-musl=m.gmane.org@lists.openwall.com Mon Jan 04 12:58:49 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 1aG3mc-00020V-2s for gllmg-musl@m.gmane.org; Mon, 04 Jan 2016 12:58:42 +0100 Original-Received: (qmail 17767 invoked by uid 550); 4 Jan 2016 11:58:40 -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 17749 invoked from network); 4 Jan 2016 11:58:39 -0000 X-Enigmail-Draft-Status: N1110 In-Reply-To: <20160104074208.GA2016@debian> Xref: news.gmane.org gmane.linux.lib.musl.general:9048 Archived-At: On 2016-01-04 10:42, Markus Wichmann wrote: > On Mon, Jan 04, 2016 at 02:09:44AM +0300, Alexander Cherepanov wrote: >> Hi! >> >> 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 >> > > What are you talking about? free() ends the lifetime of the object > pointed to by the argument given. Right, and C11, 6.2.4p2, adds: "The value of a pointer becomes indeterminate when the object it points to (or just past) reaches the end of its lifetime." > However, after the free() only the > pointer itself is used. It won't be dereferenced again, and instead will > be immediately overwritten with a valid pointer. And while it is > possible that the pointer becomes so invalid that even loading it causes > undefined behavior, this doesn't happen on any of the supported systems > (it might happen on i386 with segmentation, but Linux/i386, which is the > only supported OS for musl, doesn't use segmentation). You presume that the bits would be loaded that were there before free(). But this is not mandated by the C standard. A compiler is permitted to completely replace the value of the pointer with, e.g., NULL (as a hardening measure, to catch use-after-free) or just use NULL as the pointer's value in the first comparison in the loop (as an optimization). It's permitted by the rules of the C standard and makes the program faster and smaller (the whole "for" loop is eliminated), so why not? IOW even if it's technically not undefined behavior (this could be debated) and not broken by current compilers it's wrong C, could be broken by future compilers, will trigger sanitizers/verifiers (I hope at least some of them will be able to catch this) etc. > So it looks like unnecessary complexity to me to apply this patch. It has its pros and cons. IMHO it's better to fix this problem (with my patch or in any other way) but that's just MHO and I appreciate that POVs vary. -- Alexander Cherepanov