87661 - Security issues with AutoConfig feature/ Moving the creation of AutoConfig

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect)

Description

[Mitesh Shah]

Opening up a new bug to track the security issues with current design of
AutoConfig module. As per the talk with mstoltz, there is a little risk with the
current design of autoconfig.

Right now, users can set this preference through  prefs.js without netscape.cfg.
It is desirable if we somehow not allow users to set this preference through
prefs.js. Also, even if it is set through netscape.cfg, browser saves it to
prefs.js so it becomes visible to users. 

By disabling users to play with this preference and allowing only Enterprise IS
to set it through netscape.cfg, we can save some possible security exploits.

Also, regarding LDAP access through AutoConfig,
Making prefs context XPCONNECT enabled is a HUGE risk so using a JS sandbox or
JS->C->XPCOM interface (essentially a sandbox)  for LDAP makes lot of sense.

Comment 1

[Mitesh Shah]

I think it would help if  we instantiate AutoConfig only after netscape.cfg fins
the "autoadmin.global_config_url" pref. I will post the patch later on to move
the creation of nsAutoConfig to nsPrefService.cpp.

Comment 2

[Mitesh Shah]

Attachment: Removing registration of nsAutoConfig from PrefsFactory, adding instatiation in nsPrefService

Comment 3

[Mitchell Stoltz (not reading bugmail)]

This is a good idea. It will limit AutoConfig to those who really know what
they're doing (enterprise IT folks). Let's restrict the enabling of AutoConfig
to netscape.cfg.

Comment 4

[Mitchell Stoltz (not reading bugmail)]

bnesse and I talked about making it so that only netscape.cfg can turn on
autoconfig - the user can't turn it on by adding a pref to all,js, etc. Is this
what your patch does?

Comment 5

[Mitesh Shah]

Attachment: Previous patch had bad line number information, updated patch

Comment 6

[Brian Nesse (gone)]

This patch will not, and can not, guarantee that autoconfig is enabled in 
netscape.cfg. What it will guarantee is that the autoconfig stuff can't be 
enabled in prefs.js or any other user supplied file.

There is no way to tell what file a preference comes from, so in theory, you 
could enable autoconfig in all.js. In practice however, making this work would be 
difficult.

The autoconfig pref will only be checked after sucessfully reading a .cfg file. 
So even if you enabled the preference in all.js, you would still need to have a 
valid .cfg file (i.e. one that is hashed correctly and doesn't throw an error 
while being processed) in order for the autoconfig code to be executed.

Comment 7

[Brian Nesse (gone)]

You have declared two new functions Get/SetCfgURL() neither of these is declared
in either nsAutoConfig.h or nsIAutoConfig.idl. I'm not even sure why it compiles...

I don't really see the need for GetCfgURl(). As the autoConfig object is created
and left as a standalone observer, I don't think there is any consumer around to
ever call the Get method.

In ReadConfigFile() I would suggest not doing the autoConfig check until after
the code which validates the .cfg file name (and probably the vendor name too).
I understand this will add some complexity but the additional security, however
minor, probably justifies it.

Comment 8

[Mitesh Shah]

Actually, I have added the attribute cfgURL in nsIAutoConfig.idl and that will 
devlare getter and setter for cfgURL. I didn't find a way to do 'writeonly
attribute' so had to implement both getter and setter. if you think we only need
setter, I can switch the attribute declaration to a function call.

Regarding, the security check after autoconfig. If the security check fails,
anyway we are existing from the browser so I thought it shouldn't be a problem
if we instantiate autoconfig before that check

Comment 9

[Brian Nesse (gone)]

Ok, your reasoning is sound in both cases. r=bnesse.

Comment 10

[Mitesh Shah]

Adding Seth and Shaver for superreview. 

Comment 11

[Brian Nesse (gone)]

After thinking about it some more, I think I'd like you to move the autoConfig
check after all. As we only pass errors back up to the caller, there is no
guarantee that we will be quitting on error. Even if we are quitting now, there
is no guarantee that this will not change in the future. With the impending
coming of runtime profile switching, I'd just rather not risk it.

Comment 12

[Mitesh Shah]

This is not a topembed bug anymore. Bug 94349 was blocking a topembed and it got 
fixed. This is an additional improvement to that fix.

Comment 13

[Mitesh Shah]

Attachment: Moved AutoConfig after the added security check, improved indentation, wrapped longer lines

Comment 14

[Brian Nesse (gone)]

Very nice. As a recommendation, you might add to the commemnt when you
instantiate the autoconfig object that it will live, referenced only by the
obeserver list, until it's expiration. This will keep future coders from
wondering why it's being created and nothing is ever "done" with it. r=bnesse.

Comment 15

[Judson Valeski]

shouldn't the cfgURL be a "wstring"?

Comment 16

[Mitesh Shah]

This is the value of autoconfig URL being set as a preference for Enterprise
customers which is a 'char *' as far as I know.  

Comment 17

[Mike Shaver (:shaver emeritus)]

 interface nsIAutoConfig : nsISupports {
   void downloadAutoCfg();
+  attribute string cfgURL;

"Config" in the interface name should be mirrored in the members: please use
"downloadAutoConfig" and "configURL" as names instead (and fix up the
hopefully-few callers of "downloadAutoCfg").

+// attribute string cfgURL
+NS_IMETHODIMP nsAutoConfig::GetCfgURL(char * *aCfgURL)
+{
+    if (!aCfgURL) 
+        return NS_ERROR_NULL_POINTER;
+    if (!mCfgURL.IsEmpty())
+        *aCfgURL = mCfgURL.ToNewCString();
+    return NS_OK;

That seems wrong to me: getters should always leave their out param in a known
state if they're going to return a success code.  So in the empty case, you
probably want to either dup the empty string or store nsnull in *aCfgURL and
return success.  And then you need to check for ToNewCString failure, so that
you can return an unambiguous NS_ERROR_OUT_OF_MEMORY.
(Are we sure we want to use an empty string as the no-config-URL signal?  I
think using nsnull is the prevailing mechanism.)

 if (NS_SUCCEEDED(rv))
-  {
-
+    {
+      

and

+  if(NS_SUCCEEDED(rv)) {
+

are inconsistent with the usual

+  if (NS_SUCCEEDED(rv) && (nsCRT::strlen(urlName) > 0)) {  
+    

style (space before opening paren, cuddled brace).  Please follow the dominant
style in the files you're editing.

Of course,

+  if (NS_SUCCEEDED(rv) && (nsCRT::strlen(urlName) > 0)) {  

is wasteful: there's no need to compute the length of the string to find out
that it's non-empty: just check the first character against '\0', please.

Above that,

+    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.

Comment 18

[Mitesh Shah]

- Will change the names to downloadAutoConfig() and ConfigURL ( I think after we
don't even need downloadAutoConfig() in the interface)

- will change the getter design to return nsnull in case of empty string. 

- 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.

Comment 19

[Mitesh Shah]

Attachment: updated patch

Comment 20

[Mike Shaver (:shaver emeritus)]

If downloadAutoConfig isn't needed in the interface, let's get it out.  Chip,
can you explain the vendor/lock-file stuff to us?

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

Comment 21

[Chip Clark]

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.

Comment 22

[Mike Shaver (:shaver emeritus)]

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!)

Comment 23

[Chip Clark]

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 24

[Asa Dotzler [:asa]]

a=asa on behalf of drivers

Comment 25

[Mitesh Shah]

Fix checked in

Comment 26

[Mitesh Shah]

Created a new bug 96897 to discuss about the netscape.cfg code. Marking this fixed.

Comment 27

[lrg]

I have discovered a regression caused with this bug, so I am reopening it.

http://bugzilla.mozilla.org/show_bug.cgi?id=97228

Comment 28

[lrg]

This bug is now a duplicate of bug 97182

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

Comment 29

[lrg]

Closed the wrong bug

Comment 30

[Mitesh Shah]

Regression bug 97228 has been fixed.

Comment 31

[lrg]

The bug that was blocking this one has been verified fixed.  This fix now works 
without causing any further problems, closing as verified fixed