899099 - Create a method that can be shared across different components to validate if a URI is secure

Status: NEW

(Core :: Security, defect)

Description

[Ivan Alagenchev :ialagenchev]

nsMixedContentBlocker(http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#290) has logic to validate if a URI is secure. The same logic is reused via duplication in InsecurePasswordUtils.js for https://bugzilla.mozilla.org/show_bug.cgi?id=762593
We need to create a method that can be used across the various components that need to validate if a URI is secure and avoid duplication of code. nxMixedContentBlocker and InsecurePasswordUtils will then have to be modified to invoke the newly created method.

Comment 1

[Tanvi Vyas[:tanvi]]

Dan pointed out that we should be checking for nested protocols.

Example:
jar:jar:https://... 
view-source:jar:https://...

The actual transport mechanism is https, but we would end up blocking these.  Not a big deal right now (we are over-blocking something that is probably rare).  No reason for view-source to be embedded within a page anyway.

But it could become a security problem.  Imagine an addon registers a handler foo: that and gives it a protocol flag URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT since it is local or encrypted.

Then image we have an embedded content like
foo:http://...

Mixed Content Blocker would allow this HTTP connection since it doesn't check the innermost protocol.

We can check the innermost protocol with NS_GetInnermostURI().

When we do create this generic function, we will have to be careful about what we call it and comment it heavily so people know when they can and cannot use it.  Insecure Passwords and Mixed Content are both cases where the word "secure" means free form eavesdroppers and MITM attackers.  But another use case might need "secure" to mean something stricter (or less strict) than that.

Comment 2

[Tanvi Vyas[:tanvi]]

Hmmm... these test page implies we are checking nested urls:

https://people.mozilla.com/~tvyas/viewsourceiframe.html - viewsource:https:// allowed.

https://people.mozilla.com/~tvyas/viewsourcemixediframe.html - viewsource:http:// blocked by mixed content blocker.

Comment 3

[Tanvi Vyas[:tanvi]]

Potential nesting protocols:
view-source
jar
feed
pcast

Comment 4

[Tanvi Vyas[:tanvi]]

Happy to mentor this bug if someone would like to take it on.

Comment 5

[matthew.lazarow]

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> Happy to mentor this bug if someone would like to take it on.

Hi Tanvi, I'm looking for a good first bug to get started here but when I went looking for the file I cant find it; looking at bug 624883 its showing resolved. If there's still an issue I'd love to help and learn how this whole procedure works. Thanks!

Comment 6

[Tanvi Vyas[:tanvi]]

(In reply to matthew.lazarow from comment #5)
> (In reply to Tanvi Vyas [:tanvi] from comment #4)
> > Happy to mentor this bug if someone would like to take it on.
> 
> Hi Tanvi, I'm looking for a good first bug to get started here but when I
> went looking for the file I cant find it; looking at bug 624883 its showing
> resolved. If there's still an issue I'd love to help and learn how this
> whole procedure works. Thanks!

Hi Matthew,
I'm glad you are interested in contributing!  What file are you referring to?  Bug 624883 was about disallowing view-source in an iframe.  This bug is about making a generic method for determining whether a uri's is "secure"[1] based on the uri's ProtocolHandler flags[2].  The method would take a uri as an input parameter.  It would perform some checks on it and return true or false depending on whther the url is secure for these contexts or not.

There are multiple pieces of the code that do something like this currently, and it would be nice to have a utility function instead[3].

Also note that none of the current consumers handle nesting.  We could make the utility function properly check the protocol handler flags against each nested uri.

[1] the uri has a scheme that is secure to load within an HTTPS page (for mixed content blocking) or a scheme that is secure enough to have a password field.
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIProtocolHandler.idl#131
[3] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#458, https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm#83, http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#2954, http://mxr.mozilla.org/mozilla-central/source/image/imgRequest.cpp#1258, http://mxr.mozilla.org/mozilla-central/source/image/imgRequest.cpp#141

Comment 7

[matthew.lazarow]

Hi Tanvi,
Thank you for helping me with this! I was looking for nsMixedContentBlocker.cpp from the very first post (back in 2013) but wasn't able to find it and just making sure it was still an issue. This looks like a fun one, and with the info you provided I think I can get off to a good start this weekend. I think I can get as far as posting a patch on the forums here, but after that I will most likely need some guidance. I only have to deal with a small team of programmers at work, so bugzilla still feels very foreign to me.
Thanks again!!

Comment 8

[Subhasish Kundu]

Hi Tanvi,

  I am new to the entire Bugzilla and code contribution. So I started browsing the list of good first bug. I am interested in this bug. If this bug is still open (as it is shown open in Bugzilla, status is NEW), I'd love to work on it. 

  I am looking forward to learn a lot from contributing to this bug. 

Thanks,
Subhasish

Comment 9

[Tanvi Vyas[:tanvi]]

(In reply to Subhasish Kundu from comment #8)
> Hi Tanvi,
> 
>   I am new to the entire Bugzilla and code contribution. So I started
> browsing the list of good first bug. I am interested in this bug. If this
> bug is still open (as it is shown open in Bugzilla, status is NEW), I'd love
> to work on it. 
> 
>   I am looking forward to learn a lot from contributing to this bug. 
> 
> Thanks,
> Subhasish

Hello Subhasish,

Yes this bug is still open.  We need to create a method that takes a URI and returns whether or not it is secure, similar to the way nsMixedCotnetnBlocker, InsecurePasswordUtils, imgLoader, and imgRequest do today (references in comment 6).  I'm not sure where the best place to put this code is though; if you are interested in contributing, I can ask around and find out where to put this method.

The line numbers referenced in comment 6 are probably off now, but here is the correct line number for mixed content blocker, which will give you an idea what to look for in the other files:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#504

Comment 10

[Subhasish Kundu]

Hi Tanvi,
  Yes I am interested in the bug. I need little guidance. I am checking the file you referred to and trying to accumulated knowledge regarding Mixed Content Blocking.

Thanks,
Subhasish

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to Subhasish Kundu from comment #10)
> Hi Tanvi,
>   Yes I am interested in the bug. I need little guidance. I am checking the
> file you referred to and trying to accumulated knowledge regarding Mixed
> Content Blocking.
> 
> Thanks,
> Subhasish

Hi Subhasish,

This bug is no longer as simple as I originally thought.  We need to see how it fits in with Secure Contexts and bug 1162772.  Hence removing the first good bug and mentor tags.

Comment 12

[Matthew N. [:MattN]]

Since everyone else seems to be afraid of making incremental progress in a shared API, I'm going to simply port+refactor the code from https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?rev=b18e03d64493#1390 into two methods: one taking an nsIURI and one taking an nsIDocument which uses the nsIURI helper.

We can deal with nested and other cases in follow-ups this doesn't need to be perfect for the initial use cases if it defaults to saying things are less secure than reality.

Comment 13

[Matthew N. [:MattN]]

Actually, I think bug 1162772 is more relevant to https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?rev=b18e03d64493#1390 so I will work on that.