Bug 1364723 - Bug 968342 and bug 1361020 follow-up: Only check mailbox/imap/JsAccount messages for 'cross-loading'. r=rkent a=jorgk RELEASE_BASE_20170612
authorJorg K <jorgk@jorgk.com>
Tue, 06 Jun 2017 17:22:00 +0200
changeset 27889 0efb66a4b2cb16e05d20473a64eb8c6f92ea9b45
parent 27888 d34bebe73f47dcacd4628a6775add93a96fd2e39
child 27890 9722a9e139f491f10ea937baeaac2317a077f9a1
push id1964
push usermozilla@jorgk.com
push dateMon, 12 Jun 2017 13:18:47 +0000
treeherdercomm-beta@0efb66a4b2cb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrkent, jorgk
bugs1364723, 968342, 1361020
Bug 1364723 - Bug 968342 and bug 1361020 follow-up: Only check mailbox/imap/JsAccount messages for 'cross-loading'. r=rkent a=jorgk
mailnews/base/src/nsMsgContentPolicy.cpp
mailnews/news/src/nsNntpUrl.cpp
--- a/mailnews/base/src/nsMsgContentPolicy.cpp
+++ b/mailnews/base/src/nsMsgContentPolicy.cpp
@@ -20,16 +20,17 @@
 #include "nsIWebNavigation.h"
 #include "nsContentPolicyUtils.h"
 #include "nsIDOMHTMLImageElement.h"
 #include "nsIFrameLoader.h"
 #include "nsIWebProgress.h"
 #include "nsMsgUtils.h"
 #include "nsThreadUtils.h"
 #include "mozilla/mailnews/MimeHeaderParser.h"
+#include "nsINntpUrl.h"
 
 static const char kBlockRemoteImages[] = "mailnews.message_display.disable_remote_image";
 static const char kAllowPlugins[] = "mailnews.message_display.allow_plugins";
 static const char kTrustedDomains[] =  "mail.trusteddomains";
 
 using namespace mozilla::mailnews;
 
 // Per message headder flags to keep track of whether the user is allowing remote
@@ -234,47 +235,63 @@ nsMsgContentPolicy::ShouldLoad(uint32_t 
   // If the requesting location is safe, accept the content location request.
   if (IsSafeRequestingLocation(aRequestingLocation))
     return rv;
 
   // Now default to reject so early returns via NS_ENSURE_SUCCESS
   // cause content to be rejected.
   *aDecision = nsIContentPolicy::REJECT_REQUEST;
 
-  // If aContentLocation uses a protocol we handle, we require that the load
-  // comes from the "normalised" principal which exists for imap, mailbox, news
-  // and other protocols where the URL inherits from nsIMsgMessageUrl.
-  // This is basically a "same origin" test. For other protocols we check the
-  // pre-paths.
+  // We want to establish the following:
+  // \--------\  requester    |               |              |
+  // content   \------------\ |               |              |
+  // requested               \| mail message  | news message | http(s)/data etc.
+  // -------------------------+---------------+--------------+------------------
+  // mail message content     | load if same  | don't load   | don't load
+  // mailbox, imap, JsAccount | message (1)   | (2)          | (3)
+  // -------------------------+---------------+--------------+------------------
+  // news message             | don't load (4)| load (5)     | load (6)
+  // -------------------------+---------------+--------------+------------------
+  // http(s)/data, etc.       | (default)     | (default)    | (default)
+  // -------------------------+---------------+--------------+------------------
   nsCOMPtr<nsIMsgMessageUrl> contentURL(do_QueryInterface(aContentLocation));
   if (contentURL) {
+    nsCOMPtr<nsINntpUrl> contentNntpURL(do_QueryInterface(aContentLocation));
+    if (!contentNntpURL) {
+      // Mail message (mailbox, imap or JsAccount) content requested, for example
+      // a message part, like an image:
+      // To load mail message content the requester must have the same
+      // "normalised" principal. This is basically a "same origin" test, it
+      // protects against cross-loading of mail message content from
+      // other mail or news messages.
+      nsCOMPtr<nsIMsgMessageUrl> requestURL(do_QueryInterface(aRequestingLocation));
+      // If the request URL is not also a message URL, then we don't accept.
+      if (requestURL) {
+        nsCString contentPrincipalSpec, requestPrincipalSpec;
+        nsresult rv1 = contentURL->GetPrincipalSpec(contentPrincipalSpec);
+        nsresult rv2 = requestURL->GetPrincipalSpec(requestPrincipalSpec);
+        if (NS_SUCCEEDED(rv1) && NS_SUCCEEDED(rv2) &&
+            contentPrincipalSpec.Equals(requestPrincipalSpec))
+          *aDecision = nsIContentPolicy::ACCEPT; // (1)
+      }
+      return NS_OK; // (2) and (3)
+    }
+
+    // News message content requested. Don't accept request coming
+    // from a mail message since it would access the news server.
     nsCOMPtr<nsIMsgMessageUrl> requestURL(do_QueryInterface(aRequestingLocation));
-    // If the request URL is not also a message URL, then we don't accept.
     if (requestURL) {
-      nsCString contentPrincipalSpec, requestPrincipalSpec;
-      contentURL->GetPrincipalSpec(contentPrincipalSpec);
-      requestURL->GetPrincipalSpec(requestPrincipalSpec);
-      if (contentPrincipalSpec.Equals(requestPrincipalSpec))
-        *aDecision = nsIContentPolicy::ACCEPT;
+      nsCOMPtr<nsINntpUrl> requestNntpURL(do_QueryInterface(aRequestingLocation));
+      if (!requestNntpURL)
+        return NS_OK; // (4)
     }
+    *aDecision = nsIContentPolicy::ACCEPT; // (5) and (6)
     return NS_OK;
   }
 
-  // Compare pre-paths.
-  nsCOMPtr<nsIMsgMailNewsUrl> mailnewsUrl(do_QueryInterface(aContentLocation));
-  if (mailnewsUrl) {
-    nsCString contentPrePath, requestingPrePath;
-    aContentLocation->GetPrePath(contentPrePath);
-    aRequestingLocation->GetPrePath(requestingPrePath);
-    if (contentPrePath.Equals(requestingPrePath)) {
-      *aDecision = nsIContentPolicy::ACCEPT;
-      return NS_OK;
-    }
-  }
-
   // If exposed protocol not covered by the test above or protocol that has been
   // specifically exposed by an add-on, or is a chrome url, then allow the load.
   if (IsExposedProtocol(aContentLocation))
   {
     *aDecision = nsIContentPolicy::ACCEPT;
     return NS_OK;
   }
 
@@ -416,17 +433,17 @@ nsMsgContentPolicy::IsExposedProtocol(ns
 {
   nsAutoCString contentScheme;
   nsresult rv = aContentLocation->GetScheme(contentScheme);
   NS_ENSURE_SUCCESS(rv, false);
 
   // Check some exposed protocols. Not all protocols in the list of
   // network.protocol-handler.expose.* prefs in all-thunderbird.js are
   // admitted purely based on their scheme.
-  // news, snews, nntp, imap, pop and mailbox are checked before the call
+  // news, snews, nntp, imap and mailbox are checked before the call
   // to this function by matching content location and requesting location.
   if (MsgLowerCaseEqualsLiteral(contentScheme, "mailto") ||
       MsgLowerCaseEqualsLiteral(contentScheme, "addbook") ||
       MsgLowerCaseEqualsLiteral(contentScheme, "about"))
     return true;
 
   // check if customized exposed scheme
   if (mCustomExposedProtocols.Contains(contentScheme))
--- a/mailnews/news/src/nsNntpUrl.cpp
+++ b/mailnews/news/src/nsNntpUrl.cpp
@@ -299,30 +299,17 @@ NS_IMETHODIMP nsNntpUrl::GetKey(nsMsgKey
 {
   NS_ENSURE_ARG_POINTER(key);
   *key = m_key;
   return NS_OK;
 }
 
 NS_IMETHODIMP nsNntpUrl::GetPrincipalSpec(nsACString& aPrincipalSpec)
 {
-  // URLs look like this:
-  // news://server:port/folder?group=ggg&key=nnn [ &part=ppp &filename=fff ].
-  // Just strip the part which will also remove the filename.
-  // Normalised spec: news://server:port/folder?group=ggg&key=nnn
-  nsCOMPtr<nsIMsgMailNewsUrl> mailnewsURL;
-  QueryInterface(NS_GET_IID(nsIMsgMailNewsUrl), getter_AddRefs(mailnewsURL));
-
-  nsAutoCString spec;
-  mailnewsURL->GetSpecIgnoringRef(spec);
-  int32_t ind = spec.Find("&part");
-  if (ind != kNotFound)
-    spec.SetLength(ind);
-  aPrincipalSpec.Assign(spec);
-  return NS_OK;
+  return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP nsNntpUrl::SetUri(const char * aURI)
 {
   mURI = aURI;
   return NS_OK;
 }