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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This info should really live in the protocol handler.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

[Darin Fisher]

Comment on attachment 221838 [details] [diff] [review]
Patch

>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,
...
>+protected:
>+    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

>+static NS_DEFINE_CID(kSimpleNestedURICID, NS_SIMPLENESTEDURI_CID);

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

>+#define NS_SAFEABOUTPROTOCOLHANDLER_CID              \
>+{ /* 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
>+{
>+public:
...
>+    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:"


>+nsSafeAboutProtocolHandler::nsSafeAboutProtocolHandler()
>+{
>+}
>+
>+nsSafeAboutProtocolHandler::~nsSafeAboutProtocolHandler()
>+{
>+}

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:

        "nsSimpleNestedURI"

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", 

ditto

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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

[Darin Fisher]

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

[Boris Zbarsky [:bzbarsky]]

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

[Darin Fisher]

OK, let's go for null checking then.  NS_ENSURE_TRUE(mInnerURI, NS_ERROR_NOT_INITIALIZED) would probably do it.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

[Daniel Veditz [:dveditz]]

Comment on attachment 225499 [details] [diff] [review]
Updated to darin's comments

r=dveditz

Comment 12

[Mark Mentovai]

This landed on the trunk at 1402.  Bustage fix for Camino:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsAboutBookmarks.mm&branch=&root=/cvsroot&subdir=mozilla/camino/src/bookmarks&command=DIFF_FRAMESET&rev1=1.6&rev2=1.7

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed.  Thanks for the reviews, all!

Comment 14

[Boris Zbarsky [:bzbarsky]]

Mark, sorry about the camino thing.  I should have done the search in the "mozilla" lxr.  :(

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 225499 [details] [diff] [review]
Updated to darin's comments

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

this semicolon doesn't really seem to match surrounding style...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hmm... yeah.  Must have been pretty automatic.  Mark, is not having it there OK in general ok?

Comment 17

[Mark Hammond [:markh] [:mhammond]]

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