203371 - Enhancement to the security model proposed in bug 183824

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect, P2)

Description

[harishd]

The new security model requires service providers to maintain an xml
file that lists trusted domains to gain access to public and private services. 
However, to access public service the client may not have to rely on the list
provided by the service provider instead can lookup a centralized list to
determine if same origin checks can be circumvented

The steps involved for the enhanced security model are as follows:

1) Client requests web-scripts-access.xml from the service provider. If
the file exists then determine access as dictated by the file.
2) If web-scripts-access.xml is not found then make a SOAP request to a
well known service that maintains a list of public services.
  - The transport URIs for the well known services can/should be
listed ( comma separated ) in prefs.js.
3) If not listed anywhere then throw an exception.

Note: The list provided by the service can be very specific unlike the
centralized list.

Comment 1

[harishd]

Attachment: Patch v1.0

Not ready for reviews..yet. But, feel free to go through the patch.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 121704 [details] [diff] [review]
Patch v1.0 

+  // Since the SOAP request, to the central server, will be made
+  // from the native code we can safely override cross-domain 
+  // checks by pushing in a null jscontext on the context stack.
+  nsCOMPtr<nsIJSContextStack> stack = 
+    do_GetService("@mozilla.org/js/xpc/ContextStack;1");
+  if (stack)
+    stack->Push(nsnull);
+
...
+    if (NS_FAILED(rv))
+      return rv;
...
+    if (NS_FAILED(rv))
+      return rv;  
...
+  if (stack) {
+    JSContext* cx;
+    stack->Pop(&cx);
+    NS_ASSERTION(!cx, "context should be null");
+  }

No returning early here before you've popped the null context you pushed on the
stack, that could let JS that executes after this point run with system
principals. Just break out of the loop on failure, then pop, then check for
failures and return early. And add comments about this.

Comment 3

[harishd]

>No returning early here before you've popped the null context you pushed on the
>stack, that could let JS that executes after this point run with system
>principals. Just break out of the loop on failure, then pop, then check for
>failures and return early. And add comments about this.

Yup...I would have missed that. Thanks for catching it.

Comment 4

[harishd]

Attachment: Patch v1.1 [ not final yet ]

Comment 5

[harishd]

Attachment: Patch v1.2

Comment 6

[harishd]

Comment on attachment 124572 [details] [diff] [review]
Patch v1.2

Note: I've moved a couple of helper methods into nsWSAUtils and have renamed a
few methods in nsWebScriptsAccess file. The changes to watch out for are
IsPublicService and checkAccess methods and nsDNSService impl.

Comment 7

[harishd]

correction: nsDNSService/nsDNSListener

Comment 8

[Darin Fisher]

Comment on attachment 124572 [details] [diff] [review]
Patch v1.2

>Index: extensions/webservices/security/src/nsWSAUtils.cpp

>+ * Portions created by the Initial Developer are Copyright (C) 2002

2003 :-)

>+#define SECURITY_PROPERTIES "chrome://communicator/locale/webservices/security.properties"

nit:

static const char kSecurityProperties[] = "...";

in case you ever need to reference this string literal again, it is
good to use a declare the string literal like this so that multiple
copies of it won't appear in the resulting object file.  of course,
most compilers coalesce multiple occurances of string literals, but
some (older versions of GCC that we use for mozilla releases) don't.
even though you only reference this string literal once, it might be
good to declare it instead of using a #define so that if it is ever
referenced again, you won't have to go back and fixup this declaration.


>+nsWSAUtils::VerifyIsEqual()
>+{
>+  static PRUint32 size = sizeof(kStrings)/sizeof(kStrings[0]);

nit: nsMemory.h defines the macro NS_ARRAY_LENGTH for this.


>+  NS_INIT_ISUPPORTS();

this macro now-a-days expands to nothing and need not be included.
if you look around the tree in fact most of these have been removed ;)


>+    mFullyQualifiedDomainName.Assign(aHostEnt->hostEnt.h_name);

technically speaking FQDN ends with '.' and h_name doesn't end
with a dot, so maybe mOfficialHostName would be better ;-)


>+nsDNSListener::GetFullyQualifiedDomainName(nsIURI* aServiceURI,
>+                                           char** aResult)
...
>+  while (!mLookupFinished)
>+    eventQ->ProcessPendingEvents();

hmm... does this really work?  i don't see you posting an
event to the UI thread anywhere.  seems like you would need to
post a PLEvent to the UI thread's event queue from OnStopLookup
to cause ProcessPendingEvents to run.  Otherwise, you are just
waiting for some other UI event to cause ProcessPendingEvents
to run.  This might lead to wierd delays until the user moves
their mouse or something like that.


>Index: extensions/webservices/security/src/nsWSAUtils.h

>+ * Portions created by the Initial Developer are Copyright (C) 2002

2003


>+  nsXPIDLCString mFullyQualifiedDomainName;

why not nsCString?  nsXPIDLCString is only to be used when you
need to use getter_Copies.


>Index: extensions/webservices/security/src/nsWebScriptsAccess.cpp

>+    nsXPIDLCString path;
>+    url->GetPrePath(path);
>+    nsXPIDLCString directory;
>+    url->GetDirectory(directory);
>+    path += directory;

use nsCAutoString instead of nsXPIDLCString here.


>+  if (*aEntry  && ((*aEntry)->mFlags & WSA_FILE_DELEGATED))
>+    return CreateDelegatedEntry(aEntry);
>+  return NS_OK;
>+}

nit: slight codesize savings...

  if (*aEntry  && ((*aEntry)->mFlags & WSA_FILE_DELEGATED))
    rv = CreateDelegatedEntry(aEntry);
  return rv;


>+nsresult 
>+nsWebScriptsAccess::CreateEntry(nsIDOMDocument* aDocument,
>+                                const PRBool aIsDelegated,
>+                                AccessInfoEntry** aEntry)
> {
>+  NS_ENSURE_ARG_POINTER(aDocument);
>+  PRBool valid;
>+  nsresult rv = ValidateDocument(aDocument, &valid);
>   NS_ENSURE_SUCCESS(rv, rv);
>+  if (!valid) {
>+    return NS_OK;
>+  }

hmm... doesn't |return NS_OK| indicate that |*aEntry| is set
to something valid?  in this case, it is left uninitialized.
seems dangerous to me.


>+  if (count) {
>+    rv = CreateEntry(allowList, aEntry);
>+  }
>+  else {
>+    // Since there are no ALLOW elements present grant access to all.
>+    rv = CreateEntry(WSA_GRANT_ACCESS_TO_ALL, aEntry);
>+  }
>+
>+  return NS_OK;

did you mean:

  return rv;

instead?


>+  nsXPIDLCString path;
>+  url->GetPrePath(path);
>+  nsXPIDLCString directory;
>+  url->GetDirectory(directory);
>+  path += directory;

nsCAutoString instead of nsXPIDLCString.


>+    nsXPIDLCString fqdn;
>+    rv = listenerInst->GetFullyQualifiedDomainName(mServiceURI, 
>+                                                   getter_Copies(fqdn));

wouldn't it be nice if the second param were |nsACString&| instead?
then you could use nsCAutoString here to avoid a heap allocation in
most cases.


>+nsWebScriptsAccess::IsPublicService(const char* aHost, PRBool* aReturn)
>+{
>+  *aReturn = PR_FALSE;
>+  nsresult rv = NS_OK;
>+  // Cache the master servers included in the prefs.
>+  if (mMasterServers.Count() == 0) {
>+    nsCOMPtr<nsIPrefBranch> prefBranch = 
>+      do_GetService(NS_PREFSERVICE_CONTRACTID);
>+  
>+    if (!prefBranch)
>+      return NS_OK;

footprint nit:

	return rv;


>+    const PRUnichar *inputs[5]  = 
>+      { 
>+        kNamespace2002.get(),
>+        NS_LITERAL_STRING("canAccess").get(),

hmm... on some platforms NS_LITERAL_STRING expands to NS_ConvertASCIItoUCS2,
which means that by the time ReportError is called, the ~NS_ConvertASCIItoUCS2
would have run.  As a result, you would be accessing unowned memory.

for this reason, you should use NS_NAMED_LITERAL_STRING instead.


>+        faultNamespaceURI.get(),
>+        faultCode.get(),
>+        faultString.get()
>+      };
>+    return nsWSAUtils::ReportError(
>+                         NS_LITERAL_STRING("SOAPFault").get(), 
>+                         inputs, 5);

Comment 9

[Johnny Stenback (:jst)]

This patch seems to make a SOAP call to a canAccess() method:

+    rv = call->Encode(nsISOAPMessage::VERSION_1_1,
+                      NS_LITERAL_STRING("canAccess"), 
+                      kNamespace2002, // The target URI
+                      0, 0, 1, bodyBlocks);

That's IMO not what we want to ask here, the question the server will answer is
"is this web service public", so the name of the method should IMO reflect that.
How about "isServicePublic"?

And the code that parses the pref that holds the master servers (which should be
master services, not servers, right?) could be a bit more robust, i.e. deal with
whitespaces around commas and stuff like that...

Comment 10

[harishd]

Attachment: Patch v1.3 [ not yet final ]

Includes Darin's and Jst's suggestions except the eventQ question. Will have to
talk to Darin.

Comment 11

[harishd]

Attachment: Patch v1.4

Includes Darin's and Jst's suggestions.

Comment 12

[harishd]

Comment on attachment 126396 [details] [diff] [review]
Patch v1.4

Note: I'm thinking of replacing the SOAP call , to the master services, with a
simple HTTP Post to avoid webservices dependency. I'll open up a new bug to
address that issue. For now SOAP is fine :)

Comment 13

[Darin Fisher]

Comment on attachment 126396 [details] [diff] [review]
Patch v1.4

>Index: extensions/webservices/security/src/nsWSAUtils.cpp

>+static void* PR_CALLBACK
>+DummyHandler(PLEvent *aEvent)
>+{
>+  return 0;
>+}
>+
>+static void PR_CALLBACK
>+DestroyHandler(PLEvent *aEvent)
>+{
>+  delete aEvent;
>+}
>+ 
>+NS_IMPL_THREADSAFE_ISUPPORTS1(nsDNSListener,
>+                              nsIDNSListener);
>+
>+nsDNSListener::nsDNSListener()
>+  : mLookupFinished(PR_FALSE)
>+{
>+}
>+
>+nsDNSListener::~nsDNSListener()
>+{
>+}
>+
>+NS_IMETHODIMP
>+nsDNSListener::OnStartLookup(nsISupports* aContext, const char* aHost)
>+{
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDNSListener::OnFound(nsISupports* aContext, 
>+                       const char* aHost, 
>+                       nsHostEnt* aHostEnt)
>+{
>+  mLookupFinished = PR_TRUE;
>+  if (aHostEnt)
>+    mOfficialHostName.Assign(aHostEnt->hostEnt.h_name);
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDNSListener::OnStopLookup(nsISupports* aContext, 
>+                            const char* aHost,
>+                            nsresult aStatus)
>+{
>+  mLookupFinished = PR_TRUE;
>+  
>+  // Post an event to the UI thread's event queue to cause
>+  // ProcessPendingEvents to run.
>+  nsresult rv = NS_OK;
>+  if(!mEventQService) {
>+    mEventQService = do_GetService(kEventQueueServiceCID, &rv);
>+    if (NS_FAILED(rv))
>+      return rv;
>+  }
>+  
>+  nsCOMPtr<nsIEventQueue> uiEventQ;
>+  mEventQService->GetThreadEventQueue(NS_UI_THREAD, getter_AddRefs(uiEventQ));

hmm... why are you caching the event queue service into mEventQService?
are you using this listener object with more than one DNS lookup call?
if so, then you could have thread synchronization problems here.  i
think it would be better to allocate one nsDNSListener object for each
call to nsIDNSService::Lookup.

also, from nsEventQueueUtils.h, you can simply call NS_GetMainEventQ,
which will return the UI thread's current event queue.


>+
>+  if (uiEventQ) {
>+    PLEvent* event = new PLEvent();
>+    NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
>+
>+    PL_InitEvent(event, this, DummyHandler, DestroyHandler);
>+    rv = uiEventQ->PostEvent(event);

if (NS_FAILED(rv)), then call PL_DestroyEvent(event);


>+  rv = dns->Lookup(host, this, nsnull, getter_AddRefs(dummy));
>+  
>+  if (NS_FAILED(rv)) 
>+    return rv;
>+
>+  if(!mEventQService) {
>+    mEventQService = do_GetService(kEventQueueServiceCID, &rv);
>+    if (NS_FAILED(rv))
>+      return rv;
>+  }

ah.. you're even having a race here to assign mEventQService.
yuck!  i think it'd be a lot better if you just allocated a little
object to represent the result of the lookup.  make it implement
nsIDNSListener and make it own a reference to the object which
is currently called nsDNSListener... and when OnStopLookup is
called post the event to the main event queue... and when that
event fires, set mLookupFinished.  that way, you are only tweaking
the nsDNSListener object on the main thread.  otherwise, i think
you're just asking for pain ;-)

Comment 14

[Darin Fisher]

incidentally, or fwiw...

www.yahoo.com has a canonical name of www.yahoo.akadns.net

i just happened to be doing some DNS testing, saw this, and remembered this bug.
 maybe it's just an interesting datapoint... i'm sure yahoo.com isn't the only
major website which has a canonical name pointing outside its domain.

Comment 15

[harishd]

Attachment: patch v1.5

Includes changes suggested by darin.

Comment 16

[Darin Fisher]

Comment on attachment 126630 [details] [diff] [review]
patch v1.5

>Index: extensions/webservices/security/src/nsWSAUtils.cpp

>+    PL_InitEvent(event, this, EventHandler, DestroyHandler);

hmm... i was just wondering if it might be a good idea to make this
event own the nsDNSListener object.  in particular, is it possible
for the function calling ProcessPendingEvents to return before this
PLEvent executes?  or if not, then here's another question: what
happens if the user tries to close the browser while we are performing
this lookup?  or is that not possible because the UI will be modal?

seems like if the user cannot do something to interrupt the PPE call,
then all should be fine as is.

r=darin

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 126630 [details] [diff] [review]
patch v1.5

- In nsWSAUtils::GetOfficialHostName():

+  nsCOMPtr<nsIDNSListener> listener = new nsDNSListener();
+  NS_ENSURE_TRUE(listener, NS_ERROR_OUT_OF_MEMORY);
...
+  nsDNSListener* listenerInst = 
+      NS_REINTERPRET_CAST(nsDNSListener*, listener.get());

How about making |listener| an nsRefPtr<nsDNSListener> in stead to avoid the
cast and the extra local variable?

- In EventHandler():

+  return 0;

This returns a void*, so maybe use nsnull here?

- In nsDNSListener::OnStartLookup():

+{
+    return NS_OK;
+}

Over-eager indentation, use two spaces for consistency :-)

- In nsWSAUtils.h:

+class nsWSAUtils {
+  public:
+    static PRBool IsEqual(const nsAString& aLhs, const nsAString& aRhs);
...
+  private:
...
+    ~nsWSAUtils() {}
+};

The other header file (and most of Mozilla) uses different indentation of class
declarations, something more like:

+class nsWSAUtils
+{
+public:
+  static PRBool IsEqual(const nsAString& aLhs, const nsAString& aRhs);
...
+private:
...
+  ~nsWSAUtils() {}
+};

- In nsWebScriptsAccess.cpp

+#define WSA_GRANT_ACCESS_TO_ALL    (1 << 0)
+#define WSA_FILE_NOT_FOUND	    (1 << 1)
+#define WSA_FILE_DELEGATED	    (1 << 2)
+#define SERVICE_LISTED_PUBLIC	    (1 << 3)
+#define HAS_GLOBAL_SERVER_DECISION (1 << 5)

Why 5 and not 4 on the last one?

- In nsWebScriptsAccess::CreateEntry():

+  nsresult rv = 
+    GetDocument(PromiseFlatCString(nsDependentCString(aKey) + 
+		 NS_LITERAL_CSTRING("web-scripts-access.xml")).get(),
+		 getter_AddRefs(document));

No need for that NS_LITERAL_CSTRING().get() there when you can simply pass in
the raw literal.

- In nsWebScriptsAccess::CreateEntry(nsIDOMDocument* aDocument,
				     const PRBool aIsDelegated,
				     AccessInfoEntry** aEntry):

+    rv = aDocument->GetElementsByTagNameNS(kNamespace2002, kDelegateTag, 
+					   getter_AddRefs(delegateList));

and

+  rv = aDocument->GetElementsByTagNameNS(kNamespace2002, kAllowTag, 
+					 getter_AddRefs(allowList));

Off-by-one indentation of next-line arguments.

- In nsWebScriptsAccess::IsPublicService():

+  // Since the SOAP request, to the central server, will be made

Loose the commas.

+    const PRUnichar *inputs[5]  = 
+      { 
+	 kNamespace2002.get(),
+	 NS_LITERAL_STRING("canAccess").get(),
+	 faultNamespaceURI.get(),
+	 faultCode.get(),
+	 faultString.get()
+      };
+    return nsWSAUtils::ReportError(
+			  NS_LITERAL_STRING("SOAPFault").get(), 
+			  inputs, 5);

The data in the NS_LITERAL_STRING() in inputs will be out of scope (using some
compilers) and the above code will crash. Pull the NS_LITERAL_STRING out into
an NS_NAMED_LITERAL_STRING() and point to its internal string in inputs.

- In nsWebScriptsAccess.h:

-class nsWebScriptsAccess : public nsIWebScriptsAccessService
+class nsWebScriptsAccess : public nsIWebScriptsAccessService		        

You're only adding whitespace to the end here. Don't.

sr=jst with those issues, and Darin's issue fixed.

Comment 18

[harishd]

Fix is in.