96897 - AutoConfig: Extra security check in ReadConfig() needs a review

Status: RESOLVED INCOMPLETE

(Core :: Preferences: Backend, defect)

Description

[Mitesh Shah]

Opening up a new bug according to following discussion on bug 87661

------- Additional Comments From Mike Shaver 2001-08-21 11:12 -------


+    vendorLen = PL_strlen(lockVendor);
+
+    //  lockVendor and lockFileName should be the same with the addtion of 
+    // .cfg to the filename by checking this post reading of the cfg file 
+    // this value can be set within the cfg file adding a level of security.
+    
+    if (PL_strncmp(lockFileName, lockVendor, fileNameLen -4) != 0)
+      return NS_ERROR_FAILURE;
+  }

computes vendorLen and then doesn't use it.  And the check you're doing there
doesn't match the comment: it would have a lockFileName of "shaver.txt" and a
lockVendor of "shaver" succeed, which is not what you say you want.  If you need
a .cfg suffix, please check for one, or amend the comment.

Can you explain how requiring the config file name to match the vendor name adds
security, versus simply computing the config file name from the specified
vendor?  It seems like it's a recipe for administrative confusion, where two
prefs need to be changed in lockstep.



------- Additional Comments From Mitesh Shah 2001-08-21 13:05 -------

- Regarding lock file name and vendor check, it was already existing code. I
just changed the indentation. CCing chipc so that he can comment on it.



------- Additional Comments From Mike Shaver 2001-08-22 14:34 -------

If I can be made to understand (and approve of) that, I think this baby's ready
for checkin.



------- Additional Comments From Chip Clark 2001-08-22 14:51 -------

the concept is (or was) to have a vendor "name" for a config file.  Long ago
(before I was involved with the project) the pref was created so enterprise
clients could make their own cfg file... and using MD5Class hashing "sign" the
file with a vendor name.

Then... there would be a check for the vendor name.  If it matched (ie, the cfg
was being used by the correct people) the cfg would succeed and all would be ok.

However, MD5Class stuff was removed from the cfg as it was embedded into
Security.  It was felt to load all the security stuff at startup JUST to check
the cfg file wasn't necessary (since the file was really only a simple bit
exchange "hashing").

SO.... I removed the MD5Class stuff but implemented a simple "check" on the
vendor name.

The cfg file name is set in the all.js file (or all-ns.js file for N6).   The
file is read (and the prefs loaded).   Then we do a check to see if the vendor
name and the cfg file name match.   IF not, we exit without starting Netscape.

It's not very sophisticated... but if someone wanted to try and use the file
outside of it's intended enviroment they might miss the "vendor" pref and as a
result not be able to start Netscape.   

If you really wanted to get fancy you could add a pref (completely unrelated) to
the general.config.... and check it inside the cfg file... and set (or change)
the general.config.vendor pref as a result making it even more difficult to use
the cfg file outside the intended environment.


Long explanation for a simple (if somewhat stupid) implementation.



------- Additional Comments From Mike Shaver 2001-08-23 10:30 -------

So why do we want to prevent someone from using the .cfg file outside the
intended environment?  Can't people just set their prefs to match that anyway? 
What are we protecting against?

(sr=shaver on the patch, mitesh, but I suspect that we're going to want to open
another bug to clean up the vendor/lockfile stuff.  Let's keep this bug open
until we know one way or the other.  And thanks for being responsive to review!)



------- Additional Comments From Chip Clark 2001-08-23 14:19 -------

I'm not sure I understand the reasoning... but it's a corporate thing.   
Mitch Green might be able to answer it more clearly.  

I don't think the idea is to prevent it - but make it difficult (which is what
the original security implementation was for).

Comment 1

[Chip Clark]

I'll accept the bug and I'll remove the check.  (Yes, I was the one that put it
in  in the first place)... but that was to carry over the pref 
'general.config.vendor' which was put in place by the security folks.  

So... I'll make the changes and get approval for the reduced security by someone
in the security team.

Comment 2

[Rodney Velasco]

QA over to me...this is somewhat of an eclient specific issue.

Comment 3

[Chip Clark]

Attachment: Diff bug 92447 (nsPrefService.cpp) and "corrected" version

Comment 4

[selmer (gone)]

Brian, you seem to be the logical new owner for this bug.  Please reassign if I
got it wrong.  Thx.

Comment 5

[Benjamin Smedberg]

Code still exists, but changing it is probably riskier than not at this point.