Last Comment Bug 337746 - [FIX]Move "safe about" hardcoding out of security manager
: [FIX]Move "safe about" hardcoding out of security manager
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: x86 Linux
P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Selena Deckelmann :selenamarie :selena use ni?
Depends on: 337748 342108 342369
Blocks: 341313
  Show dependency treegraph
Reported: 2006-05-12 11:51 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2009-09-17 13:48 PDT (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (48.33 KB, patch)
2006-05-12 12:59 PDT, Boris Zbarsky [:bz] (still a bit busy)
darin.moz: superreview+
Details | Diff | Splinter Review
Updated to darin's comments (51.55 KB, patch)
2006-06-13 16:29 PDT, Boris Zbarsky [:bz] (still a bit busy)
dveditz: review+
darin.moz: superreview+
Details | Diff | Splinter Review

Description User image Boris Zbarsky [:bz] (still a bit busy) 2006-05-12 11:51:02 PDT
This info should really live in the protocol handler.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2006-05-12 12:59:18 PDT
Created attachment 221838 [details] [diff] [review]
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2006-05-12 13:04:10 PDT
Summary of changes:

1)  Add an nsIAboutModule methods so that about: URIs can decide for themselves
    whether they're available to untrusted content.
2)  Make any about redirector module that drops privs be available to untrusted
    content.  This makes about:buildconfig available.  I can use a hardcoded
    list here instead or add another field to the table, if desired.
3)  Rename nsViewSourceURI to nsSimpleNestedURI.  Change its Read and Write
    methods a bit to allow it to work without making assumptions about its inner
    URI.  The rest of the code just moved.
4)  Remove the hardcoded list in security manager.
5)  Change "about" from ChromeProtocol to DenyProtocol.

Once I get this in, I'll work on fixing CheckLoadURI to use protocol flags in general; hence the creation of the moz-safe-about handler.
Comment 3 User image Darin Fisher 2006-05-23 14:12:58 PDT
Comment on attachment 221838 [details] [diff] [review]

>Index: netwerk/base/src/nsSimpleNestedURI.h

>+ * URI class that inherits from nsSimpleURI and has some other URI that the URI
>+ * resolves too.  All objects of this class should always be immutable, so that

I'm confused by the wording in the first sentence.  "has some other URI that the
URI resolves too."  <<-- can you clarify that?

>+#include "nsCOMPtr.h"
>+#include "nsSimpleURI.h"
>+#include "nsINestedURI.h"
>+#include "nsNetUtil.h"

I think it would be nice if the header file did not include nsNetUtil.h as it
makes a big impact on compilation time.  Maybe you could make the constructor
for this class be non-inline.  Anyways, the amount of code in the constructor
is non-trivial once NS_TryToSetImmutable is expanded.

>+#define NS_SIMPLENESTEDURI_CID                           \
>+{ /* 56388dad-287b-4240-a785-85c394012503 */             \
>+     0x56388dad,                                         \
>+     0x287b,                                             \
>+     0x4240,                                             \
>+     { 0xa7, 0x85, 0x85, 0xc3, 0x94, 0x01, 0x25, 0x03 }  \

Should this move into nsNetCID.h?  Do you consumers want a way to get at this
exact implementation?  I don't see why we should discourage that.

>+static nsresult
>+nsSimpleNestedURIConstructor(nsISupports*, const nsIID&, void**);

Hmm... it is very unfortunate to need to forward declare this here.

>+class nsSimpleNestedURI : public nsSimpleURI,
>+    friend nsresult nsSimpleNestedURIConstructor(nsISupports*,
>+                                                 const nsIID&, void**);
>+    // To be used by deserialization only
>+    nsSimpleNestedURI()
>+        : nsSimpleURI(nsnull)
>+    {
>+    }

Why does this need to be protected?  It doesn't leave the object in
an inconsistent state.  All this does is make consumers have to use
the component manager to get a nsSimpleNestedURI that does not have
an inner URI component, right?  I think I'd prefer to just make this
constructor public and remove the nsSimpleNestedURIConstructor forward
and friend declarations :)

>Index: netwerk/base/src/nsSimpleNestedURI.cpp


Declare this inside nsSimpleNestedURI::GetClassIDNoAlloc?

>+    rv = aStream->ReadObject(PR_TRUE, getter_AddRefs(mInnerURI));

Hmm... nsISupports** not equal to nsIURI**... do we need to care?

>Index: netwerk/protocol/about/public/nsIAboutModule.idl

>+    /**
>+     * Check whether aURI is safe for untrusted content.  If it is, web pages
>+     * and so forth will be allowed to link to this about: URI.  Otherwise,
>+     * only chrome will be able to link to it.
>+     */
>+    boolean safeForUntrustedContent(in nsIURI aURI);

I wonder if it would make sense to generalize this a bit more.  Maybe
have the function take a flags parameter that allows the caller to
test a variety of different features?  Or, have this function return
a flags value that indicates features for the given URI?  Maybe such
generalization is overkill?  I like adding in flexibility for down
the road :)

>Index: netwerk/protocol/about/src/nsAboutProtocolHandler.h

>+{ /* 1423e739-782c-4081-b5d8-fe6fba68c0ef */         \
>+    0x1423e739,                                      \
>+    0x782c,                                          \
>+    0x4081,                                          \
>+    {0xb5, 0xd8, 0xfe, 0x6f, 0xba, 0x68, 0xc0, 0xef} \

Again, I'm a fan of putting this in nsNetCID.h

>+class nsSafeAboutProtocolHandler : public nsIProtocolHandler
>+    virtual ~nsSafeAboutProtocolHandler();
> };

Does this dtor need to be public?  Can it be private and non-virtual?

>Index: netwerk/protocol/about/src/nsAboutProtocolHandler.cpp

>+        rv = NS_NewURI(getter_AddRefs(inner), "moz-safe-about:x");

Can this just be "moz-safe-about:"


Impl these in the header so they can be inlined away.

>Index: netwerk/protocol/about/src/nsAboutRedirector.cpp

>+    for (int i=0; i<kRedirTotal; i++) 

nit: add whitespace around operators

>Index: netwerk/build/nsNetModule.cpp

>+    { "Simple Nested URI", 

I've been trying to use the classname as the classname:


I know the rule hasn't been applied universally to necko, but I'd really like
for it to be so.  Then, it would seem that classname could be made somewhat
useful.  Given a ClassID, you could easily map it to a classname that could
be more easily located in the source code.

>+    { "Safe About Protocol Handler", 

Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2006-05-23 14:50:34 PDT
> I don't see why we should discourage that.

I do.  They can't do anything but crash or call Read() when they get it.  The only reason we have a CID is so that we can implement nsISerializable.

> It doesn't leave the object in an inconsistent state.

It does.  Having a null inner URI is an inconsistent state.  Trying to use the object in that state will crash.  There is no way to get the object out of this inconsistent state.  So....

> Hmm... nsISupports** not equal to nsIURI**... do we need to care?

Hmm...  Not sure.  I copied the code in nsSimpleURI, but I guess there we _are_ guaranteed that the two are equal, right?

Note that I'm passing in the IID I want out, so I would expect the nsISupports* that comes back to in fact point to the right thing, right?  So I think this is ok.

I can generalize the nsIAboutModule API, sure.

> Can this just be "moz-safe-about:"

Maybe, but I'd have to read the impl of nsSimpleURI to tell. I'd rather not depend on that sort of detail in the impl.

I'll make the classname changes.

Thoughts on the CID thing for simple nested URIs?

Comment 5 User image Darin Fisher 2006-05-23 18:04:01 PDT
Hmm... it is generally assumed that it should be possible to construct a xpcom component and have something that is in a consistent state.  that means that there must be the notion of a valid default constructor.  I understand that we only have a component factory to support fastload.  I guess you want to avoid adding null checks for mInner, right?  I'm not sure what to suggest.  It may be wise to assume that the component may be instantiated by timel^h^h^h^h^hrandom folks :-/
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2006-05-23 18:34:01 PDT
I could probably add some null checks (with asserts if they fail) and throws, sure.  It's your module, so it's your call.
Comment 7 User image Darin Fisher 2006-05-23 19:05:48 PDT
OK, let's go for null checking then.  NS_ENSURE_TRUE(mInnerURI, NS_ERROR_NOT_INITIALIZED) would probably do it.
Comment 8 User image Ben Goodger (use ben at mozilla dot org for email) 2006-06-12 17:09:18 PDT
What about CanExecuteScripts? about:neterror has special dispensation there, as should probably any other internal page we have (I need this for about:feeds - right now I have to hack SSM). Or is that outside the scope of this...?
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2006-06-12 17:38:46 PDT
Outside the scope.  There are several followup patches I want to do if I ever get this in; Darin asked me to split them up into separate bugs.

Want to file a bug on the CanExecuteScripts thing and make it depend on this one?

I'll try to get that patch updated to Darin's comments ASAP.
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2006-06-13 16:29:06 PDT
Created attachment 225499 [details] [diff] [review]
Updated to darin's comments

Updated to darin's comments.  Again, the only expected behavior change is that pages will be able to link to about:buildconfig.  If desired, that can be avoided.
Comment 11 User image Daniel Veditz [:dveditz] 2006-06-15 16:51:20 PDT
Comment on attachment 225499 [details] [diff] [review]
Updated to darin's comments

Comment 13 User image Boris Zbarsky [:bz] (still a bit busy) 2006-06-19 16:52:56 PDT
Fixed.  Thanks for the reviews, all!
Comment 14 User image Boris Zbarsky [:bz] (still a bit busy) 2006-06-19 16:53:36 PDT
Mark, sorry about the camino thing.  I should have done the search in the "mozilla" lxr.  :(
Comment 15 User image Christian :Biesinger (don't email me, ping me on IRC) 2006-06-27 16:38:12 PDT
Comment on attachment 225499 [details] [diff] [review]
Updated to darin's comments

Index: extensions/python/xpcom/components/
+    def getURIFlags(self, aURI):
+        return 0;

this semicolon doesn't really seem to match surrounding style...
Comment 16 User image Boris Zbarsky [:bz] (still a bit busy) 2006-06-27 19:09:41 PDT
Hmm... yeah.  Must have been pretty automatic.  Mark, is not having it there OK in general ok?
Comment 17 User image Mark Hammond [:markh] 2006-07-12 03:18:40 PDT
In general, Python doesn't care about semi-colons - they can be used to separate mutiple statement on a single line, but otherwise are ignored.  It's not uncommon to see trailing semi-colons slipping in as people 'port' snippets from JS, C++, etc (well, not uncommon for me anyway :)

Note You need to log in before you can comment on or make changes to this bug.