After exiting firefox(nightly, win64), MozTemp isn't deleted and stays in AppData\LocalLow\Mozilla.

Bob, is this fallout from the low integrity sandbox?

(In reply to Chris Peterson [:cpeterson] from comment #1) > Bob, is this fallout from the low integrity sandbox? Yes, this is for the low integrity sandbox. It should delete them as part of a normal shutdown, but I definitely need to do something for abnormal shutdowns. atlanto - does this happen for a normal shutdown of Firefox?

(In reply to Bob Owen (:bobowen) from comment #2) > atlanto - does this happen for a normal shutdown of Firefox? Yes, it happens on normal exit. Without any extensions/addons.(new profile)

(In reply to atlanto from comment #3) > (In reply to Bob Owen (:bobowen) from comment #2) > > atlanto - does this happen for a normal shutdown of Firefox? > > Yes, it happens on normal exit. Without any extensions/addons.(new profile) OK, thanks. This is definitely something I need to get fixed before we can move this out of Nightly.

[Tracking Requested - why for this release]: Regression

I'll be looking at this on Monday.

This is Nightly only, so shouldn't be affecting Fx40. Try push without e10s enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32d1bfad7fc7 Try push with e10s enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3903c042c459 I'm getting an access denied on that Win8 M3 test, so need to investigate further. Test passes locally (on Win7).

The problem on the test servers is that they obviously delete everything in Temp and so also the Low folder. IE recreates this, but if you recreate this yourself it doesn't automatically get set as a low integrity directory. I could look into setting the integrity level, or perhaps a new MozLow under Temp. I might just go back to AppData\LocalLow and use "Mozilla\Temp-*" instead of "Mozilla\MozTemp-*", so that I know that I can remove all the old MozTemps. I mainly wanted to put this under Temp, because people know that that is somewhere to look for temporary files to delete.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15e01dc7ef8c I've gone for the AppData\LocalLow option as even though it's not under Temp it seems like the simplest and so less error prone. The clean-up of these Temp-* directories should be much more robust now anyway, unless people play with the new suffix pref.

Like I explained in comments 2 and 3, this moves the creation of the low integrity temp to the main process and stores a fixed UUID suffix in a pref. This is to try and make sure it gets cleaned up properly, even after a crash. I took the UUID part out of directory service as I didn't want to add a dependency on the preference service into it. I changed the start of the temp directory to "Temp-" from "MozTemp-", so I can clean up the old stuff more easily in the next patch. It's in a Mozilla dir anyway, so we don't really need the Moz anyway.

I've put this clean-up code in the child process, just for safety. As it will probably be sandboxed (if not turn off), so if I've screwed up the logic somewhere, it shouldn't be able to delete anything not in low integrity areas. I'll back this out after it's been in Nightly for a reasonable amount of time (a month should do I think).

Comment on attachment 8606301 [details] [diff] [review] Part 1: Change low integrity temp to a fixed dir per profile and improve clean-up. Review of attachment 8606301 [details] [diff] [review]: ----------------------------------------------------------------- Always a fan of moving complexity out of xpcom/ :)

Comment on attachment 8606301 [details] [diff] [review] Part 1: Change low integrity temp to a fixed dir per profile and improve clean-up. Review of attachment 8606301 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentProcess.cpp @@ +36,5 @@ > > + nsAdoptingString tempDirSuffix = > + Preferences::GetString("security.sandbox.content.tempDirSuffix"); > + if (tempDirSuffix.IsEmpty()) { > + NS_WARNING("Low integrity temp suffix pref not set."); can this happen in practice? Seems like a warning might be too lenient. ::: toolkit/xre/nsAppRunner.cpp @@ +628,5 @@ > + // Get (and create if blank) temp directory suffix pref. > + nsresult rv; > + nsAdoptingString tempDirSuffix = > + Preferences::GetString("security.sandbox.content.tempDirSuffix"); > + if (tempDirSuffix.IsEmpty()) { should we check this for thins like ../ or other directory changing characters? What are the security implications if this pref gets highjacked? @@ +663,5 @@ > + return; > + } > + } > + > + // Get the low integrity Mozilla temp directory. Looks like the code between line 667 and 681 can be replaced with CleanUpSandboxEnvironment().

Comment on attachment 8606302 [details] [diff] [review] Part 2: Add temporary code to clean up the old low integrity temps on Windows. Review of attachment 8606302 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentProcess.cpp @@ +75,5 @@ > +#if defined(NIGHTLY_BUILD) > +static void > +CleanUpOldSandboxEnvironment() > +{ > + // Temporary code to clean up the old low integrity temp directories. please file a bug on removing and reference that here.

Thanks Jim and Nathan. A couple of questions. (In reply to Jim Mathies [:jimm] from comment #13) > Comment on attachment 8606301 [details] [diff] [review] > > + Preferences::GetString("security.sandbox.content.tempDirSuffix"); > > + if (tempDirSuffix.IsEmpty()) { > > + NS_WARNING("Low integrity temp suffix pref not set."); > > can this happen in practice? Seems like a warning might be too lenient. It could, but only if messing with prefs I think. If they ever reset tempDirSuffix when not at level 1 and then changed back to level 1, after a content process crash or if they were using more than one content process (and they were standing on one leg and facing east), I think it could happen. I did think about adding observers to the prefs to try and catch this sort of thing, but I think that's getting over complicated for an edge case that should sort itself after a re-start. What do you think? Either way, I'll do some more testing around that sort of scenario, before landing. (Not sure I'll manage the standing on one leg bit :-) ). > ::: toolkit/xre/nsAppRunner.cpp > @@ +628,5 @@ > > + // Get (and create if blank) temp directory suffix pref. > > + nsresult rv; > > + nsAdoptingString tempDirSuffix = > > + Preferences::GetString("security.sandbox.content.tempDirSuffix"); > > + if (tempDirSuffix.IsEmpty()) { > > should we check this for thins like ../ or other directory changing > characters? What are the security implications if this pref gets highjacked? Hmm, the Windows nsLocalFile::Append checks for that sort of thing. But I'm also using SetLeafName, which doesn't as far as I can tell. I thought that it used Append internally, but on re-reading that's just on the working path which is an nsString. So, it looks like I do need to fix this. Nathan - do you think we should fix this in nsLocalFile::SetLeafName in nsLocalFileWin.cpp. Maybe, I'm mis-reading the code, but it seems odd that we'd allow SetLeafName to change anything but the final file name. > @@ +663,5 @@ > > + return; > > + } > > + } > > + > > + // Get the low integrity Mozilla temp directory. > > Looks like the code between line 667 and 681 can be replaced with > CleanUpSandboxEnvironment(). Not quite, but I think I can do some de-duplication there, thanks. (In reply to Jim Mathies [:jimm] from comment #14) > Comment on attachment 8606302 [details] [diff] [review] > Part 2: Add temporary code to clean up the old low integrity temps on > Windows. > > Review of attachment 8606302 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/ipc/ContentProcess.cpp > @@ +75,5 @@ > > +#if defined(NIGHTLY_BUILD) > > +static void > > +CleanUpOldSandboxEnvironment() > > +{ > > + // Temporary code to clean up the old low integrity temp directories. > > please file a bug on removing and reference that here. Yes, good plan, I'll do that, thanks.

(In reply to Bob Owen (:bobowen) from comment #15) > (In reply to Jim Mathies [:jimm] from comment #13) > > ::: toolkit/xre/nsAppRunner.cpp > > @@ +628,5 @@ > > > + // Get (and create if blank) temp directory suffix pref. > > > + nsresult rv; > > > + nsAdoptingString tempDirSuffix = > > > + Preferences::GetString("security.sandbox.content.tempDirSuffix"); > > > + if (tempDirSuffix.IsEmpty()) { > > > > should we check this for thins like ../ or other directory changing > > characters? What are the security implications if this pref gets highjacked? > > Hmm, the Windows nsLocalFile::Append checks for that sort of thing. > But I'm also using SetLeafName, which doesn't as far as I can tell. > I thought that it used Append internally, but on re-reading that's just on > the working path which is an nsString. > So, it looks like I do need to fix this. > > Nathan - do you think we should fix this in nsLocalFile::SetLeafName in > nsLocalFileWin.cpp. > Maybe, I'm mis-reading the code, but it seems odd that we'd allow > SetLeafName to change anything but the final file name. Ooo, that's a good point. Yeah, it seems like we should be doing some sort of input validation here. Please file bugs/patches for fixing nsLocalFileWin.cpp and nsLocalFileUnix.cpp (which looks more lax than its Windows counterpart). Including tests for this would be great, too.

(In reply to Bob Owen (:bobowen) from comment #15) > Thanks Jim and Nathan. > > A couple of questions. > > (In reply to Jim Mathies [:jimm] from comment #13) > > Comment on attachment 8606301 [details] [diff] [review] > > > > + Preferences::GetString("security.sandbox.content.tempDirSuffix"); > > > + if (tempDirSuffix.IsEmpty()) { > > > + NS_WARNING("Low integrity temp suffix pref not set."); > > > > can this happen in practice? Seems like a warning might be too lenient. > > It could, but only if messing with prefs I think. > If they ever reset tempDirSuffix when not at level 1 and then changed back > to level 1, after a content process crash or if they were using more than > one content process (and they were standing on one leg and facing east), I > think it could happen. You should consider stashing this in the Windows registry instead to keep people from mucking with it. We currently keep things there that we don't expect users to tweak generally, like taskbar grouping ids. Also, you can access the registry even if you don't have a profile loaded yet. > Either way, I'll do some more testing around that sort of scenario, before > landing. great, thanks. > (Not sure I'll manage the standing on one leg bit :-) ). > > > ::: toolkit/xre/nsAppRunner.cpp > > @@ +628,5 @@ > > > + // Get (and create if blank) temp directory suffix pref. > > > + nsresult rv; > > > + nsAdoptingString tempDirSuffix = > > > + Preferences::GetString("security.sandbox.content.tempDirSuffix"); > > > + if (tempDirSuffix.IsEmpty()) { > > > > should we check this for thins like ../ or other directory changing > > characters? What are the security implications if this pref gets highjacked? > > Hmm, the Windows nsLocalFile::Append checks for that sort of thing. > But I'm also using SetLeafName, which doesn't as far as I can tell. > I thought that it used Append internally, but on re-reading that's just on > the working path which is an nsString. > So, it looks like I do need to fix this. > > Nathan - do you think we should fix this in nsLocalFile::SetLeafName in > nsLocalFileWin.cpp. > Maybe, I'm mis-reading the code, but it seems odd that we'd allow > SetLeafName to change anything but the final file name. > > > @@ +663,5 @@ > > > + return; > > > + } > > > + } > > > + > > > + // Get the low integrity Mozilla temp directory. > > > > Looks like the code between line 667 and 681 can be replaced with > > CleanUpSandboxEnvironment(). > > Not quite, but I think I can do some de-duplication there, thanks. yeah, that's what I meant. Lets share some of this code. > > (In reply to Jim Mathies [:jimm] from comment #14) > > Comment on attachment 8606302 [details] [diff] [review] > > Part 2: Add temporary code to clean up the old low integrity temps on > > Windows. > > > > Review of attachment 8606302 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/ipc/ContentProcess.cpp > > @@ +75,5 @@ > > > +#if defined(NIGHTLY_BUILD) > > > +static void > > > +CleanUpOldSandboxEnvironment() > > > +{ > > > + // Temporary code to clean up the old low integrity temp directories. > > > > please file a bug on removing and reference that here. > > Yes, good plan, I'll do that, thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/116d87a1a2a8 https://hg.mozilla.org/integration/mozilla-inbound/rev/ca9f095bb18c

https://hg.mozilla.org/mozilla-central/rev/116d87a1a2a8 https://hg.mozilla.org/mozilla-central/rev/ca9f095bb18c