* [PATCH] fix use of pointer after free in unsetenv @ 2016-01-03 23:09 Alexander Cherepanov 2016-01-04 3:05 ` Rich Felker 2016-01-04 7:42 ` Markus Wichmann 0 siblings, 2 replies; 10+ messages in thread From: Alexander Cherepanov @ 2016-01-03 23:09 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 201 bytes --] 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 -- Alexander Cherepanov [-- Attachment #2: 0001-fix-use-of-pointer-after-free-in-unsetenv.patch --] [-- Type: text/x-patch, Size: 897 bytes --] From f446b5811a8abc08bcc8202aa241dce82d4c917d Mon Sep 17 00:00:00 2001 From: Alexander Cherepanov <cherepan@mccme.ru> 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); } for (; __environ[i]; i++) __environ[i] = __environ[i+1]; -- 1.7.10.4 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fix use of pointer after free in unsetenv 2016-01-03 23:09 [PATCH] fix use of pointer after free in unsetenv Alexander Cherepanov @ 2016-01-04 3:05 ` Rich Felker 2016-01-04 10:52 ` Alexander Cherepanov 2016-01-04 7:42 ` Markus Wichmann 1 sibling, 1 reply; 10+ messages in thread From: Rich Felker @ 2016-01-04 3:05 UTC (permalink / raw) To: musl 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 The bug sounds a lot scarier than it actually is. I don't think any compilers will break this yet but it is indeed UB. > >From f446b5811a8abc08bcc8202aa241dce82d4c917d Mon Sep 17 00:00:00 2001 > From: Alexander Cherepanov <cherepan@mccme.ru> > 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]); Rich ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fix use of pointer after free in unsetenv 2016-01-04 3:05 ` Rich Felker @ 2016-01-04 10:52 ` Alexander Cherepanov 2016-01-04 11:56 ` Alexander Monakov 0 siblings, 1 reply; 10+ messages in thread From: Alexander Cherepanov @ 2016-01-04 10:52 UTC (permalink / raw) To: musl 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 <cherepan@mccme.ru> >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fix use of pointer after free in unsetenv 2016-01-04 10:52 ` Alexander Cherepanov @ 2016-01-04 11:56 ` Alexander Monakov 2016-01-04 15:47 ` Alexander Monakov 0 siblings, 1 reply; 10+ messages in thread From: Alexander Monakov @ 2016-01-04 11:56 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fix use of pointer after free in unsetenv 2016-01-04 11:56 ` Alexander Monakov @ 2016-01-04 15:47 ` Alexander Monakov 2016-01-04 21:05 ` Rich Felker 0 siblings, 1 reply; 10+ messages in thread From: Alexander Monakov @ 2016-01-04 15:47 UTC (permalink / raw) To: musl 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): > > 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]); > } > } > } > } Hm, there's no need to preserve relative order of env entries, is there? for (i=0; __environ[i]; i++); for (int im=i-1; i-->0; ) if (!memcmp(name, __environ[i], l) && __environ[i][l] == '=') { if (__env_map) { for (j=0; __env_map[j]; j++); for (int jm=j-1; j-->0; ) if (__env_map[j] == __environ[i]) { __env_map[j] = __env_map[jm]; __env_map[jm] = 0; free(__environ[i]); break; } } if (i != im) __environ[i] = __environ[im]; __environ[im--] = 0; } (in practice I'd rather spell i-->0 as --i>=0 above) Alexander ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fix use of pointer after free in unsetenv 2016-01-04 15:47 ` Alexander Monakov @ 2016-01-04 21:05 ` Rich Felker 2016-01-04 21:28 ` Alexander Monakov 0 siblings, 1 reply; 10+ messages in thread From: Rich Felker @ 2016-01-04 21:05 UTC (permalink / raw) To: musl 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. > > 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]); > > } > > } > > } > > } > > 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". However the order in __env_map is irrelevant. Its only purpose is to track which slots are allocated so we can free them. Rich ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fix use of pointer after free in unsetenv 2016-01-04 21:05 ` Rich Felker @ 2016-01-04 21:28 ` Alexander Monakov 2016-01-04 21:53 ` Rich Felker 0 siblings, 1 reply; 10+ messages in thread From: Alexander Monakov @ 2016-01-04 21:28 UTC (permalink / raw) To: musl 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. > > 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? Alexander ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fix use of pointer after free in unsetenv 2016-01-04 21:28 ` Alexander Monakov @ 2016-01-04 21:53 ` Rich Felker 0 siblings, 0 replies; 10+ messages in thread From: Rich Felker @ 2016-01-04 21:53 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fix use of pointer after free in unsetenv 2016-01-03 23:09 [PATCH] fix use of pointer after free in unsetenv Alexander Cherepanov 2016-01-04 3:05 ` Rich Felker @ 2016-01-04 7:42 ` Markus Wichmann 2016-01-04 11:58 ` Alexander Cherepanov 1 sibling, 1 reply; 10+ messages in thread From: Markus Wichmann @ 2016-01-04 7:42 UTC (permalink / raw) To: musl 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. 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). So it looks like unnecessary complexity to me to apply this patch. Ciao, Markus ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fix use of pointer after free in unsetenv 2016-01-04 7:42 ` Markus Wichmann @ 2016-01-04 11:58 ` Alexander Cherepanov 0 siblings, 0 replies; 10+ messages in thread From: Alexander Cherepanov @ 2016-01-04 11:58 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-01-04 21:53 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-03 23:09 [PATCH] fix use of pointer after free in unsetenv Alexander Cherepanov 2016-01-04 3:05 ` Rich Felker 2016-01-04 10:52 ` Alexander Cherepanov 2016-01-04 11:56 ` Alexander Monakov 2016-01-04 15:47 ` Alexander Monakov 2016-01-04 21:05 ` Rich Felker 2016-01-04 21:28 ` Alexander Monakov 2016-01-04 21:53 ` Rich Felker 2016-01-04 7:42 ` Markus Wichmann 2016-01-04 11:58 ` Alexander Cherepanov
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).