Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Reporter
	Description • 11 years ago

+++ This bug was initially created as a clone of Bug #839120 +++

(In reply to Brian Smith (:bsmith) from bug #839120 comment #14)
> SafeBrowsing is a security check, and network security checks should be done
> by the parent process anyway, not by the child process. So, there should be
> no SafeBrowsing code running in the child process in the first place. IIRC,
> the SafeBrowsing service suspects the channel while the check is done, and
> then resumes it when the check is complete. That suspend/resume should
> happen in the parent process. So, there shouldn't be any need for
> complicated proxying code. Instead, I think the difficulty is in migrating
> the code that connects the safe browsing from the place it is currently done
> (which would run in the child processes) to a place that runs in the parent
> processes. If things are done this way, then a compromised child process
> will not be able to influence the SafeBrowsing decision.
> 
> Also note that, as of now, NSS and most of PSM are unavailable in child
> processes, and URL classifier does use NSS/PSM for its cryptography. And,
> certificate validation and other network security checks are done the same
> way.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 1 • 10 years ago

GCP, how bad do you think this bug will be? (i.e. give me a time estimate)

	Gian-Carlo Pascutto [:gcp]
	Comment 2 • 10 years ago

The problem is that although I know how SafeBrowsing updates work, they likely aren't the real problem. i.e. the remote lookups currently happen as async calls on the main thread from the nsUrlClassifierDBService. The database update IO is already done on a worker thread. All of those things likely have to remain in the main process (also because they do crypto). The problem is the interaction with the content processes.

SafeBrowsing is linked with the rest of the browser via these places:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#4644
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsChannelClassifier.cpp
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4582

Maybe someone who knows nsHttpChannel can give a more informed idea about this? SafeBrowsing works by suspending the Channel until it gets a decision, then setting an error if the link is considered dangerous. Then nsDocShell picks up that error and redirects to about:blocked with suitable parameters (malware/phishing).

Basically I think you need someone who understands nsHttpChannel more than you need someone who understands SafeBrowsing.

	Josh Matthews [:jdm]
	Comment 3 • 10 years ago

The nsHttpChannel code runs in the main process, and I would expect the NS_ERROR_PHISHING_URI error to propagate just fine to the child channel and therefore to the docshell code in the child. What exactly are you worried about in this scenario?

	Gian-Carlo Pascutto [:gcp]
	Comment 4 • 10 years ago

I'm not speccifically worried about anything. I'm pointing out how it works. I have no idea how the above relates to e10s, or specifically, what work there is to be done if everything will work "just fine".

If nsHttpChannel runs in the main process, including the nsChannelClassifier part, I'm not sure if there is anything that need to be done?

	Yusuke Yamamoto
	Comment 5 • 10 years ago

Hi all,

I try to run SafeBrowsing feature on Firefox OS, and coded some modifications to Gecko.
https://github.com/KDDI-tech/gecko/tree/safebrowsing

You can see the warning pages which are displayed by malware or attack web pages.
Although, actually, this is WIP code,  there are some issues.

1. can’t update cache DB on safebrowsing feature.
  modified clientID “B2G"  to “Firefox”. in SafeBrowsing.jsm L:131, clientID have a value “B2G”, the safebrowsing feature can’t update cache DB, because google’s safe browsing server reject the connection.

2. jslib doesn’t work correctory 
 in my code, the object of jslib has “FakeBackStagePath”, so we can’t access some modules[1] that over jslib.
 [1] : the modules under “ B2G/gecko/toolkit/components/url-classifier/content “, e.g. jslib.RequestBackoff, jslib.Function.prototype.inherits

We believe that this code help you to implement SafeBrowsing feature.

One last thing, actuary I don't know why SafeBrowsing feature doesn't work on Firefox OS.
Are there any known issues?

Best regards.

	Gian-Carlo Pascutto [:gcp]
	Comment 6 • 10 years ago

I think you want bug 839120. For this bug I think there's an outstanding issue with the block pages not working correctly. I have no idea what issues there are on Firefox OS but it looks you found some.

	Yusuke Yamamoto
	Comment 7 • 10 years ago

Hi gcp,
Thanks comments. got it.
Although, I guess I'm having a quite bit confusion that what kind of situation about SafeBrowsing.

Is there the META bug about SafeBrowsing feature, and is there any person who implement about SafeBrowsing feature?

	Gian-Carlo Pascutto [:gcp]
	Comment 8 • 10 years ago

This is the meta bug for making SafeBrowsing work with e10s and I linked you the meta bug on making it work on Firefox OS.

All the info we know is in those bugs. If you read them, you'll see we guess that SafeBrowsing *probably* mostly works, but that some small bugs *probably* remain and will have to be fixed. *We don't know for sure what they are until someone carefully tests the feature.* All the info we have is in the bugs.

I tested this a bit on desktop and concluded that the buttons on the about:blocked page don't work on e10s mode. There may be more bugs, that's just the first one I ran into. 

I haven't tested Firefox OS. Nobody, besides you, is currently actively looking into it, which means you probably know more than us about what is needed to make it work. If you find bugs and are able to fix them, we'd *very* much appreciate patches: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

I hope this answer your question, because I'm not quite sure what you're actually asking.

	[:mmc] Monica Chew (no longer reading bugmail)
	Comment 10 • 10 years ago

(In reply to Chris Peterson (:cpeterson) from comment #9)
> Safe Browsing test sites:
> 
> http://www.itisatrap.org/firefox/its-a-trap.html
> http://www.itisatrap.org/firefox/its-an-attack.html

It's true those are test sites, but they trigger a very different code path than non-test sites. I recommended that you try to use sample urls from phishtank.com (make sure that they are also caught in Chrome) as much as possible to avoid these differences.

	Chris Peterson [:cpeterson]
	Comment 11 • 10 years ago

Moving to M3 milestone. These bugs affect e10s dogfooding but Brad and Gavin did not think they were M2 blockers.

	Jim Mathies [:jimm]
	Comment 12 • 10 years ago

Aside from the two bugs I filed here, I'm not finding a lot of issues with pb mode in e10s. Also, I didn't find a lot of disable tests related to pb mode. If anyone knows of something that's broken, please post a description here.

	Gian-Carlo Pascutto [:gcp]
	Comment 13 • 10 years ago

Private browsing has nothing to do with SafeBrowsing.

	Jim Mathies [:jimm]
	Comment 14 • 10 years ago

(In reply to Gian-Carlo Pascutto [:gcp] from comment #13)
> Private browsing has nothing to do with SafeBrowsing.

Oh, thought they were basically the same. 

I tested blocked page test pages as well, didn't have any problems. about:blocked comes up and the chrome in the page works.

I'm tempted to resolve this bug wfm. I'm really not finding a lot of issues here. gcp, any areas that are broken that you know of in e10s mode?

	Gian-Carlo Pascutto [:gcp]
	Comment 15 • 10 years ago

Needinfoing myself to take a look and see if everything works.

	Jim Mathies [:jimm]
	Comment 16 • 10 years ago

Resolving this for now since its basically a tracker that never accumulated any bugs. If we find something broken with safe browsing and e10s lets file and triage those issues in individual bugs.