Closed Bug 337746 Opened 18 years ago Closed 18 years ago

[FIX]Move "safe about" hardcoding out of security manager


(Core :: Security: CAPS, defect, P1)






(Reporter: bzbarsky, Assigned: bzbarsky)




(1 file, 1 obsolete file)

This info should really live in the protocol handler.
Depends on: 337748
Attached patch Patch (obsolete) — Splinter Review
Attachment #221838 - Flags: superreview?(darin)
Attachment #221838 - Flags: review?(dveditz)
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.
Priority: -- → P1
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", 

Attachment #221838 - Flags: superreview?(darin) → superreview+
> 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?

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 :-/
I could probably add some null checks (with asserts if they fail) and throws, sure.  It's your module, so it's your call.
OK, let's go for null checking then.  NS_ENSURE_TRUE(mInnerURI, NS_ERROR_NOT_INITIALIZED) would probably do it.
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...?
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.
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.
Attachment #221838 - Attachment is obsolete: true
Attachment #225499 - Flags: superreview?(darin)
Attachment #225499 - Flags: review?(dveditz)
Attachment #221838 - Flags: review?(dveditz)
Comment on attachment 225499 [details] [diff] [review]
Updated to darin's comments

Attachment #225499 - Flags: review?(dveditz) → review+
Attachment #225499 - Flags: superreview?(darin) → superreview+
Fixed.  Thanks for the reviews, all!
Closed: 18 years ago
Resolution: --- → FIXED
Mark, sorry about the camino thing.  I should have done the search in the "mozilla" lxr.  :(
Depends on: 342108
Depends on: 342369
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...
Hmm... yeah.  Must have been pretty automatic.  Mark, is not having it there OK in general ok?
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 :)
QA Contact: caps
You need to log in before you can comment on or make changes to this bug.