890224 - SecurityReportTool needs notifications to allow for security reporting

Status: RESOLVED WONTFIX

(Core :: Security, defect)

Description

[Kailas]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/28.0.1500.52 Chrome/28.0.1500.52 Safari/537.36

Steps to reproduce:

Visit sites that contains security errors  such as https://people.mozilla.com/~mgoodwin/mixed
https://csptest.computerist.org

Security errors such as SSL certificate errors, mixed-content, CORS, HSTS, doesn't contain notifyObserver events to provide security related information to security report tool. Existing mechanism is to register event listener on nsIConsoleService and capture error strings. 
. 


Actual results:

lack of nofifyObservers for security related errors. 


Expected results:

All security related errors should be correctly categorized and give notification to allow security reporting

Comment 1

[Mihai Morar, (:MihaiMorar)]

I can confirm this issue on Latest Nightly:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130704 Firefox/25.0
(20130704031323)

Comment 2

[Kailas]

Attachment: Observer added for SSL error

Observer notification has a SSL error code in 3rd parameter.

Comment 3

[Kailas]

Attachment: Observer Notification for Mixed-content as well as for SSL error

Updated patch.
Mixed content observer notifications added for active as well as display contents and SSL observer notification now sends host name with SSL error code.

Comment 4

[Kailas]

Attachment: Observer Notification for Mixed-content as well as for SSL error

Added more information with mixed-content notiifcations. In particular added Document URI as 3rd parameter to observer.

Comment 5

[Mark Goodwin [:mgoodwin]]

Comment on attachment 773129 [details] [diff] [review]
Observer Notification for Mixed-content as well as for SSL error

Review of attachment 773129 [details] [diff] [review]:
-----------------------------------------------------------------

I think you want Tanvi (or someone else) to do a proper review; I spotted a couple of things you may want to address first though:

::: content/base/src/nsMixedContentBlocker.cpp
@@ +456,5 @@
>    // If the content is display content, and the pref says display content should be blocked, block it.
>    if (sBlockMixedDisplay && classification == eMixedDisplay) {
> +    // Add observers
> +    nsCOMPtr<nsIObserverService> observerService(do_GetService("@mozilla.org/observer-service;1"));
> +    if (observerService) {      

You'll want to remove any trailing whitespace from lines; reviewers will pick this up routinely and you'll save time if you've checked yourself beforehand. I'd recommend setting your editor up to highlight style issues like this.

@@ +460,5 @@
> +    if (observerService) {      
> +      if (rootDoc) {
> +        nsIURI* documentURI = rootDoc->GetDocumentURI();
> +        nsAutoCString spec;
> +         nsAutoString aData;

The 'a' prefix is usually used for arguments - 'aData' here should be just 'data' (or, better still, you could describe what's in it)

@@ +486,5 @@
>  
>    } else if (sBlockMixedScript && classification == eMixedScript) {
> +    // Add observers
> +    nsCOMPtr<nsIObserverService> observerService(do_GetService("@mozilla.org/observer-service;1"));
> +    if (observerService) {      

Trailing whitespace, again

@@ +490,5 @@
> +    if (observerService) {      
> +      if (rootDoc) {
> +        nsIURI* documentURI = rootDoc->GetDocumentURI();
> +        nsAutoCString spec;
> +        nsAutoString aData;

See comment above re. prefixes.

@@ +497,5 @@
> +        observerService->NotifyObservers(aContentLocation, "report-mixed-content", aData.get());
> +      } else {
> +        observerService->NotifyObservers(aContentLocation, "report-mixed-content", nullptr);
> +      }
> +    }

These blocks look near identical; if there are more of these it may be cleaner to move this to a function that notifies observers of something.

Comment 6

[Kailas]

Attachment: bug-890224.patch

@Mark: addressed your comments

Comment 7

[Mark Goodwin [:mgoodwin]]

Comment on attachment 777845 [details] [diff] [review]
bug-890224.patch

Review of attachment 777845 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me Kailas.

Comment 8

[Garrett Robinson [:grobinson]]

Kailas, please make sure you are assigned to the bugs you are currently working on. It was very difficult for me to find this bug and that was the primary reason why.

Comment 9

[Kailas]

(In reply to Garrett Robinson [:grobinson] from comment #8)
> Kailas, please make sure you are assigned to the bugs you are currently
> working on. It was very difficult for me to find this bug and that was the
> primary reason why.

Garrett: Sorry for any inconvenience caused. Thanks for assigning this bug to me. 
I didn't find an option to assign bug to me. After you have assigned bug to me, I can see Edit option in front of "Assigned To:". 

Is there any other way to assign bugs? So that in future I will be able to take charge of bugs, which I am writing a patch.

Comment 10

[Garrett Robinson [:grobinson]]

(In reply to Kailas from comment #9)
> Is there any other way to assign bugs? So that in future I will be able to
> take charge of bugs, which I am writing a patch.

In the top section next to "Assigned to:", there are buttons to edit or take the Asignee. Take is the same as edit, but autofills your bugmail address.

Comment 11

[Kailas]

Garrett: It seems that my account does not have permissions to see those buttons.  Please see the screenshot of a random bug (This screenshot shows visible things to my account) : http://s9.postimg.org/j8bb5n8dr/bugzilla_permissions.png

Is there any procedure to enable permissions to my account (username: [e-mail]) so that I will be able to assign bugs to me.

Comment 12

[Frederik Braun [:freddy]]

Comment on attachment 777845 [details] [diff] [review]
bug-890224.patch

Tanvi: Can you review this?

Comment 13

[Tanvi Vyas[:tanvi]]

Comment on attachment 777845 [details] [diff] [review]
bug-890224.patch

Hi Kailas.  Thanks for working on this patch and sorry for the delay!

This review is just for the nsMixedContentBlocker.cpp code changes and doesn't include the changes to /security/manager/ssl/src/TransportSecurityInfo.cpp.

>diff --git a/content/base/src/nsMixedContentBlocker.cpp b/content/base/src/nsMixedContentBlocker.cpp
>--- a/content/base/src/nsMixedContentBlocker.cpp
>+++ b/content/base/src/nsMixedContentBlocker.cpp
>@@ -21,16 +21,19 @@
> #include "nsIHttpChannel.h"
> #include "mozilla/Preferences.h"
> #include "nsIScriptObjectPrincipal.h"
> #include "nsISecureBrowserUI.h"
> #include "nsIDocumentLoader.h"
> #include "nsIWebNavigation.h"
> #include "nsLoadGroup.h"
> #include "nsIScriptError.h"
>+#include "nsIObserverService.h"
>+#include "nsIURI.h"
>+#include "nsGkAtoms.h"
Why is nsGKAtoms.h needed?  Also nsIURI is already used throughout the code without including it specifically.


>+static void
>+GenerateObserverNotification(nsIURI* aContentLocation,
>+                             nsIDocument* rootDoc)
aRootDoc instead of rootDoc

>+{
>+  // Add observers
>+  nsCOMPtr<nsIObserverService> observerService =
>+    do_GetService("@mozilla.org/observer-service;1");
>+  if (observerService) {
>+    if (rootDoc) {
>+      nsIURI* documentURI = rootDoc->GetDocumentURI();
>+      nsAutoCString spec;
>+      nsAutoString data;
>+      documentURI->GetSpec(spec);
>+      data.AppendASCII(spec.get());
>+      observerService->NotifyObservers(aContentLocation, "report-mixed-content", data.get());

Why use two nsAutoStrings here?  Can't you just do the following:
nsAutoCString docLocSpec;
documentURI->GetSpec(docLocSpec);
observerService->NotifyObservers(aContentLocation, "report-mixed-content", docLocSpec.get());


Do you also need to add in this code (I am not sure if you need it, but the LogMixedContentMessage() function uses it.  Perhaps you can ask Garrett):
NS_ConvertUTF8toUTF16 docLocSpecUTF16(docLocSpec);

In that case, you would then do:
observerService->NotifyObservers(aContentLocation, "report-mixed-content", docLocSpecUTF16.get());


>+    } else {
>+      observerService->NotifyObservers(aContentLocation, "report-mixed-content", nullptr);
>+    }
In which cases are you expecting that the rootDoc does not exist.  You pass it in in every call, so why do you need this else clause?



>   // If the content is display content, and the pref says display content should be blocked, block it.
>   if (sBlockMixedDisplay && classification == eMixedDisplay) {
>+    GenerateObserverNotification(aContentLocation, rootDoc);
Take a look at the LogMixedContentMessage() function - http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#155.  There are actually 6 different things you could report/observe from Mixed Content Blocker:

* mixed display content loaded (within this there are two possibilities - the users browser allows the mixed display content by default or it was blocked by default but the user explicitly decided to disable protection and load the content)
* mixed display content blocked
* mixed active content loaded (within this there are two possibilities - the users browser allows the mixed active content by default or it was blocked by default but the user explicitly decided to disable protection and load the content)
* mixed active content blocked.

How will the observer know which of these 4 events have taken place?  Moreover, mixed active content may be blocked but then allowed by the user.  With the current calls to GenerateObserverNotifications, the function will get called when
1) mixed display is blocked by default and the load detected is for mixed display.  Whether the content is actually loaded or blocked by the user has yet to be determined and is checked after the call to the observer notification.
2) mixed active content is blocked by default and the load detected is for mixed active.  Whether the content is actually loaded or blocked by the user has yet to be determined and is checked after the call to the observer notification.

You will completely miss cases where mixed display or mixed active content are allowed by the users browser by default.

The placement of the calls to GenearteObserverNofitication depend on which of these events you want to observe.  And you probably need another parameter to GenerateObserverNotifications to properly report on what event was triggered.

Comment 14

[BugBot [:suhaib / :marco]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:dveditz, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 15

[Frederik Braun [:freddy]]

Wow. Nine years. This is very far away from anything considered active or relevant. I'm closing as WONTFIX and clearing the needinfo.