56002 - Make path to profile dir unpredictable

Status: VERIFIED FIXED

(Core :: Security, enhancement, P3)

Description

[Mitchell Stoltz (not reading bugmail)]

--- jar, from bug 55731: ---
 b) We again are being burnt by the fact that the attacker can guess where we
will place a file (re: default user directory). At some point we need to put
some more randomization in this placement.. but we never seem to get aronud to
it... and it has burnt us many times.

Let's do this. Aside from bug 55731, we've had numerous problems in Communicator
and NS6 so far involving the reating of the default profile.

So, how to do it? If we want even more security, we should apply this fix to all
profile directories, not just the default one, as an attacker may be able to
guess other path names besides "default/" The trick is to do this in a way that
isn't too complex for the user. The simplest way would be to append a random
number to the end of the profile directory name, and store that number locally
where it can't be discovered by an attacker. That way, everything in the
directory is protected. Otherwise, we'd have to change every file that ever gets
added to the profile directory, and this would be harder to maintain.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

ccing selmer and bhuvan for suggestions. How can we do this and not make the
user experience (or troubleshooting) more complex?

Comment 2

[selmer (gone)]

I don't see this going into RTM.  I think it merits a design discussion.  There
could be a lot of unintended side-effects to making this change.

Comment 3

[Stephen P. Morse]

Appending random numbers to the end of the profile directory name won't work 
because that would send us beyond the 8.3 limit (and, yes, there are still some 
platforms that will be broken by such).  Better is to have a mapping of the name 
the user choses for his profile name (or default that we chose for him) and the 
actual profile name.  The actual profile name is generated randomly.  The 
mapping could be kept in the registry or some equivalent place (i.e., the place 
where we already store the list of profiles names and their actual paths).

Comment 4

[selmer (gone)]

Munging a number into the leaf of the profile directory name somehow is probably
not a big deal.  However, changing the location of the defaults directory still
seems problemmatic.

Comment 5

[Ben Bucksch (:BenB)]

Sorry if this is a dumb question, but can somebody please explain what this will
help? Mozilla still has to find the profile :), so if we can find, why wouldn't
the attacker find it, assuming he can read files? Unless we're doing something
freaky like "ls ~/.mozilla*" to find the profile, the path to the profile has to
be stored somewhere, and an attacker can also read and interpret that.

> Appending random numbers to the end of the profile directory name won't work
> because that would send us beyond the 8.3 limit

This is not a problem on Unix (and neither on Mac, I think), and the profiles
folder path code is not XP anyway.

Comment 6

[Jim Roskind]

The critical mis-assumption above is:

   "...assuming he can read files..."

The attacker typically cannot read files from your local disk (that *would* be 
a privacy/security breach of significance).  Indeed, the goal of many attacks is 
to read files (or directories).  

Sadly, knowing where the user's directory is placed often allows the attacker to 
*place* content at known locations in the file system... and then execute that 
trojan content... and then using that trojan content, actually read files from 
the file system.

Comment 7

[Ben Bucksch (:BenB)]

So, it is usual that exploits only allow to write, but not to read, arbitary
files? Does this (ability to writing arbitary files) include existing files? If
yes, an attacker could create a new default profile and then overwrite our
prfiles list file to point there. The user would notice it, but the attack would
succeed, not?
If a common class of exploits only allows to write new files in known locations,
this bug might indeed help.

Comment 8

[Jim Roskind]

The mistake in that last comment is the (oft repeated) statement about "writing
arbitrary files."  The attacker does not typically begin an attack with such
powers. Certainly the files "written to" are not arbitrary.  Morover, the
contens are often heavilly restricted, and also far from arbitrary.

The attacker is restricted in terms of what files s/he can (effectively) write
to.  For example, the attacker provides a page of html most of the time, and
hence gets to write to "some" file in the cache (hopefully, a file whose
path/name cannot be guessed).  Some cookie data is placed into the cookie file,
and hence the attacker has effective partial write permission to at least part
of that file.

Each and every byte of downloaded content and data, that arrives in the users
directory structure, represents a possible trojan byte of code.  If the location
of the trojan bytes are not knowable to the attacker, then an attack will be
that much more difficult to construct.

Comment 9

[Ben Bucksch (:BenB)]

Oh, I'm starting to understand. So, e.g. if an attacker knew *where* the script
he just loaded via http was cached, he could load it via file:, which would give
the script more rights.
<shut-up/>

Comment 10

[David Krause]

Ben, it's also hard to understand the purposes behind this bug considering that
we can't see what bug 55731 is about and how the exploit in that bug works.

Comment 11

[selmer (gone)]

The mapping between profile name and profile directory name is already stored in
our profile registry.  Making the directory name different from the profile name
is not difficult.  Using something other than "default" should be very
straightforward for the default case.  I doubt that attackers can easily get the
info out of our registry file, but I'll let security experts determine that :-)

So, I suggest we do this for newly created (or migrated) profiles:
	assume 8.3 names
	use default.nnn instead of default where nnn is a random (unique) number
	use <profilename>.nnn instead of <profilename> again nnn is random

I'm not sure how we would fix this for existing profiles created by mozilla
derived clients prior to the introduction of this fix.  Messing around with
existing names has always lead us into murky waters.  There are prefs and junk
that refer into the profile and we always forget some of them when attempting
things like this.  This might be an argument to getting this in before RTM...

Comment 12

[Phil Peterson]

We sometimes send people into their profile directory to fix something by hand
(add a pref, delete mail summary files). Since randomizing user-provided profile
names would make this hard to do, and the user-provided name already isn't
easily predictable, would we randomize only the default profile name?

I don't see the problem with 8.3. If that's a concern, just change "default"
into "def00000" through "def99999"

Comment 13

[selmer (gone)]

Closing the hole with "default" is a big step and probably covers the majority
of cases (Bhuvan, are most single profiles migrated named "default"?)

The next most at risk group would be people who choose their profile name the
same as their username or their actual name.  An attacker like the one described
in 55731 could reasonably expect that the user's login name on the news server
will also be the user's profile name and would frequently be right.

We _already_ have the problem that the profile name doesn't always match the
profile directory name because we have to uniquify the directory name.  In this
case, I believe we generate names like <profilename>.001 and .002 etc.  The
proposed fix would extend this to all profile directory names except the .nnn
would be random as well as unique.

If it would help, we could put a .txt file in the directory that says what the
profile name for that profile directory actually is ;)

Anyway, getting this for "default" is probably the 80% case.

Comment 14

[racham]

I guess we can say the probable name for a single profile in 4.x world is
"default". However, we have to remember that in 4.x, when the user has no
profile, he/she is always presented with a CreateProfile Wizard and gets chance
to change the name of the profile from "default". It will nice if we have some
data to back up our "default" profile name assumption here.

Today, in 6.0, the only time we create unique profile directories is when the
user tried to migrate a profile and already has a profile directory with that
name. So, we use nsIFIle's APIs to create unique directory name and the format
for unique directories is <profile name>-1, -2, ...-n (not <profilename>.nnn).

We also have to remember that getting profile directory name from the registry
might not be a hard task...

Comment 15

[selmer (gone)]

People who chose something other than default have already moved out of the high
risk category and their risk then depends on the type of name they have chosen.
 I don't think that argues against fixing this for "default" unless we have
reason to believe a majority of users changed the name (and if they did, how
come this attack is serious?)

If the attacker can interpret and use the info in the registry, then there's no
point in making any change for this bug at this time.  Does anyone know if
that's the case?

Comment 16

[Jim Roskind]

If an attacker can read your registry, then you have a securtiy breach that
needs to be fixed... it is that simple.  There is a ton of stuff about what is
installed on the current machine that is kept in the registry, and this is
typically considered confidential information.

Most users don't set up a second profile, and hence they live with the "default"
name.  This factoid has caused numerous security firedrills at Netscape (the
default profile, plus some other exploitation, which is what we typically
repaired).   I can tell you that IF we had unguessable profile names, that we
would not have been forced to make several premature (security fix) releases of
Navigator x.x.

Comment 17

[Stephen P. Morse]

I was about to say what jar just said.  The issue of an attacker reading the 
registry is tangential to this issue.

What's important here is that an attack already has the ability to write to 
several files.  Cookies is one example.  If he can write arbitrary things to 
those files and if he can guess where those files wind up, then he can do 
damage.  The fix is to block both of those ifs.  Many of our security fixes to 
date have involved blocking the first IF -- i.e., restricting what he can write 
in the file or preventing what he writes from being executed.  What is being 
proposed here is to block the second IF -- i.e., allowing him to guess where the 
file wound up.

Comment 18

[selmer (gone)]

Sounds like we're all in agreement that the registry problem is bogus (if you
read my comments again, you'll see that I was really agreeing with this
sentiment :-)

So, again, we should focus on fixing the name "default" to something much less
guessable.  Phil's suggestion of defnnnnn sounds good to me.  How long would it
take a bit of javascript to make 100,000 attempts to find the thing?  Do we need
even more randomness than defnnnnn implies?

Comment 19

[Stephen P. Morse]

Of course defnnnnn.nnn is even better.  And better yet is nnnnnnnn.nnn (why do 
we need the "def").

It's not a matter of how long it would take javascript to try all the 
combinations.  The attack typically involves the website luring you into 
clicking on a link to the file.  Now how long do you think it would take the 
website to lure you to click on 100,000 different links?

Comment 20

[Mitchell Stoltz (not reading bugmail)]

So, is the consensus to make this change for "default" only? Or for all profile
directories? "defnnnnn" or something similar will help users & tech support
people find the profile directory when necessary.

Comment 21

[selmer (gone)]

So far, the consensus is around default only.  Nobody has responded that the
users who explicitly chose a name have done so poorly that they're generally
open to attackers.

Steve, is it the case that no use of frames and the submit() or click() methods
or any other javascript can cause content to be loaded and interpreted without
the user explicitly cooperating?  I haven't tried it, but I'm worried about the
attacker's ability to generate a link and have javascript load it for him/her
without user intervention.

Comment 22

[Jim Roskind]

5 random characters are possibly enough, and certainly a drastic improvement.
Instead of a "mere" 10 to the 5th power (digits only), we'd have at least 26
letters, 10 numerals, and at least an underscore, for a total of 37 ^ 5th, or
easilly 70 million.  IF we assumed we could *try* to handle case as well, that
would be 63^5th, or just shy of a billion (most file systems support
case-sensitivity, and this support will increase over time).

IF we were trying to get "fancy" we'd offer the option to randomly generate the
profile name for the "default" user, and we could continue to show it (via UI)
as "default," even though the name would be a full 8 characters of randomness,
or at least 63 ^ 8th, or about 250 trillion (we'll need drastic speed-ups in JS
before this will be problematicly guessable!).

Comment 23

[Ben Bucksch (:BenB)]

jar,
I was about to suggest the chars :).

If I understood selmer's comments right, the profilename is already independant
of the profiledir. Even then, I think, we should keep the first "def", because
the user might want to fiddle lwith the files (e.g. to get to the raw msg folder
files).

Comment 24

[Mitchell Stoltz (not reading bugmail)]

morse, do you want to own this? Sounds like you know what to do to make this
happen. It would take me longer.

Comment 25

[selmer (gone)]

Jar - very amusing.
Ben - yes, the display name is independent of the directory name (but it would
be nice to keep at least some crumb of a correlation between them for those who
must grunge around in the directory)

Anyway, my question was, for "default" can we take "def" and append a 5 digit
NUMBER and be done with this or is more randomness than that required?  Other
more complex solutions are welcome from people willing to attach a correct patch
and check in the fix.

Comment 26

[Mitchell Stoltz (not reading bugmail)]

Using the full character set rather than just numbers is no more difficult...we
simply generate a larger random number and encode it as characters. I agree with
Ben and selmer that the directory should be easily spottable by the user, so it
should probably have come correlation to the profile name.

Comment 27

[Stephen P. Morse]

Mitch, OK I'll take a crack at this and try to come up with a patch.  The only 
thing that I'm going to have to grope for is the code that currently puts the 
profile info in the registry.  So if anyone can tell me where that is, you can 
save me some searching time.

Comment 28

[Stephen P. Morse]

Ignore my question above.  I don't need to find the registry code -- the only 
thing I need is where the "default" string comes from and I just found it (in 
mozilla/profile/src/profile.cpp).

Comment 29

[selmer (gone)]

Steve, please include Bhuvan in the reviews.

Comment 30

[Mitchell Stoltz (not reading bugmail)]

Thanks, Steve. I would be happy to review this.

Comment 31

[Stephen P. Morse]

I have a couple of patches that I'm about to attach.  One is for nsProfile.cpp 
and the other is for newProfile1_2.js.  Here are some comments on the patches:

1. The random name is generated for the default profile only.  Frankly I would 
prefer to see this used for all profiles.  In order to do that, simply remove 
the if statements added in both patches and replace them with their true parts 
only.

2. The random name is of the form xxxxxxxx.def where xxxxxxxx is an 8-digit 
random number based on the current time.  If people still think that it's 
important to use characters instead of digits in order to increase the number of 
possibilities, this will require more work.  Frankly I don't think it's worth 
it.

3. The change in newProfile1_2.js is to fix the display of the profile pathname 
that appears when you are creating a new profile.  The wizard currently displays 
the following:

   Your new settings, preferences, and bookmarks will be stored in:
   <profile pathname goes here>

In the case of the randomized name I display the leaf directory as ????????.def 
where the user actually sees the question marks.  It would be tricky to figure 
out what the actual random name is going to be at the time the wizard is running 
so I sort of punted here.  I figure the user doesn't care to see the actual 
random name anyhow.  

Let me point out that there already are some problems with this display.  
Specifically, if the user enters a new profile name, the displayed pathname does 
not get updated -- the leaf remains as the original one.  However if the user 
presses "Chose Folder" to change the folder, then the leaf of the displayed 
pathnname is correctly updated.  So, since this display is a bit flakey already,  
I saw no reason to go to great lengths to make it display the random name 
correctly.

Comment 32

[Stephen P. Morse]

Attachment: patch for nsProfile.cpp

Comment 33

[Stephen P. Morse]

Attachment: patch for newProfile1_2.js

Comment 34

[Stephen P. Morse]

Correction:  In my explanation above I said that the random name is of the form 
xxxxxxxx.def.  That's wrong -- I meant to say xxxxxxxx.prf.

Comment 35

[Mitchell Stoltz (not reading bugmail)]

Reassigning to morse. The patches look good to me securitywise, but someone more
familiar with this code should probably review it.

Comment 36

[(not reading, please use [e-mail] instead)]

these patches aren't ok yet.

1) is time_t XP code?  use PRTime structures.
2) you use "Default User" as hard coded string.  doesn't that come from a .dtd
or .properties file that will be localized for every language?

Comment 37

[(not reading, please use [e-mail] instead)]

adding selmer to the cc list.

Comment 38

[racham]

Steve Morse,

There are more cases we may need to look at.

1. Migrated profiles take a different route and they are not covered in the
above case. Case where the user's 4.x single profile name is "default" and we
migrate that. All we do today is to create "default" directory on the 6.0
default profile location. So, we need to do this check at
http://lxr.mozilla.org/seamonkey/source/profile/src/nsProfile.cpp#1515
and check for string "default" as the default profile name in 4.x is "default".

2. Also, if there are no 4.x profiles or if automigration fails we create a
default proflie named "default" (little inconsistency with FrontEnd here...) and
the directory is also named after the profile as default and that is done in the
routine CreateDefaultProfile()
http://lxr.mozilla.org/seamonkey/source/profile/src/nsProfile.cpp#1891.
But again this routine uses CreateNewProfile and you covered the issue over
there. So, only thing you need to fix the descrepancy here is to change the
define value on
http://lxr.mozilla.org/seamonkey/source/profile/src/nsProfile.cpp#93
from "default" to "Default User" (that will also fix the inconsistency we have).

bhuvan

Comment 39

[(not reading, please use [e-mail] instead)]

I agree with morse's comments.

1) we should "salt" the directory name for all profiles, not just the default
2) I don't like the "???????" showing up in the UI for the default profile.  why
can't we come up with the "salted" name and show it to the user?  or is that too
confusing?

I agree with something else selmer implied in an earlier comment:  we should
have the profile's name in the name of the "salted" directory name.

like sspitzer123123, default58520.

the user will still need to make sense of the dir name on disk.  having "salt"
only is of no help.

I don't think this will be ready by rtm.

is there an rtm subset of the problem we want to fix for rtm?

Comment 40

[Stephen P. Morse]

Let me try to address the comments raised above:

--------------------

1. sspitzer: Use PRTime structure instead of time_t

Good point.  I'll make that change and attach the diffs shortly

2. sspitzer: "Default User" should be in dtd or proterties file

That's difficult to do.  I use it in two places in my patches above -- once in a 
cpp file and once in a js file.

The one in the js file can, with a bit of trickery, use the "Default User" that 
is in newProfile1_2.dtd.  The trickery is required because a js file accesses 
strings in a .properties file but the "Default User" is currently in a .dtd 
file.  But I know how to solve that problem (I have done it in my own modules) 
by defining a dummy attribute in the xul file, initializing it from the dtd 
string, and then referencing it from the js file.

The one in the cpp file is more difficult since it has no way to access the dtd 
file containing the string.  Yes, we could add a .properties file and have it 
access it there.  But now we are repeating the "Default User" string in two 
places and if we ever change it in the future we'll probably forget that it is 
in two places.  Also we are adding a new file and now need to start modifying 
makefiles and manifest files.

Finally, note that the string "Default", which as Bhuvan pointed out is used for 
migrated profiles, is indeed hard-coded into the cpp file and cannot be 
localized.

Bottom line of all this is that maybe we should consider randomizing *all* 
profiles and not just the default one.  Then we don't have to fetch the "Default 
User" string and this problem suddenly goes away.  I was already leaning in this 
direction and now this argument makes it even more attractive to do so.

3. bhuvan: migrated profiles take a different route

If we no longer check for "Default User" (or "Default" in this case), is this 
still a problem?  If so, could you indicate what change I need to make for this 
case.

4. bhuvan: If no 4.x profiles, we create default one named "Default"

This I'm sure is no longer a problem if we randomize *all* profiles and not just 
the default one.

5. sspitzer: We should randomize all profiles, not just the default

Agreed, as noted by all my comments above

6. sspitzer: I don't like the "???????" showing up in the UI

This appears only when the profile is created.  You don't even see the path when 
the profile is selected on later uses of the profile manager.

The display is already flawed (as I mentioned above) because it still keeps 
displaying the default name even after the user has entered a new name in the 
profile-name field.

Finally, the profile wizard doesn't know what the randomized name will be 
because it hasn't been randomized yet.  The randomization will occur in the cpp 
code after the user clicks finish.

So I think that the question marks are the best that we can hope to get for rtm.  
It could always be fixed in the future but, frankly, I doubt if it's worth ever 
doing so.

7. sspitzer: we should have the profile's name in the random directory name.

Yes, this would be a good idea so that we can tell the user where to find his 
profile.  However, simply appending the random digits to the end of the profile 
name won't work because it will push us over the 8.3 limit (that limit is still 
needed on some platforms).

A better solution to this problem is to have a text file of a fixed name (e.g., 
profile.txt) inside each profile directory and that file contains the name of 
the profile (see selmer's comment of 2000-10-15 22:34)

----------------------------------

OK, I think I've addressed all the comments.  So now I'll put up the revised 
patches.

Comment 41

[Stephen P. Morse]

Attachment: revised patch for nsProfile.cpp

Comment 42

[Stephen P. Morse]

Attachment: revised patch for newProfile1_2.js

Comment 43

[Stephen P. Morse]

I forgot to mention that my revised patches do not cover item 7 above.  I 
recomment that that be done post rtm.  If others feel that we need to do that 
prior to rtm, then I'll come up with a quick patch to do that.

Comment 44

[Stephen P. Morse]

Of course there's an alternative way for the user to find where his profile 
directory wound up.  Specifically he would run regexport and redirect the output 
to a file.  Then he would use his favorite text editor to read the file, find 
his profile name, and see the corresponding directory pathname.

Comment 45

[Mitchell Stoltz (not reading bugmail)]

I agree that we should "salt" all directory names if possible, not just the
default directory. However, I agree with Seth that the user should be able to
recognize a particular profile directory on their disk, preferably without
having to consult some sort of mapping file. "profilename23456" is the obvious
way to do this. I know it would be more work, but can we do it this way on
platforms which do support long filenames (this is the majority of platforms,
I'm sure)? I'm anticipating problems and complaints when a user is asked to find
their profile directory among several - they would either have to look up the
correct profile directory in a table, or open them up and look through the
contents, or potentially choose the wrong one to modify.

Comment 46

[selmer (gone)]

Where, specifically, do we need to keep profile names to 8.3?  Wouldn't the OS
just do the ~ thing in those cases?

Even granting that 8.3 is sometimes required, the original proposal had at least
the first 3 characters of the profile's name preserved.  It isn't a lot, but
it's better than nothing.  Of course, this leads us back to the discussion about
the amount of randomness required...

I don't see any argument against obscuring all NEW profile names.  Just stay
away from attempting to rename existing ones.  I don't think we should create a
new file in the profile at this time.  As Steve pointed out there is another way
to get that info (even if it's very difficult.)

Comment 47

[Ben Bucksch (:BenB)]

I also agree with Seth and Mitch. Munging in the profiles dir is still a common
task for users. Examples:
- Edit prefs.js - the only way to set hidden prefs, e.g. to add the "do not
strip domainname in POP account name" pref
- Move/Save/Convert/Rescue local mails
I don't see completely randomized profile names as an option. (However, I fail
to understand why we are still restricted by 8.3 filenames, but well.)

I think, 3 numbers or even 3 chars (37^3 ~= 50000) of randomness are not enough,
because might be able to programmatically (JS) try all cases.

Add all 3 requirements (partly readable name, 8.3, large radomness), and I don't
see how you could randomize all profile dirs. However, "defCCCCC" would still
work. (If the random chars are a real problem, just use numbers for now, and I
*might* hack the code to use chars later.)

Comment 48

[selmer (gone)]

Is there any way to make this attack _without_ inducing the victim to click on a
preformed link to the infected file?  I don't see an answer about whether some
bit of JS can be gotten executed that could attempt all possible combinations if
the randomness isn't great enough.

I would rather not introduce a false sense of security, so I don't think we
should take this patch unless the randomness is sufficient for the life of the
product.

Comment 49

[Ben Bucksch (:BenB)]

selmer,
I don't know the actual incidents (Mitch and jar will have to fill in here), but
JS is so powerful these days (and getting more powerful all the time) that I
don't see anything off-hand that could be triggered by a link-click, but not JS.
E.g. |document.location.href=malitiousURL|.

8.3: Let's say we add 5 random chars to the profile anme and call this the
prodile dir (something like this is the plan, not?). On Win95, this would mean:
mitchH7L4V -> mitchH~1 and defaultZ5B9 -> defaul~1, right? Effect: 0 :-(.

Comment 50

[Stephen P. Morse]

It seems to me that all the objections I'm hearing above (about the user being 
able to find the profile directory) would be solved if the user could easily see 
a mapping of profile name to leaf-directory name.  Suppose I modify the 
profile-manager display so that instead of just showing a single column of 
profile names, it shows two columns -- one with the profile names and the other 
with the leaf-directory name.

I'll try to work up a patch to accomplish this.

Comment 51

[Ben Bucksch (:BenB)]

morse, I don't think that would help. In most cases, the user never sees the
profile manager (because tehre is only 1 (profile). Even if he doesn, he him
have ignored these odd numbers all the time and when a tech support says to open
the profile dir, the user doesn't remember the number.
Example:
From mailnews.js: pref("mail.allow_at_sign_in_user_name", false);  //strip off
chars following the @ sign in mail user name
You are an ISP using a "[e-mail]" POP accountname scheme. How
would you explain to a user how to set up the account in Messenger? With your
suggestion, the description gets several steps longer and more failure-prone
(with means lost $ for companies).

Comment 52

[Ben Bucksch (:BenB)]

BTW: While this was just one example (there are many more), it was not a
randomly picked one. Many ISPs in Germany require an "@" in the account
username, see bug 57129.
Also, of course, "more failure-prone" doesn't only mean lost $ for ISPs (through
tech support), but also real problems for users. E.g. user messes up prefs.js ->
navigator doesn't work as expected anymore -> user tries to fix it -> doesn't
work -> user this "start from scratch" and deletes profile -> user lost all
mails (of course, this is a worst-case scenario, but I *was* in such situations
before).

Comment 53

[Stephen P. Morse]

Attaching diffs for the change to display the leaf of the profile directory 
along with profile name in the profile manager window.  Also attaching a screen 
shot showing what the profile manager now looks like.

Comment 54

[Stephen P. Morse]

Attachment: patch to profileManager.js (displays leaf name with prof name)

Comment 55

[Stephen P. Morse]

Attachment: patch to profileSelection.js (displays leaf name with prof name)

Comment 56

[Stephen P. Morse]

Attachment: Screen shot showing display of leaf name with profile name

Comment 57

[Stephen P. Morse]

Ben, I'm not sure I understand the example you are trying to present.  Could you 
please go through it step by step (without editorializing).

Comment 58

[Stephen P. Morse]

A word of explanation about the screen shot attached.  The profiles "def1" and 
"morse" were old profiles created prior to any of these changes.  So the leaf 
directory name for these profiles equals the profile name.  That is why no leaf 
directory name is displayed for them.  All the others are newer profiles which 
have random leaf names.  The leaf names are displayed in those cases.

Comment 59

[(not reading, please use [e-mail] instead)]

I've got a suggestion:

we now have:
.mozilla/<profile name>/prefs.js

to fix this bug, let's do:
.mozilla/<profile name>/<salt>/prefs.js

the user can find there files
the salt is there, and can 8.3 or bigger depending on the OS.

there is already code in the tree to take a string and "hash if necessary" so it
is safe for the underlying file system.  right now that code lives in
nsMsgUtils.cpp, but this would be the excuse to move it out.

<salt> should be the only directory under <profile name>

and, as far as UI goes, the users profile is still in <profile name>, the there
is no need to change the ui.

this should also be a pretty easy as far as implementing a fix.

comments?
 

Comment 60

[Stephen P. Morse]

One more bit of polish to my patches.  Currently the patch is fetching the 
current time in microseconds and taking the last eight digits.  It turns out 
that on win32, that time is really in milliseconds and the last three digits are 
always zero.  So the revised patch for nsProfile.cpp converts the time to 
milliseconds before taking the last eight digits.

Comment 61

[Stephen P. Morse]

Attachment: new patch for nsProfile.cpp (uses milliseconds instead of microseconds)

Comment 62

[(not reading, please use [e-mail] instead)]

I'm strongly against having:

"def33 == 3242342342334" in the profile UI to represent "sspitzer"

that's a really bad UI.  adding ben since he owns the profile manager UI.

any comments on my suggestion on how to implement this?

Comment 63

[selmer (gone)]

Steve, I appreciate your zeal, but we can't use the patches that change the
profile manager UI.  The idea of showing this info has been entertained and
rejected at least twice.  There is no support for this solution at any point in
time.

Additionally, we need to avoid making ANY ui change in this fix if we want RTM
approval.  The UI freeze and the L10N deadline have both passed and a solution
requiring such changes will simply be rejected.

We don't require the UI changes for RTM in any case.  If the user explicitly
chose a directory, we should notice and honor that.  If they did not do so, we
should apply our munging rules.  The fact that the directory is then different
is not significant enough to worry about for the RTM.

Please don't attach another patch until we have some agreement on the final
solution.

Comment 64

[selmer (gone)]

Seth, I think you've hit on the correct solution - for the trunk.  It might also
be the correct solution for the branch, but I suspect the patch is large and the
risk is also not trivial.  At the least, the profile manager isn't expecting
this extra directory level and could easily be horked by it.  I hope other parts
of the product won't care about that extra level, but the only way to know for
sure is to actually try it.  Can you and Bhuvan get together and figure out how
much change and risk this really is for the branch?

Comment 65

[selmer (gone)]

Unless Seth & Bhuvan come back with a small, safe patch for his proposal, I
think we still need to do some direct name munging for RTM.  I don't feel like I
got a good answer about the amount of randomness required.  It does sound like 5
digits isn't considered enough by some.  We could get more digits by using the
defnnnnn.nnn approach or we could switch to allowing characters (both upper and
lower case) and digits.  If we expand into characters, I'd like to go with
defaulnn.nnn so we get some more characters from the profile name.  Any other
proposals?  Any agreement?

Comment 66

[Stephen P. Morse]

OK, I'll forget the UI changes.  Are we back to considering the changes only for 
the default directory and not for the user directory.  In that case we have the 
problem of hard-coding "Default Profile" into the nsProfile.cpp file rather 
thanhaving it in a .properties or .dtd file.  Is there any objection to doing 
that?

Comment 67

[Stephen P. Morse]

Instead of defnnnnn.nnn, how about what I'm currently doing which is 
nnnnnnnn.prf (or .def if you prefer)?  That's cleaner to implement.

Comment 68

[selmer (gone)]

I'm not objecting to doing this for all user names, only in the case where the
user explicitly chose a directory.

I don't like nnnnnnnn.prf because it implies NONE of the characters from the
profile name are present in the directory name.  Similarly for nnnnnnnn.def
unless you'd also do nnnnnnnn.sel for selmer - but that would be wierd.

If we could use upper + lower + digits and just 3 characters, I'd rather go with
8 characters from profile name + dot + 3 characters randomness.  Is 62^3 =
238,328 combinations random enough?  If not, then 7 chars profile name + 1 char
random + dot + 3 chars random should give 62^4 = 14,776,336 combinations and
that aught to be enough for RTM.

Sorry if that's harder, but the usability aspect is not trivial.  People need to
be able to find this stuff when things go wrong and the absence of any clue will
simply cause people to give up.

Comment 69

[Stephen P. Morse]

OK, we have 11 positions to play with (8 before the dot and 3 after).  So we 
will use the first x positions to mirror the head of the profile name and the 
last 11-x positions for the randomness.  Now it's a matter of deciding as to 
what x should be.

Comment 70

[Ben Bucksch (:BenB)]

selmer,
re amount of randomness: (all IMO) 5 chars are enough. 4 chars are minimum. 5
digits are OK for rtm, if chars give you implementation headache - we can still
change to chars later. 4 digits are not enough.
morse,
see the bug I reaferenced for a more detailed description of the
@-in-pop-account-name example. My point was that normal (even novice) users
*have* to go into the profile dir sometimes, and the example tried to proof this
claim.

Comment 71

[selmer (gone)]

OK.  5 digits are not enough.  As I said previously, I don't want any solution
that doesn't actually solve the problem correctly.

Steve, are you willing to rework your fix to use upper/lower/digits rather than
just digits?  If so, then use x=6.  If not, then use x=4.

Bhuvan, can you help make sure we're only changing directory names when the user
did NOT explicitly choose a name?

Are we agreed now?

Comment 72

[Stephen P. Morse]

OK, I'll get started right now reworking my fix to use upper/lower case 
characters and will use x=4.

Comment 73

[selmer (gone)]

?

If you're doing upper/lower/digits, then x=6.

Comment 74

[(not reading, please use [e-mail] instead)]

working on a small safe patch to do <profile name>/<salt>

Comment 75

[racham]

Steve Morse,

1. We have a clear separation for the users who didn't specify a dir of thier
choice to store profiles vs those who did. Inside the ::CreateNewProfile(), you
have a if/else loop (starting at
http://lxr.mozilla.org/seamonkey/source/profile/src/nsProfile.cpp#1043). You
should be doing your changes in if loop which is where all those who didn't
specify the profile dir explicitely. You can see a comment to that effect in
that loop.

2. Given that, now you need to take care of special default case we have in
routine CreateDefaultProfile routine too. Lines 1917 through 1919
(http://lxr.mozilla.org/seamonkey/source/profile/src/nsProfile.cpp#1917)
should be replaced by your new randomized directory name routine...and the
secind param on the function call on line 1922 should be changed to
profilePath.GetUnicode() to be in line with CreateNewProfile function declaration.

3. Also, there is a migration case I talked about yesterday.
http://lxr.mozilla.org/seamonkey/source/profile/src/nsProfile.cpp#1532
 and line 1533 should be replaced by your new randomized dir call.

4. Finally, I think it will be better if you can make AppendRandom to return
nsresult and check for the success of same at all places where it is called. You
are doing a Append operation inside that function (which returns some rv). It is
crucial that we succeed there and also some of the code pieces you are reaplcing
have been checking for the return value.

Comment 76

[Stephen P. Morse]

I'll be off-line for a few hours so I want to post what I have before I leave.  
This is a temporary patch for nsProfile.cpp which adds the following to my 
previous patch:

1. Random names are of the form XXXXXXyy.yyy where X's are from profile name and 
y's are random characters

2. Random characters were chosen from the set of 26 lower case letters, 10 
digits, and underscore.  I did not use upper-case letter because some platforms 
are case-insensitive on their file names (e.g., win32).

3. Made the fix described by Racham's comment #1 above

I still need to digest the rest of his comments and add them to the patch.

Comment 77

[Stephen P. Morse]

Attachment: interim patch for nsProfile.cpp -- uses letters as well as digits

Comment 78

[(not reading, please use [e-mail] instead)]

I've a fix for this.  racham and I are going to meet and make sure I've gotten
all the places where I need to do it.

I'll attach the patch.

Comment 79

[(not reading, please use [e-mail] instead)]

Attachment: first attempt at a fix. still need to work with racham.

Comment 80

[(not reading, please use [e-mail] instead)]

ok, racham helped me find the other case (migration).

migration, -CreateProfile, profile wizard, silent creation of "Default User" all
work.

r=racham

I'll attach the latest patch.  the one open question is:

is 8 characters of 0-9 enough of a salt?  (10^8)  if not, I can do 8 characters
of 0-9,A-Z which would be (36^8).

(note, not a-z, since on some filesystems a == A)

Comment 81

[(not reading, please use [e-mail] instead)]

Attachment: second version of my fix, r=racham

Comment 82

[(not reading, please use [e-mail] instead)]

I am seeing srand() wrong.

I've got a new patch that fixes that and uses 0-9,A-Z.

taking this bug from morse.

Comment 83

[selmer (gone)]

How will these patches work with I18N?  I forgot to ask that this morning, sorry.

I don't see in Seth's patch where we're leaving it alone if the user chose it
explicitly.  Is it just that we're already within that context?  (I'm not
talking about the existing check, I'm talking about what the user did in the UI.)

Comment 84

[selmer (gone)]

Sorry, didn't realize this hadn't been marked rtm need info.  Doing that now.
Please update to rtm+ when the patch is r= & sr=.

Comment 85

[(not reading, please use [e-mail] instead)]

Attachment: new better patch, correct seeding of srand() with PRTime. uses a-z,0-9

Comment 86

[(not reading, please use [e-mail] instead)]

i18n is safe, since a-z,0-9 is safe no matter what the system char set it.

even if the user picks the path, we still do the salting.  that is by design.

racham, can you review?
alecf, can you super review?

Comment 87

[Ben Bucksch (:BenB)]

Seth, I thought it was a requirement (as stated by selmer) not to alter paths
specified by the user? I aqree with selmer, because in this case, the user
explicitly expressed a wish (which usually means, he cares that it is done
exactly that way), and the problems this bug is supposed to fix doesn't exist.

Comment 88

[Ben Bucksch (:BenB)]

I should note that I don't care about the user-specified path for N6.0.

Comment 89

[(not reading, please use [e-mail] instead)]

after talking to racham, I'll make it so if the user specifies the directory, we
don't salt.

here's one good reason:

if you have a profile directory somewhere, you'd need to be able to set a
profile *right* to that directory.  the extra salt would mess you up.

I'll make the fix, and get it reviewed and rtm+.

Comment 90

[(not reading, please use [e-mail] instead)]

racham and I are going to rtm- this bug.

there are too many edge cases to do this right, and I feel any fix that
addressed them all would be too risky for 6.0, especially with a few days left.

regressing the profile code at this point would be a bad thing.

since 4.x has this problem, I don't feel bad about shipping 6.0 with the same
problem.

this will be something we'll think through and test fully on the tip (maybe for
6.5?  not sure)

note:  I don't plan on checking in anything on the branch or tip, since we still
don't have a complete solution.

at some point we'll discuss all the edge cases, and how to handle them.

Comment 91

[Ben Bucksch (:BenB)]

Seth,
but please don't post-pone this bug into the unknown future. Even if it doesn't
make N6.0, we should still try to work it out and get into the tree (trunk)
soon, assuming you are all still interested.

> you'd need to be able to set a
> profile *right* to that directory.  the extra salt would mess you up.

Completely agreed, I had the same concern, but didn't dare to speak up,
considering the time pressure. MS Outlook Express (or was it Outlook?) is very
bad in that it creates a single folder called after the profile name under the
profile dir. I was always confused which folder to select - I remember vaguely
some very frustrated hours trying to migrate an old msg base.

So, if you go with the extra subfolder, make sure that all code that selects a
profile folder works regardless of which folder (the one with the human-readble
name or the random one) is selected.
(I just realize that we might have the same problem already with our multiple
profiles, if the user is not aware that "default" is the real profile, not the
profiles folder. Did this never turn out to be a problem on Windows?)

Comment 92

[Ben Bucksch (:BenB)]

> assuming you are all still interested

This part shouldn't have made it into the submission, sorry. I didn't mean to
imply that you don't care.

Re chars: I think, you should use lower and upper chars. Platforms on which the
case is not significant won't be hurt by that. OTOH, platforms on which it is
get extra randomness.

Comment 93

[(not reading, please use [e-mail] instead)]

racham and selmer may have resurrected this with a simple idea on when to salt.

marking need info again while I work on it.

Comment 94

[(not reading, please use [e-mail] instead)]

Attachment: newest version of the fix. this one only salts if the directory doesn't already exists.

Comment 95

[Stephen P. Morse]

I have a comment on seth's patches.  Namely the use of PR_Now() to obtain the 
salt.  From the tests that I ran with my own patches, I observed that PR_Now() 
is returning the time in milliseconds and right-padding with three zeroes to 
produce a time in microseconds.  At least that's what is done on win32.  In 
other words, the last three digits of the salt are being wasted.  That can 
easily be corrected by dividing by 1000 before doing anything with the result of 
PR_Now().

Comment 96

[(not reading, please use [e-mail] instead)]

morse:  I salt rand (using srand) with the number of seconds since the epoch.

PR_Now() / 1,000,000

see the latest patch.

Comment 97

[Stephen P. Morse]

OK, I just noticed the division by 1,000,000 (before reading your last posting).  
So now my question is why are you converting to seconds instead of to 
millisconds.  If someone was trying to guess your seed based on the time he 
thought you created the directory, he'd have it 1000 times easier to guess if 
you used seconds instead of milliseconds.

Comment 98

[Stephen P. Morse]

Also, why are you generating a salt (based on current time) and the using rand?  
This makes sense if you were generating a sequence of random numbers.  But since 
you only need one random number, the salt itself is just that.  In other words, 
go with the current time as the random number.

Comment 99

[(not reading, please use [e-mail] instead)]

I agree:  use milliseconds and not seconds for the salt of rand.

using the milliseconds as the directory name seems less safe than generating a 8
random characters.

Comment 100

[selmer (gone)]

Tangent alert!

These minor corrections are solving a non-problem.  Please take the final patch
for r= and sr= and get this into [rtm+] state ASAP.

Comment 101

[Stephen P. Morse]

Non issue or not, why not get it optimal?  It's just a matter of change 
1,000,000 to 1,000.

Comment 102

[(not reading, please use [e-mail] instead)]

Attachment: same fix, but div 1000 instead of div 1000000.

Comment 103

[(not reading, please use [e-mail] instead)]

I asked racham to review last night, I follow up on the review today.

then, I'll get a super review.

Comment 104

[racham]

r=racham (for patch #17619).

Here the cases we need to be testing :

We salt, if the profile directory does not exists already..

1. CreateNewProfile wizard (-profilemangaer or -profilewizard)
2. -CreateProfile option
3. Silent profile (named "default") creation (no 4.x && 6.0 profiles)
4. Someone deletes the profile folder from the disk
5. All migrated profiles

I will also test the patch.

Comment 105

[(not reading, please use [e-mail] instead)]

more test cases:

2a) -CreateProfile <profile>

2b) -CreateProfile "<profile> <path to directory that exists>"

2c) -CreateProfile "<profile> <path to directory that doesn't exist>"

Comment 106

[racham]

Seth,

Here is the case we may run into problems :

1. I create a profile say "foo", under default location (say the salt is an2t39s4)
2. So, I have $HOME/.mozilla/foo/an2t39s4 folder for that profile
3. I do bunch of custimozations
4. Quit the app
5. Run profilemanager and delete profile entry 'foo' but don't delete files
6. After that, let's say I want to use the same the dir
7. So, if you open the proflie wizard and type that the profile name you just
deleted ('foo'), it will go and look for existence of $HOME/.mozilla/foo. We
find it's existence and we try to use it without salting now. So, contents under
$HOME/.mozilla/foo/an2t39s4 are ignored. Remember a key point here, we just
typed profile 'foo' in the wizard and the app got the default dir
($HOME/.mozilla) and added the profile name 'foo' to it and then checked the
existence...
8. In order to avoid let's say we choose that folder explicitely, in that case,
we type the profile name to be 'foo' and the choose the directory to be
$HOME/.mozilla/foo/an2t39s4. So, now we do our routine again, where we append
the profile name and check for $HOME/.mozilla/foo/an2t39s4/foo and that will not
exist and we salt again..ultimately, we point somewhere else..

We always append the profile name for the reason that we want to protect users
from choosing root folders profile location and then later trying to do "Delete
Files" operation on such case.

Comment 107

[Ben Bucksch (:BenB)]

> We always append the profile name for the reason that we want to protect users

Gee, don't do that. That's was exactly the problem with Outlook (Express) I
described above.

Do I exactly what I say (i.e. accept exactly the folder I entered), just
tolerate a possible salt dir.

Comment 108

[Jim Roskind]

Hmm... racham's comment confused me.
Just to be clear, assuming the salt directory name is an2t39s4.
Then the default user would have all data in: $HOME/.mozilla/an2t39s4/default/*
IF a profile was created for "foo," then all their data wolud be in:
$HOME/.mozilla/an2t39s4/foo/*
There is no reason to hide profiles for one user from another user on a single
machine.  There is no apparent reason to believe that profiled users don't have
read access to the file system, and hence this level of hiding would have no impact.

Racham's scenario above showed the salt directroy *under* the profile name
directory, and I didn't see any motivation for that.

Which of us is confused?  If I'm confused, what was the motivation for putting
the salt under the profile named directory?  It also seemed (as I thought abotu
this in my mind at least) that coding changes would me fairly small when the
salt came before the profile name in the path.

Comment 109

[(not reading, please use [e-mail] instead)]

the scenario racham points out is a real problem.  nice job in finding it.

jar, your confusion comes from this:

we append the salt after the profile name, not before it.

we do .mozilla/profile/salt not .mozilla/salt/profile

moving the salt would solve this problem, but I haven't investigated how
feasible this is.  I'll do that now.

Comment 110

[Alec Flett]

PLEASE put it under the profile name, not above like jar suggested.
The benefits are entirely for the developers - easier to manage profiles by hand 
- but if we have a choice let's at least help someone!

Comment 111

[Jim Roskind]

Again confused: Why would putting the salt lower help a developer?

Comment 112

[Stephen P. Morse]

Not I'm getting confused.  Jim asked the question:

   Racham's scenario above showed the salt directroy *under*
   the profile name directory, and I didn't see any motivation
   for that.

I thought that the reason for this was to help the user find his profile.  We 
tell him to look in the folder that he specified and then for the profile name 
that he specified.  Then under that he finds a single salt that he ignores.  If 
we do it the other way, then he might find several salts before finding his 
profile name and he wouldn't know which one to traverse -- he would have to 
traverse them all until he found the one that contained his profile name.

Wasn't that the motivation for what you did or am I confused?

Comment 113

[(not reading, please use [e-mail] instead)]

morse and alecf are correct:  having the salt after the profile was to help the
user find there files on disk.

Comment 114

[Jim Roskind]

I presumed there would be a single salt directory, and under that *all* profiles
would be placed.  There is no real advantage to having multiple salt
directories, one per user.  We're hiding these names from generic malicious web
content, not from a fellow user that has a profile on the same machine.

Comment 115

[Stephen P. Morse]

Jim, how would we know that a salt directory exists?  Consider this scenario.  
User creates a profile foo1 in the default folder.  So he gets 
.mozilla/an2t39s4/foo1.  Now he wants to create profile foo2.  How does the 
browser know that an2t39s4 is the salt as opposed to some other subdirectory?  
It doesn't so it creates some other salt and puts foo2 under that salt.

Comment 116

[Stephen P. Morse]

I think the problem that Racham raised can be fixed in his step 7:

   7. So, if you open the proflie wizard and type that the profile name
   you just deleted ('foo'), it will go and look for existence of
   $HOME/.mozilla/foo. We find it's existence and we try to use it
   without salting now.

Once we find it's existence and we know we are in the default folder, we should 
use it by assuming that it already contains a single folder which is the salt.  
So we don't resalt but we do use the salt that is already there.

Comment 117

[Jim Roskind]

In answer Morse's question: How would we know a salt directory exists?
My expectation was that we'd list the name of our salt directory in our
registry.  This is (I'm guessing) the same place as we currently list the names
of profiles.

The hack of having the salt directory be a solo-directory to aid in its
identification is not needed if you record the name of the directory.

As I said, I had a simple picture in my mind, and I was not groking the
advatages of this alternate strategy.  I don't think there are great problems
with either, but my picture seemed (to the uneducated kibitzer) simpler.

Comment 118

[Stephen P. Morse]

If we add new information to our registry, we'd have to make sure it was 
done in a backwards-compatible manner.

Comment 119

[selmer (gone)]

Why can't the new problem be fixed by changing Bhuvan's step 7 to check for the
existence of the salt directory?  By definition there should be exactly one
directory with a salted name in there.  We might want to add a .slt extension or
something evil like that to make our identification more accurate.  I believe
this would result in a correct solution.

Jar's proposal isn't any worse and I don't see that it confuses the issue overly
much (again, I would add .slt or something to the name of that directory so it's
easier to identify and document.)

There isn't really a backwards compatibility problem, but Bhuvan has had to deal
with this for other registry fields.  It just makes the total solution more
complex is all.

My vote is to fix step 7 as I suggested in the first paragraph if that results
in the smallest correct patch.

Comment 120

[racham]

Adding .slt for the salted dir seems to be reasonable. Adding salt entry into
profile registry is certainly more work at this point of time.

Comment 121

[(not reading, please use [e-mail] instead)]

Attachment: fix, that checks before salting.

Comment 122

[(not reading, please use [e-mail] instead)]

in this latest fix, I do the following:

AddLevelOfIndirection(dir) checks if dir/prefs.js exists.  if so, just use dir.

if not, check if dir/<foo>.slt exists, where <foo> is 8 characters and <foo>.slt
is a directory.  if it exists, use it for dir.

else, generate <bar> and use dir/<bar>.slt

racham, please review and test.

Comment 123

[selmer (gone)]

Is prefs.js completely reliable?  I'm worried that there's an edge case where
it's valid to not have a prefs.js (or not have it YET when this happens.)  Would
it be safer to see if there are any contents other than a single .slt directory?

Comment 124

[Stephen P. Morse]

selmer's right, when you first create a profile there is no prefs.js file.  That 
file doesn't get created until the first time that you use the profile.  If you 
simply create it with the profile manager but don't start the browser using that 
profile, you have no prefs.js file.

Comment 125

[(not reading, please use [e-mail] instead)]

there are probably edge cases lurking where there is no prefs.js file.

the question is:  if they don't have prefs.js, what would be lost by doing the
extra salt?  no prefs == no mail or news accounts.

steve scenario:

create an account "foo", delete it but leave the files, create "foo" again.
there is no prefs.js, but there will be a "xxxxxxxx.slt" directory under
.mozilla/foo, so we'd just use it.

if "foo" was a profile created before this patch (so there was no .slt
directory), but never used, deleting and then re-adding "foo" would set the
directory to .mozilla/foo/<salt>.slt

but since the profile was never run, what does it matter?

Comment 126

[Alec Flett]

I agree with seth - prefs.js is really the only way to know if a profile has 
been used... I don't know how soon the file is created, but it's pretty 
early..

and if there is no prefs.js, and the profile dir hasn't been used, then it's 
safe to re-use that dir anyway.

Comment 127

[racham]

Also, we don't need to worry about the users who deliberately moved or deleted
prefs.js file.

I looked at the patch. It looks good. Again, let me try couple of cases and see
that we are doing good..

Comment 128

[racham]

Seth,

An update to internal profile data structure to consider new salted directory is
missing in the patch which will affect windows and linux users...but it is very
easy to fix.

Scenario :

1. Created a profile 'foo' in the past
2. Let's it is created under $HOME/mozilla/foo
3. Let's say I wiped out my folder foo from $HOME/mozilla
4. Then I used new product with the salted strategy
5. So, we see $HOME/mozilla/foo missing and we create one
6. Then we salt. say I got $HOME/.mozilla/foo/sadf87fd.slt
7. Then I dump all default profile files into $HOME/.mozilla/foo/sadf87fd.slt
8. I run the app...
9. If you go and look at you $HOME/.mozilla/foo folder you will find files like
panels.rdf, localstore.rdf, prefs.js along with sadf87fd.slt
10. We should not see any other files or directories over there other than
sadf87fd.slt

What went wrong :

Between step 6 and 7, you need to update the profile data structure so that it
knows about this newly salted dir..

One of the ideas for fixing this :

You can simply use SetProfileDir() routine after you are done with salting in
this location and that should take care of the rest. (location : nsProfile.cpp
874,884 in your patch)

Other than this, I didn't see any major problems..

Comment 129

[racham]

In the step 1, when I mentioned created profile 'foo' in the past, referring to
creation of profile using builds without this patch..

Comment 130

[Ben Bucksch (:BenB)]

> You can simply use SetProfileDir() routine after you are done with salting in
> this location

Isn't that potentionally dangerous, in the case we misinterpreted the profile as
empty before?
E.g. I wiped my prefs.js only, but left my mail folders in there. You think, "no
prefs.js -> no profile -> salt -> set to new profile dir", so the old mail
folder won't be found.

Wouldn't it be better not to "salt", and just use the existing profile dir?

Comment 131

[(not reading, please use [e-mail] instead)]

good catch on the edge case.  I'll attach a new version of the patch with
racham's suggested fix.

Comment 132

[(not reading, please use [e-mail] instead)]

Attachment: latest version of the fix.

Comment 133

[racham]

Ben, if the user deletes prefs.js, he/she lost all the mail/news related prefs
and lots of other settings too..So,I don't think we should be trying rescue
those  who deletes their prefs.js file..

Comment 134

[racham]

Seth,

patch looks good to me. r=bhuvan

Comment 135

[selmer (gone)]

I'm a little worred about just returning from AddLevelOfIndirection if a call to
Exists() fails.  My initial impression is that it's better to assume it doesn't
exist in that case and continue to create the salt directory.  Granted, I didn't
look at the implementation of Exists() and I don't know what kinds of things
make it fail - that's part of why I'm concerned :-)

Comment 136

[Alec Flett]

that's an XPCOM failure, meaning something when horribly wrong. as always, we
should be using NS_ENSURE_SUCCESS(rv, rv) so we at least get an assertion

Comment 137

[(not reading, please use [e-mail] instead)]

current status of this beast:

I've got a r=racham.

from alecf's comments, he'd like to see me use NS_ENSURE_SUCCESS(rv, rv).

I'll add that, get racham to re-review, and get work on getting alecf to super
review.

Comment 138

[(not reading, please use [e-mail] instead)]

Attachment: same fix, using NS_ENSURE_TRUE()

Comment 139

[(not reading, please use [e-mail] instead)]

r=racham, sr=alecf

marking rtm+.

PDT:  should this land on the tip first, and then the branch?

Comment 140

[selmer (gone)]

This bug is in candidate limbo.  We will reconsider this fix once we have a 
candidate in hand, but we can't take this fix before then.

Comment 141

[(not reading, please use [e-mail] instead)]

fix landed on trunk.

Comment 142

[Conrad Carlen (not reading bugmail)]

No matter what the name of the profile dir is or where it is, there is
scriptable code to find it. See:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/sidebar/src/nsSidebar.js
#385. How are we protected from this? If attacking js could use directory
service, this patch isn't going to help.

Comment 143

[Ben Bucksch (:BenB)]

ccarlen, does a webpage (which is probably what an attacker would use) have
access to that? ("Scriptable" does not imply that.)

Comment 144

[(not reading, please use [e-mail] instead)]

conrad, if they can do that they can use any of the scriptable interfaces.

Comment 145

[selmer (gone)]

We are moving toward the candidate.  Please check this fix into the trunk so we 
can get get some cook time.

Comment 146

[(not reading, please use [e-mail] instead)]

I landed this on the trunk last night.

Comment 147

[racham]

Seth,

need salting just one more time...(Mac only).

If the user deletes a parent profile folder, Mac does the resolution of that
situation much before other platforms (see bugs 56041 and 57361) and it creates
a directory so that all persistent operations get resolved properly. That
directory is empty to start with and it will filled, if the user chooses to run
the app with that profile. So, those situations take route of
PopulateIfEmptyDir() routine. If we identify a dir not having contents at that
time, we just copy all default contents there. So, we need the fix you did at
http://lxr.mozilla.org/seamonkey/source/profile/src/nsProfile.cpp#886 through
#889 at http://lxr.mozilla.org/seamonkey/source/profile/src/nsProfile.cpp#2294.

Sorry to bring up this late. I was doing more testing and came across this one.

Comment 148

[racham]

*** Bug 58231 has been marked as a duplicate of this bug. ***

Comment 149

[racham]

Conrad,

The last case I mentinoed is of a special interest to the branch.

With what you are fixing for bugs 57011 and 57014 only on trunk, you may not
even have to worry about the case I mentioned. Because you will not be doing any
things we did on 56041 and 57361 on the trunk as result of fixing 57011, 14.

So, we need to get this change onto trunk now and so that we move it onto the
branch laterand make sure that branch has all the fixes it needs to have.

Comment 150

[Conrad Carlen (not reading bugmail)]

Bhuvan, you're right. I was just saying that, on the trunk anyway, I was doing
possibly overlapping work with somebody. But yeah, the fix you mentioned should
go in in any case.

Comment 151

[(not reading, please use [e-mail] instead)]

thanks for catching this, racham.

I'll update the patch.  can you re-review it?

Comment 152

[(not reading, please use [e-mail] instead)]

Attachment: latest and greatest version of the fix.

Comment 153

[(not reading, please use [e-mail] instead)]

when I get a review, I'll land the new part of the patch to the trunk.

(the old part already lives on the trunk.)

Comment 154

[racham]

Seth,

What I saw missing in that patch was a call to SetProfileDir adding indirection
in the new place (inside PopulateIfEmptyDir()). Because we are changing the
directory and we won't possibly hitting the SetProfileDir() call you made on
#893 (nsProifle.cpp), we need to makesure we convey this change to the registry
via updating internal data structure. So, that calls for bit more small changes.
I am attaching a small patch here (wrt the files on trunk). Please take a look,
if you think everything is OK, please update that with your megafix. Otherwise,
you can modify that to the correctness.

Comment 155

[racham]

Attachment: patch for Seth's perusal (for the case I brought up on 2000-10-27 12:31)

Comment 156

[(not reading, please use [e-mail] instead)]

r=sspitzer on racham's fix.

but that fix only applies to the trunk, which already has the other part of the
fix for 56002 on it.

this patch can't go on the branch, since it applies to new code that is trunk only.

racham, we should check it in to the trunk, but it doesn't affect this bug at all.

Comment 157

[Alec Flett]

sr=alecf on the latest patch as well

Comment 158

[(not reading, please use [e-mail] instead)]

I've landed rachams fix to the trunk.

for the branch, http://bugzilla.mozilla.org/showattachment.cgi?attach_id=17998

is still the patch we hope to land.

Comment 159

[racham]

Seth, 

There is some confusion here. The new code (patches for bugs 56041 and 57361) is 
going onto the branch also (and infact it is done in the interest of branch). It 
is checked in onto the trunk just for the sake of getting baked properly. 
Infact, that code is not meant for the trunk (once we check in patches for 56041 
and 57361 on branch, we will have those removed from the trunk and later Conrad 
will check in the correct longterm fixes for trunk). We thought it is better 
idea to have baked for a while on the trunk before we land it on the branch. So, 
please makesure that the latest patch (#18167) goes onto the branch when the 
time comes.

Comment 160

[(not reading, please use [e-mail] instead)]

ok, when those other fixes land on the branch, I'll land your additional fix on
the branch.  (assuming this bug gets rtm++).

Comment 161

[John Unruh]

QA >junruh

Comment 162

[racham]

Seth,

I spoke to PDT members about this bug. We can still get this one in if the
following cases are completed with trunk builds :

1. John Unruh covering all template testing for ProfileManager that Grace
usually does
2. Between 2 of us, we need to look at all ProfileManager regressions that have
popped up since this fix went on the trunk and check the relavance and fix them
as needed.

If we can update the bug tomorrow (by noon) for the cases mentioned above and if
we are good, we have a chance of this bug getting considered for status rtm++.

Comment 163

[Phil Peterson]

Bhuvan, not *just* the regressions since this went on the trunk (although you
should certainly look at those) but also retest all other recently fixed Profile
Manager bugs which could be regressed by this change.

Comment 164

[racham]

1. John Unruh is covering the template testing listed at
http://blues/users/gbush/publish/50_FINAL_TP/50_install_page.html

2. We will be looking at following list
   a. ProfileManager bugs fixed in the recent past (in last 30days) and check
for possible regressions associated with this fix
http://bugzilla.mozilla.org/buglist.cgi?bug_status=RESOLVED&bug_status=VERIFIED&resolution=FIXED&email1=&emailtype1=substring&emailassigned_to1=1&email2=&emailtype2=substring&emailreporter2=1&bugidtype=include&bug_id=&changedin=30&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&component=Profile+Manager+BackEnd&component=Profile+Manager+FrontEnd&short_desc=&short_desc_type=substring&long_desc=&long_desc_type=substring&bug_file_loc=&bug_file_loc_type=substring&status_whiteboard=&status_whiteboard_type=substring&keywords=&keywords_type=anywords&field0-0-0=noop&type0-0-0=noop&value0-0-0=&cmdtype=doit&newqueryname=&order=Reuse+same+sort+as+last+time


   b. Open ProfileManager bugs that have been filed/changed lately
http://bugzilla.mozilla.org/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&email1=&emailtype1=substring&emailassigned_to1=1&email2=&emailtype2=substring&emailreporter2=1&bugidtype=include&bug_id=&changedin=30&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&component=Profile+Manager+BackEnd&component=Profile+Manager+FrontEnd&short_desc=&short_desc_type=substring&long_desc=&long_desc_type=substring&bug_file_loc=&bug_file_loc_type=substring&status_whiteboard=&status_whiteboard_type=substring&keywords=&keywords_type=anywords&field0-0-0=noop&type0-0-0=noop&value0-0-0=&cmdtype=doit&newqueryname=&order=Reuse+same+sort+as+last+time

Let me know If I miseed any other criteria.

Comment 165

[John Unruh]

Problem with the Mac - The directory created under default is always the same - 
rr8gbcix.slt. Reproduced twice in a row on a G4, and once on a G3.
Linux is OK. It creates different directory names each time I start Netscape 6 
without a .mozilla and .netscape folder.
Windows is OK. It creates different directory names each time I start Netscape 6 
without a mozilla folder and mozregistry.dat and nsreg.dat.

Comment 166

[racham]

Noticed what John has mentioned...the name of the random salted folder is always 
rr8gbcix.slt on Mac. Other 2 platforms are OK (we get random names in each 
case). Need to check randomization process once again wrt to mac.

John, what about all other profilemanager tests..? Are we OK there..?

I have verified case 2a from my previous update. All the listed bugs are working 
fine on all platforms.

Checking the list in 2b right now.

Comment 167

[(not reading, please use [e-mail] instead)]

I'll do some debugging on my mac.

my guess is I'm calling srand() with the same value.

that would explain why rand() always returns the same values.

Comment 168

[(not reading, please use [e-mail] instead)]

I've fixed the rand problem.
racham and I sat down and debugged the problem.

on mac, the granularity of PR_Now() is not as good as win32 and linux, so
dividing by 1,000 and then casting always gave the same number.

diving by 1,000,000 (to get seconds, not milliseconds) works for all three
platforms.

we also fixed another problem where you set up a profile, quit, remove the
profile dir on disk, start back up and make changes (to prefs and bookmarks).
, quit and restart you loose the changes .

the fix was to make sure we update the registry after adding the level of
indirection and call SetProfileDir().

racham will post a new fix, and add some comments.

Comment 169

[racham]

Seth and I just looked at list 2b (recently filed/regressed profile manager bugs).
Only problem that we found and fixed already is to update ther registry to
contain recreated salted directories in the case where user has deleted the
profile folder or it's parent folder. Will be posting the patch along with
Seth's fix for random number generataion process that fixes the salt dir name on
mac. Will post the patch which contains these changes.

Comment 170

[racham]

Attachment: Fix random number generation process and update registry after salting in special cases

Comment 171

[Stephen P. Morse]

I don't understand how dividing by 1,000,000 to get seconds gives a unique 
number on the mac, but dividing by 1,000 to get milliseconds gives the same 
value every time.  How could that be possible?

I need to understand this because I do the division by 1,000 in the wallet 
module when I generate random names for the wallet and password database files.

Comment 172

[racham]

Attachment: After Seth's cleanup of last patch

Comment 173

[(not reading, please use [e-mail] instead)]

morse:

the number was good, it was the cast to (uint) that was giving us the same number.

it kept turning it into 4294967295, which is probably the max for a uint.

Comment 174

[selmer (gone)]

rtm++, please checkin ASAP so we can build today.

Comment 175

[(not reading, please use [e-mail] instead)]

fixed on branch and trunk.

fyi to qa, this will only affect newly created or migrated profiles.  existing
profiles will not have the added level of indirection.

Comment 176

[John Unruh]

Verified on Win, Mac and Linux 11/1 evening branch builds.

Comment 177

[John Unruh]

Verified in the 11/14 Win, Mac and Linux trunk builds.

Comment 178

[Markus Gerstel]

I'm sorry. In my opinion this won't get you any further. You have spend much
time and manpower to develop unneccessary code and an unwanted feature. Don't
you recognize that hiding the directory in any way is
A) Security by obscurity
B) Annoying to the user

Please consider this :
A) Every local program and maybe some remote exploits via websites can get a
directory listing
Fix A) by creating 3 different decoy-directories with other salts.
B) Mozilla has a file from which it knows where to find the profile. This file
is always in the same place
Fix B) by faking this file
C) Anyway the user starts the only mozilla executable on the computer
Fix C) by decoy mozilla-installations....

If you have a malicious javascript which can read from harddisk you can gather
the path from the mozilla-db then get into the profiledirectory then read
everything you want.
If you have a malicious javascript which can write onto the harddisk then simply
upload a 0-byte explorer.exe into C:\ and no windows-computer will reboot
correctly. Or upload a new mozilla.exe. The normal user who you want to protect
will install mozilla in it's default path. (NO!!!! Don't change the default path
to C:\Programs\JASOPiuf0gs89uedroiNRSjjcjy.asdjiaosj\maybe\here\maybe\not\)

Of course you can continue this "security"-paranoia : When at some point even
the user has no chance to detect where his profile has been stored, and he has
no way to remove the data except by replacing the hard disk (obviously we can
still try other media.. nvram in BIOS or RTC) - then you have won.

If you still don't want to accept this -what I think the reality is- then you
should reconsider Bug #70931 ([RFE] Override salted Profile directory).
If you can accept this - then throw your valuable newly developed code away.

Because in my opinion what you are doing is what you certainly don't intended to :
You are patronizing the user by the same way the Microsoft Internet Explorer
does : Hiding Files anywhere in the system, unneccessary problems by backing up
and restoring profile data, undetectable and maybe at some point even unremoveable.

Greetings -Markus

Comment 179

[Matthew Kruer]

This bug was brought to my attention by [e-mail], from
http://bugzilla.mozilla.org/show_bug.cgi?id=95331 that I was having problems
backing up profiles but was loosing information when restoring them. The problem
lies in the mail client, loosing all the information because of the little
random directory trick.

Reading through everything. I have found it quite easy to potential crack.  You
don?t have to have this random billion links to find the directory. It?s even
easier. The mozilla application directory, Usually C:\WINDOWS\Application
Data\Mozilla ther is a file called registery.dat get this file and that?s all
you need to find all the information for the profiles. I just opened it as a
text file and voila I got the randdome directory for the profile if you care to
know its ?directory_C:\WINDOWS\Application
Data\Mozilla\Profiles\MKruer\la6zurea.slt.? It?s not even encrypted so this
entire security issue is in my opinion bogus because it doesn?t have to be
cracked. The information is there clear as day.

A better solution if you want to protect the directory it to place a high level
encryption on the files and then store the key in the registry or some file that
is randomly generated, and placed in some other directory. The idea is to keep
the path the same so for proposes of back-up so you don?t have to re-associate
folders because they are in different directories. But when you do restore it
will ask for verification that you have permission to ?uses? the profile. You
then enter the password, it would see if it works and then store it in a safe
place so the next time you use the profile it has everything you need to access
the information.
The password for a profile would be created when the profile was created. 

Hope that makes sence.

Comment 180

[timeless]

this bug is only 35 printed pages long. It was verified fixed. If you have 
_any_ issues with it, file a new bug.

Comment 181

[timeless]

35 pages long!

Comment 182

[Markus Gerstel]

I'm sorry but 35 pages are not enough.

Comment 183

[Markus Gerstel]

*** This bug has been marked as a duplicate of 96601 ***

Comment 184

[John Unruh]

Verified dupe.

Comment 185

[Gervase Markham [:gerv]]

Reopening; this bug is not a dupe of 96601, it's the complete opposite of it.

Gerv

Comment 186

[Gervase Markham [:gerv]]

Re-resolving as FIXED.

Gerv

Comment 187

[John Unruh]

V