89097 - network.security.ports.banned.override read from all.js but not from profile (prefs.js or user.js)

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Jacek Piskozub]

The new preference to override default port blocking
network.security.ports.banned.override (bug 85601) works when inserted into the
default all.js file - for example pref("network.security.ports.banned.override",
"1080");. This setting lets me seeing any http://foo:1080 URL

However setting this pref in the profile (either prefs.js or user.js) as
user_pref("network.security.ports.banned.override", "1080"); does not have any
effect. That is a warning about banned port pops up.

The code to read this pref is in /mozilla/netwerk/base/src/nsIOService.cpp. I
cannot guess why it works only with all.js. Do prefs from the profile files need
registering?

Comment 1

[Jacek Piskozub]

Setting severity to major as prefs in all.js are subject to overwriting with
every  Mozilla installation and adding mozilla0.9.3 as it seems very easy to fix
(if someone understands the prefs code).

Comment 2

[sairuh (rarely reading bugmail)]

->security?

Comment 3

[Jacek Piskozub]

Why security? What is the reason of a setting to override the default port
blocking if an user cannot use them to see a page that has a "banned" port number? 

Actually there should be an UI setting for this but it is the focus of bug
85601. I believe therefore that this bug is blocking 85601 - marking
accordingly. If someone disagrees, plese clear the block.

Comment 4

[Mitchell Stoltz (not reading bugmail)]

Over to dougt.

Comment 5

[Doug Turner (:dougt)]

this is kind of pref problem... or you are not using the correct syntax in your
user.js file.

Try using "user_pref" instead of "pref"

Comment 6

[Jacek Piskozub]

Dough, please read the bug :-( 

One uses user_pref(); in prefs.js or user.js and pref(); in
/defaults/pref/all.js. Please notice the correct use of both setting in my
original comment. I used both exactly to avoid suggestions like that...

In meantme I asked off-page Darin Fisher who saw similar behavior in another
bug. He suggests that some necko components do not register themselves as
profile/pref change observers and therefore have no way of detecting differences
between prefs.js and all.js if the component were loaded before a profile is
selected.

Comment 7

[Doug Turner (:dougt)]

Samir, why does this work only in all.js?  What am I doing wrong?

Comment 8

[Bradley Baetz (:bbaetz)]

dougt - based on the comments from darin, I suspect you need to register
yourself as a pref observer, with a callback. See the code in nsHttpHandler::Init()

Comment 9

[Doug Turner (:dougt)]

ugh.  you are right... the profilemanager reads these in AFTER the ioserver has
started.... crap.

This probably wont be fixed until after 0.9.3 unless someone submits a patch.

Comment 10

[Bradley Baetz (:bbaetz)]

relnote added to commercial release notes, and I'll add one to the 0.9.4 relnote
bug when one is created.

Comment 11

[Doug Turner (:dougt)]

people are annoyed that they can't do this.  moving to 9.6

Comment 12

[Chase Tingley]

Attachment: patch

Comment 13

[Chase Tingley]

Patch registers nsIOService with prefs.  Following Darin's lead in bug  102332,
it also listens for xpcom-shutdown so that the prefs observers can be removed --
otherwise everything leaks.

Comment 14

[Darin Fisher]

Comment on attachment 52120 [details] [diff] [review]
patch

>Index: nsIOService.cpp

>@@ -205,50 +208,23 @@

>+    // Further modifications to the port list come from prefs
>+    nsCOMPtr<nsIPrefBranch> prefBranch;
>+    GetPrefBranch(getter_AddRefs(prefBranch));
>+    if (prefBranch) {
>+        nsCOMPtr<nsIPrefBranchInternal> pbi = do_QueryInterface(prefBranch);
>+        if (pbi)
>+            pbi->AddObserver(NETWORK_PORT_PREFS, this);
>+        PrefsChanged(prefBranch);
>         }
> 

looks like you have an indentation problem here.




>+void
>+nsIOService::PrefsChanged(nsIPrefBranch *prefs, const char *pref)
>+{
>+    if (!prefs) return;
>+
>+    nsresult rv = NS_OK;
>+    nsXPIDLCString portList;
>+
>+    // Look for extra ports to block
>+    if ((pref == nsnull) || PL_strcmp(pref, "network.security.ports.banned") == 0) {
>+        rv = prefs->GetCharPref("network.security.ports.banned", 
>+                                getter_Copies(portList));
>+        if (portList) {
>+            char* tokp;
>+            char* currentPos = (char *)portList.get();  
>+            while ( (tokp = nsCRT::strtok(currentPos, ",", &currentPos)) != nsnull )
>+            {
>+                nsCAutoString tmp(tokp);
>+                tmp.StripWhitespace();
>+
>+                PRInt32 aErrorCode;
>+                PRInt32 value = tmp.ToInteger(&aErrorCode);
>+                mRestrictedPortList.AppendElement((void*)value);
>+            }
>+
>+        }
>+    }
>+    // ...as well as previous blocks to remove.
>+    if ((pref == nsnull) || PL_strcmp(pref, "network.security.ports.banned.override") == 0) {
>+        rv = prefs->GetCharPref("network.security.ports.banned.override", 
>+                                getter_Copies(portList));
>+        if (portList) {
>+            char* tokp;
>+            char* currentPos = (char *)portList.get();  
>+            while ( (tokp = nsCRT::strtok(currentPos, ",", &currentPos)) != nsnull )
>+            {
>+                nsCAutoString tmp(tokp);
>+                tmp.StripWhitespace();
>+
>+                PRInt32 aErrorCode;
>+                PRInt32 value = tmp.ToInteger(&aErrorCode);
>+                mRestrictedPortList.RemoveElement((void*)value);
>+            }
>+
>+        }
>+    }
>+}

this routine needs to be cleaned up...  do not duplicate code like this.  either use
a loop or a helper function.




>+// nsIObserver interface
>+NS_IMETHODIMP
>+nsIOService::Observe(nsISupports *subject,
>+                     const PRUnichar *topic,
>+                     const PRUnichar *data)
>+{
>+    if (!nsCRT::strcmp(topic, NS_LITERAL_STRING("nsPref:changed").get())) {
>+        nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(subject);
>+        if (prefBranch)
>+            PrefsChanged(prefBranch, NS_ConvertUCS2toUTF8(data).get());
>+    }
>+    else if (!nsCRT::strcmp(topic, NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get())) {
>+        // Clean up the observers
>+        nsCOMPtr<nsIPrefBranch> prefBranch;
>+        GetPrefBranch(getter_AddRefs(prefBranch));
>+        if (prefBranch) {
>+            nsCOMPtr<nsIPrefBranchInternal> pbi = do_QueryInterface(prefBranch);
>+            if (pbi)
>+                pbi->RemoveObserver(NETWORK_PORT_PREFS, this);
>+        }
>+
>+        nsCOMPtr<nsIObserverService> observerService = 
>+            do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
>+        if (observerService)
>+            observerService->RemoveObserver(this, NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get());

I recall there being a bug that you can't remove a nsIObserver from the observer service
during its Observe method.  HTTP implements nsISupportsWeakReference, and the observer
service is implemented such that it'll actually hold only a weak reference to the observer
if it supports weak referencing.  This is why HTTP does not remove itself from the observer
service at xpcom-shutdown time.



>+    }
>+    return NS_OK;
>+}
>Index: nsIOService.h
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/base/src/nsIOService.h,v
>retrieving revision 1.35
>diff -u -r1.35 nsIOService.h
>--- nsIOService.h	2001/09/28 20:08:59	1.35
>+++ nsIOService.h	2001/10/04 22:47:15
>@@ -51,12 +51,16 @@
> #include "nsIEventQueueService.h"
> #include "nsIURLParser.h"
> #include "nsSupportsArray.h"
>+#include "nsIObserver.h"
> 
> #define NS_N(x) (sizeof(x)/sizeof(*x))
> 
> static const char *gScheme[] = {"chrome", "resource", "file", "http"};
> 
>+class nsIPrefBranch;
>+
> class nsIOService : public nsIIOService
>+                  , public nsIObserver
> {
> public:
>     NS_DECL_ISUPPORTS
>@@ -64,6 +68,9 @@
>     // nsIIOService methods:
>     NS_DECL_NSIIOSERVICE
> 
>+    // nsIObserver methods:
>+    NS_DECL_NSIOBSERVER
>+
>     // nsIOService methods:
>     nsIOService();
>     virtual ~nsIOService();
>@@ -88,6 +95,10 @@
> 
>     NS_METHOD CacheURLParser(const char *scheme,
>                              nsIURLParser* hdlr);
>+
>+    // Prefs
>+    void PrefsChanged(nsIPrefBranch *prefs, const char *pref = nsnull);
>+    void GetPrefBranch(nsIPrefBranch **);
> 
> protected:
>     PRBool      mOffline;

Comment 15

[Chase Tingley]

> looks like you have an indentation problem here.

oops.

> this routine needs to be cleaned up... 

This is the original code, so I wasn't going to muck with it much.  However,
this would be a good opportunity to tighten it up.

> I recall there being a bug that you can't remove a nsIObserver from the 
> observer service during its Observe method.

It looks like bug 94349 and bug 68200 mention this.  I'll add weak reference
support.

Comment 16

[Chase Tingley]

Attachment: v2

Comment 17

[Darin Fisher]

Comment on attachment 52211 [details] [diff] [review]
v2

sr=darin (good job cleaning this up!)

Comment 18

[Chase Tingley]

Thanks for the quick reviews.

When the trunk unfreezes, I'll need someone to check this in for me.

Comment 19

[Doug Turner (:dougt)]

Checking in nsIOService.cpp;
/cvsroot/mozilla/netwerk/base/src/nsIOService.cpp,v  <--  nsIOService.cpp
new revision: 1.115; previous revision: 1.114
done
Checking in nsIOService.h;
/cvsroot/mozilla/netwerk/base/src/nsIOService.h,v  <--  nsIOService.h
new revision: 1.36; previous revision: 1.35
done

Thanks alot for fixing this problem!

Comment 20

[bsharma]

Marking verified as per above developer comments.