Security issues with AutoConfig feature/ Moving the creation of AutoConfig

VERIFIED FIXED in mozilla0.9.5

Status

()

Core
Preferences: Backend
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: Mitesh Shah, Assigned: Mitesh Shah)

Tracking

Trunk
mozilla0.9.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Requesting sr)

Attachments

(4 attachments)

(Assignee)

Description

17 years ago
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.
QA Contact: sairuh → rvelasco

Updated

17 years ago
QA Contact: rvelasco → lrg
(Assignee)

Comment 1

17 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Updated

17 years ago
Summary: Security issues with AutoConfig->LDAP feature → Security issues with AutoConfig feature/ Moving the creation of AutoConfig
(Assignee)

Comment 2

17 years ago
Created attachment 45409 [details] [diff] [review]
Removing registration of nsAutoConfig from PrefsFactory, adding instatiation in nsPrefService
(Assignee)

Updated

17 years ago
Keywords: nsenterprise, topembed
Target Milestone: --- → mozilla0.9.4
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.
(Assignee)

Updated

17 years ago
Whiteboard: Requesting r and sr
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?
(Assignee)

Comment 5

17 years ago
Created attachment 45939 [details] [diff] [review]
Previous patch had bad line number information, updated patch

Comment 6

17 years ago
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

17 years ago
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.
(Assignee)

Comment 8

17 years ago
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

17 years ago
Ok, your reasoning is sound in both cases. r=bnesse.
(Assignee)

Comment 10

17 years ago
Adding Seth and Shaver for superreview. 

Comment 11

17 years ago
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.
(Assignee)

Comment 12

17 years ago
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.
Keywords: topembed
(Assignee)

Comment 13

17 years ago
Created attachment 46079 [details] [diff] [review]
Moved AutoConfig after the added security check, improved indentation, wrapped longer lines

Comment 14

17 years ago
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.
(Assignee)

Updated

17 years ago
Whiteboard: Requesting r and sr → Requesting sr

Comment 15

17 years ago
shouldn't the cfgURL be a "wstring"?
(Assignee)

Comment 16

17 years ago
This is the value of autoconfig URL being set as a preference for Enterprise
customers which is a 'char *' as far as I know.  
 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.
(Assignee)

Comment 18

17 years ago
- 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.
(Assignee)

Comment 19

17 years ago
Created attachment 46662 [details] [diff] [review]
updated patch
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

17 years ago
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.
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

17 years ago
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

17 years ago
a=asa on behalf of drivers
(Assignee)

Comment 25

17 years ago
Fix checked in
(Assignee)

Comment 26

17 years ago
Created a new bug 96897 to discuss about the netscape.cfg code. Marking this fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

17 years ago
Keywords: nsenterprise → nsenterprise+

Comment 27

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

http://bugzilla.mozilla.org/show_bug.cgi?id=97228
Status: RESOLVED → REOPENED
Depends on: 97228
Resolution: FIXED → ---

Comment 28

17 years ago
This bug is now a duplicate of bug 97182

*** This bug has been marked as a duplicate of 97182 ***
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → DUPLICATE

Comment 29

17 years ago
Closed the wrong bug
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Updated

17 years ago
Keywords: nsbranch
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Assignee)

Comment 30

17 years ago
Regression bug 97228 has been fixed.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 31

17 years ago
The bug that was blocking this one has been verified fixed.  This fix now works 
without causing any further problems, closing as verified fixed
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.