354493 - Mitigate CSRF attacks against internal networks (block rfc 1918 local addresses from non-local addresses)

Status: NEW

(Core :: Networking, enhancement, P3)

Description

[Neil Harris]

According to the linked article, JavaScript can be abused to scan internal networks, fingerprint devices, and make malicious commands to those devices if they have a web interface.

What should happen:
* if possible, Firefox should attempt to prevent or mitigate these kinds of attacks

I can think of a partial mitigation technique: perhaps pages served from non-RFC-1918 addresses should be unable to access URLs with RFC 1918 addresses, in much the same way that remote pages cannot load local files? (I can't currently think of any legitimate reason for a page to do this)

Of course, this would only helps users whose LAN uses RFC 1918 addresses, but in my experience, that's most of them.

Comment 1

[Neil Harris]

Just to amplify the above proposal, instead of the existing two tiers for file-loading,

0 Local filesystem files
1 Remote files

there would be three tiers

0 Local filesystem files
1 On-LAN remote files
2 Other remote files

"On-LAN" remote files would be those which load from addresses which are either loopback (127.0.0.0/8) or RFC 1918 addresses, which covers the vast majority on LANs. 

The rules would be the same as the existing system: content from higher tiers could access content from the same or lower tiers, but not from higher tiers. This is in effect a Bell-LaPadula-like "no read up" restriction.

Where a LAN has non-RFC-1918 addresses (eg some large-company networks), this scheme would simply have the same effect as the existing two-level policy. 

Traffic from external sites with RFC 1918 address should _never_ be routable on the public Internet, so this should not be a problem in general practice; even if it happens, the system will be no less secure than the existing policy, and the one failure mode (not being able to load RFC 1918 external content from non-RFC-1918 external content) not only fails safe, but is arguably a desirable RFC-enforcement feature rather than a bug.

NB: The above is a simple refinement of the existing local-file-loading policy to multiple tiers, which already works in conjunction with the other Gecko access control policies, and should _not_ be taken as an attempt to implement a broken Microsoft-like "Internet zones" security model. 

Questions:

* Can anyone think of any scenario where this policy would break existing sane applications?

* What should be done for proxies? Arguably, the best practice would be to consider both the IP address of the proxy and the IP address of the external resource being accessed into consideration, and use the _lower_ of the two access levels for that resource. 

* What about protocols that do not use IP addresses? Should each one have its own default privilege level for this scheme? What currently happens in this case?

* Should any of this be user tunable, either in the preference UI, or in the config file? Could the proxy config information be used to help this policy?

Comment 2

[Neil Harris]

Thinking about Bell-LaPadula some more, this might also be useful to enforce a "no write down" policy as well, were there to be any appropriate cases for this (possibly AJAX?)

Comment 3

[Neil Harris]

One last thing, and then I'll stop replying to myself...

RFC 3927 ("zeroconf"/"rendezvous"/"bonjour" etc..) IP addresses (in the range 169.254/16) should also be regarded as On-LAN addresses, as should IPv6 site-local and link-local addresses... and any others I haven't thought of yet.

Comment 4

[Neil Harris]

Note: I know very little about the details of the Gecko content-loading code, but it looks to me like nsIContentPolicy is the calling interface for the code where the existing policy is implemented

Comment 5

[Neil Harris]

It looks like this vulnerability is now actively being exploited: see

http://www.schneier.com/blog/archives/2007/02/driveby_pharmin.html

http://www.computerworld.com/action/article.do?command=viewArticleBasic&articleId=9011588

and 

http://www.symantec.com/avcenter/reference/Driveby_Pharming.pdf

for an analysis.

for a paper on the technical details.

Comment 6

[Neil Harris]

Note that the DNS redirection attack mentioned in the references above allows a very wide range of attacks later on, which could include (for example), man-in-the-middle attacks on user data and passwords, and the execution of arbitrary code the next time the user downloads software from apparently trusted sources.

Marking as critical, because getting your computer 0wned is certainly a data-loss risk at the very least.

Given the exploit information in the comment above, can someone please also mark this as CONFIRMED?

Comment 7

[alexanderz]

With all the talk I am hearing about drive by pharming , I thought I wouldn't wait for Mozilla to protect against it so I added a feature to my extension to prevent it.

Also, I couldn't resist spamming bugzilla with a link to it, so here it is: https://addons.mozilla.org/en-US/firefox/addon/5544

Currently it blocks internet sites from accessing intranet sites, but it still allows intranet sites to access internet sites. You can use it to see if it breaks any sites. It has not caused any problems for me, the only issues that have been reported are applications that run servers on localhost to allow websites to open them not opening from web links. That might actually be a good thing though since automatically launching applications from sites shouldn't be happening anyway.

Comment 8

[Samuel Sidler (old account; do not CC)]

Doug said he'd look at this.

Comment 9

[Samuel Sidler (old account; do not CC)]

I'm re-requesting blocking on this bug based on bug 411569.

In bug 411569 and in other discussions, we decided that it's better for us to restrict "external" content's ability to initiate request in the rfc 1918 space than to block specific ports, like port 9100.

This bug will stop real problems, not just theoretical ones. See: http://jeremiahgrossman.blogspot.com/2008/01/intranet-hacking-attacks-found-in-wild.html

Comment 10

[Neil Harris]

Here's a first hack at a list of address ranges classified by scope:

IPv4

127.0.0.0/8    host-local
169.254/16     link-local (RFC 3927: "zeroconf"/"rendezvous"/"bonjour" etc..)
10.0.0.0/8     private addressing (RFC 1918)
172.16.0.0/12  private addressing (RFC 1918)
192.168.0.0/16 private addressing (RFC 1918)

IPv6

::1/128        host-local
fe80::/64      link-local
fc00::/7       local communications, intended to be unroutable (RFC 4193)

Question: What to do with things like IPv4-mapped/IPv4 compatible addresses, and multicast addresses for both v4 and v6?

Comment 11

[Bob Clary [:bc] (inactive)]

Read this <http://www.0x000000.com/?i=524> and please reconsider whether this blocks 1.9. 

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

I presume Bob meant to renominate this bug as per comment 11, doing so on his behalf.

For context, as one of the drivers in the triage meeting where we de-blocked the release on this, we did so based on the [sg:moderate vector] rating in the whiteboard and the fact that there was no clear indication that this was a significant security want. If that information is incorrect, of course the bug should be reconsidered. Brandon?

Also: is this in the right component? Should it properly be in Javascript?

Doug: you the current owner, what's the estimate on time required to fix? More of a policy issue?

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Just a note to whomever will implement this filter: This block would have to occur *after* name resolution, since plenty of DNS names will resolve to local IP addresses, both in home and corporate environments.  This is probably obvious, but it doesn't hurt to mention.

Another fun side-issue: there are ISPs in the world that hand out 10.* addresses to their DSL/cable/etc customers.  (I've seen some directly a couple of years ago in Latin America.)  Yes, this is weird and a pain.

Comment 14

[Mike Shaver (:shaver emeritus)]

The dominant case of internal networks with interesting things to scan is on corporate networks, I believe, and I further believe that many such corporate networks employ explicit proxies, possibly via WPAD.  Given that in the presence of a proxy we usually do not do DNS resolution ourselves, I'm not sure how we can tell that we're hitting this case.  Doug may well have an idea, or alexanderz's extension might solve this, but I've been thinking about it since Black Hat and haven't really come up with anything other than "make sure your proxy config doesn't proxy for RFC1918" and then assuming that anything we hit the proxy for is outside 1918.  Seems pretty fragile!

rsnake and his co-presenter at Black Hat said this would be a 2-line fix, perhaps we should ask him for guidance on what the two lines should be. :)

Comment 15

[Robert Sayre]

I'm going to minus this, since it is not a regression, and there are many other security enhancements in Fx3. If we think this is more serious than sg:moderate, please renominate. 

It is difficult to make blocking decisions based on claims of severity from security researchers, since those claims are often sensationalized.

Comment 16

[Neil Harris]

See the recent Router Hacking Challenge for some real-world attacks on mass-market routers via their web-based user interfaces, many of which can be launched remotely via a web page load, and some of which do not even require Javascript to execute. Many of these attacks require no authentication at all. 

Some of the attacks include:
* authentication bypass
* cross-site request forgery
* cross-site scripting
* obfuscation/encryption deficiencies
* SNMP injection attacks due to poor SNMP creds.
* memory overwrites
* stealing config files
* factory restore attacks

Comment 17

[Mike Shaver (:shaver emeritus)]

(In reply to comment #16)
> some of which do not even require Javascript to execute

Are you indicating that we should change your bug summary here, since it's about JavaScript and scanning, to be broader and include <img> and <script> references, and <form> submissions?

Comment 18

[Bob Clary [:bc] (inactive)]

Neil outlines some of the attacks described in the Router Hacking Challenge which are disturbing enough but didn't mention the changing of the DNS settings for the home user's router. 

Home routers are notoriously misconfigured by their users, they do not set passwords, firmware fixes are not always available and even if firmware fixes are available they don't update the firmware. Leaving this avenue of attack against them is wrong.

I think the summary should be changed and the focus of the bug to prevent content on external networks from accessing any internal networks. Therefore I am changing the summary from JavaScript to Firefox.

I am renominating and ask that the full triage team make a decision on this.

Comment 19

[Neil Harris]

As per comment #16, changing edit summary to something that I think is more representative of the real threat, since this does seem to make all of those vectors (img, script, form, ... also iframe and others) feasible as ways of launching attacks.

Comment 20

[Robert Sayre]

Another question is: what does this have to with the release schedule? We take security fixes on branches.

Comment 21

[Neil Harris]

regarding comment #14, "make sure your proxy config doesn't proxy for RFC1918"
is good advice anyway. 

I can't see anything that could be done to fix this problem on the browser
alone for those users on a network that is both paranoid enough to use a proxy,
but not paranoid enough to avoid proxy-for-local or to filter infrastructure
web access; but it should be possible to help the vast majority of NAT-style
users (both corporate and domestic -- most small compnies just use NAT), as
well as to further reduce exposure in well-set-up corporate environments.

Comment 22

[Giorgio Maone [:ma1]]

Since we're renaming this bug, its title shouldn't suggest this is a Firefox specific problem, because this is just a consequence of how every browser handles cross-site requests by default (beside special cases like XHR).
Also, even if I'd definitely like to see it blocking 1.9, I don't believe severity should be tagged "critical": this is technically not even a bug, but a request for enhancement.

Side note: alexanderz's extension does not effectively work-around this issue, since it just checks URI's host name if it appears to be numeric but, as far as I can see, doesn't even try to resolve domain names.
LocalRodeo may be a better candidate since it performs DNS checks, but I suspect its extreme slowness is due to synchronous lookups perfomed in the UI thread. 
Therefore IMHO best hopes to fix this in efficiently pass through hacking nsSocketTransport or one of its clients/listeners, checking addresses after their resolution (when applicable, see the proxy caveat).

Comment 23

[Samuel Sidler (old account; do not CC)]

(In reply to comment #20)
> Another question is: what does this have to with the release schedule? We take
> security fixes on branches.

Doug and I talked about this (I can't remember if dveditz and bsterne were there, but I believe so). This is a fairly large change and we don't tend to take large security fixes on branches opting to fix them in the next major version to preserve compatibility, minimize risk, etc.

Comment 24

[Johnny Stenback (:jst)]

(In reply to comment #23)
> Doug and I talked about this (I can't remember if dveditz and bsterne were
> there, but I believe so). This is a fairly large change and we don't tend to
> take large security fixes on branches opting to fix them in the next major
> version to preserve compatibility, minimize risk, etc.

That all depends entirely on the severity of the security problem, if it's found and made public and it's bad enough and we have no major release right around the corner, we'll fix it on any and all branches no matter what the risk. And yes, we've done things like this before, ca 1.0.0.4 etc come to mind...

Comment 25

[Samuel Sidler (old account; do not CC)]

Right, which is why I said "don't tend to". There are exceptions to everything and each case is looked at individually. Specifically with this enhancement, we know about it before release and we know what attacks are possible. It's a large change and any amount of testing we can get in before release would be a Good Thing, imo.

Comment 26

[Johnny Stenback (:jst)]

Agreed.

Comment 27

[Robert Sayre]

(In reply to comment #25)
>
> Good Thing, imo.

you are not describing a blocker, imho

Comment 28

[Samuel Sidler (old account; do not CC)]

(In reply to comment #27)
> you are not describing a blocker, imho

Please don't make pointless comments in bugs. You've already said you don't believe this is a blocker above. There's no need to reiterate that. I was answering your question and responding to Johnny, to which he agreed. This was re-nominated by Bob in comment 18. If you feel strongly enough that this shouldn't be a blocker, please take your comments to the full triage team.

https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Comment 29

[Robert Sayre]

(In reply to comment #28)
> https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

You mean like renominating everything that gets minused?

Comment 30

[Brandon Sterne (:bsterne)]

My two cents are that blocking 1.9 is appropriate for this bug given that:
1) the severity of the attacks that are possible without this fix are high
impact (enough examples have been cited above), and
2) getting a fix in for Fx 3 will be a huge PR win for Mozilla; we can mitigate
an entire class of attacks which has received a lot of attention over the past
two years among WebAppSec researchers

Comment 31

[Bob Clary [:bc] (inactive)]

(In reply to comment #29)
> (In reply to comment #28)
> > https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
> 
> You mean like renominating everything that gets minused?
> 

<quote>
I'm going to minus this, since it is not a regression, and there are many other
security enhancements in Fx3. If we think this is more serious than
sg:moderate, please renominate. 
</quote>

Comment 32

[Robert Sayre]

(In reply to comment #31)
> 
> <quote>

Still sg:moderate. And, last I checked, PR and security are two separate departments. But maybe I'm hopelessly naive.

Comment 33

[Mike Shaver (:shaver emeritus)]

(In reply to comment #30)
> My two cents are that blocking 1.9 is appropriate for this bug given that:
> 1) the severity of the attacks that are possible without this fix are high
> impact (enough examples have been cited above), and
> 2) getting a fix in for Fx 3 will be a huge PR win for Mozilla; we can mitigate
> an entire class of attacks which has received a lot of attention over the past
> two years among WebAppSec researchers

I don't know how you can recommend blocking only on the basis of reward, without any discussion of risk (app incompatibility, bug habitat, schedule impact).  Bugzilla has thousands of bugs that "would be great" to have fixed, and each one of them has a list of wonderful characteristics.

If you want to help get this bug fixed sooner, I recommend writing a test suite that can be used to tell when the bug is actually fixed, and that it stays fixed.  Advocacy isn't even a tenth as useful as that.  (If you think it's more than an sg:moderate issue, then advocate against the severity criteria for security bugs, I guess, but as an sg:moderate feature request I don't think anyone is likely to say "we should prevent the release release of Firefox 3 until this is fixed" without very well-scoped risk.)

Without a proof of concept patch or extension that actually does what this bug wants, we can't even begin to tell what sorts of intranet apps or appliances will break because they load part of their interface through a name and part through an IP, therefore splitting the requests across a proxy.

Comment 34

[Doug Turner (:dougt)]

I share shaver's concern here regarding what will break.  The assumption was I were working on was that pages that are loaded from "public ip space" should never be able to access "private ip space".  I am, nor the people I spoke with, are sure if this is in fact always true.

I did do some work here.  I basically added new functionality on the channels that allows the consumer to check and set if a connection to a private IP is allowed.  A getter would determine if all of the IPs in the DNS records that were used during the load were public.  Also, there is a setter that prevents any content loads from a private IP.  

The idea would be that "someone" above necko would be able to know if nat loads were permissible.  Hooking this up to that "someone" has turned out to be difficult.  Jonas and I spoke about this for a bit, and believe the only way that this can work is if we used the loadgroup of the page.  However, I am  not sure if that will work in all cases, nor am I sure what will break.

Comment 35

[Doug Turner (:dougt)]

Attachment: very much a work in progress.

this adds a new interface nsINATInfo.  You can set or get information from a http channel (and we would have to extend this to other channels).

The idea here is that from the nsSecureBrowserUIImpl, we would be able to watch requests go by, and for any public IP load, we would prevent the subsequent loads to be able to touch private IP space by setting a flag on the nat info.

I would love some help hooking this up to the nsSecureBrowserUIImpl (if we agree it is the right place to manage this logic) and am open to comment on the patch (be kind, this is a work in progress and in no shape yet to be something considered for review)

Comment 36

[Reed Loden [:reed]]

Recommended reading:

RFC1700 - http://www.ietf.org/rfc/rfc1700.txt (page 4/5)
RFC1918 - http://www.ietf.org/rfc/rfc1918.txt
RFC3330 - http://www.ietf.org/rfc/rfc3330.txt (combination of the above and more)
RFC3927 - http://www.ietf.org/rfc/rfc3927.txt
http://www.iana.org/assignments/ipv4-address-space
http://www.cidr-report.org/as2.0/reserved-ipv4.html
http://en.wikipedia.org/wiki/IPv4#Private_networks

If you're going to block IPv4 reserved addresses, what about IPv6? The future is coming!

RFC3513 - http://www.ietf.org/rfc/rfc3513.txt
RFC4193 - http://www.ietf.org/rfc/rfc4193.txt (FC00::/7)
http://www.iana.org/assignments/ipv6-address-space
http://www.cidr-report.org/as2.0/reserved-ipv6.html
http://en.wikipedia.org/wiki/IPv6#Special_addresses

Comment 37

[Doug Turner (:dougt)]

thanks for the links.

we would do the same for ipv6.  my patch ignored this, but has a spot for the check.

Comment 38

[Neil Harris]

Regarding comment 36: see comment #10 above, which I hope catches all the cases that currently matter (perhaps 0.0.0.0/8 should be added, but I don't believe this is in any practical use). 

I don't think we should be blocking "reserved" addresses at all; the distinction for this case should be "local" addresses vs. all other addresses. "Reserved" generally means "we don't yet know what this will be used for", not "please hard-code your software to make these permanently unusuable."

Comment 39

[Neil Harris]

In particular, RFC 3879 implies that we SHOULD NOT be blocking FEC0::/10 addresses, even though their use _as site-local addresses_ is deprecated -- it states that 

   "The
   special behavior of this prefix MUST no longer be supported in new
   implementations.  The prefix MUST NOT be reassigned for other use
   except by a future IETF standards action.  Future versions of the
   addressing architecture [RFC3513] will include this information.

   However, router implementations SHOULD be configured to prevent
   routing of this prefix by default."

which I take to mean that we should not treat these addresses specially in any way.

Comment 40

[Reed Loden [:reed]]

(In reply to comment #39)
> In particular, RFC 3879 implies that we SHOULD NOT be blocking FEC0::/10
> addresses, even though their use _as site-local addresses_ is deprecated

Correct, but we should block FC00::/7, which replaced FEC0::/10 for unique local unicast specifically (instead of being the huge site-local scoped prefix that FEC0::/10 was, which caused problems).

Comment 41

[Neil Harris]

Reed, see comment #10. I'm going to upload it as a file so that others can edit it, rather than repeating the list over and over in the comments.

Comment 42

[Reed Loden [:reed]]

(In reply to comment #41)
> Reed, see comment #10. I'm going to upload it as a file so that others can edit
> it, rather than repeating the list over and over in the comments.

File uploads are not editable, so that's not going to work. I didn't mean to repeat the list, just point to documentation specifically about these addresses in order to make sure we only block the ones that we _should_ be blocking.

Comment 43

[Neil Harris]

Attachment: Proposed list of addresses to be regarded as "local" for the purposes of this bug

Address list added to attachments list, to avoid having to repeat it over and over in comments.

Comment 44

[Neil Harris]

Attachment: Proposed list of addresses to be regarded as "local" for the purposes of this bug, v2

added 255.255.255.255, and more detailed rationale as to narrow scope of this list; this is not a "bad addresses" list

Comment 45

[Neil Harris]

Based on Mike's comment #33, I've devoted some more thought about what this might break or not break, and there are quite a number of edge cases, all of which can be dealt with by adding an extra "undefined" content category which effectively circumvents these checks.

Thus, there should be three categories:
* content known to be "local"
* content known to be "non-local"
* other content, which should be regarded as "unknown"

Unless I've missed something, this means that the protections provided by this mechanism can't be as absolute as I might have hoped, unless someone can think of a better way to define "local" and "non-local" for things like URLs delivered via other apps.

Nevertheless, I think implementing this bug would still greatly reduce the size of the attack surface, and deal almost perfectly with the single commonest case  of users browsing around the Internet, either directly or through a competently-administered proxy.

--------------------------

Proposed access control rules:
* anything can access anything else, with one exception: known "non-local" content CANNOT access known "local" content

Proposed logic for categorizing content:

Easy cases:
* loaded via filesystem: "local"
* loaded via direct HTTP/FTP/HTTPS: "local" if DNS lookup was in address list above, "non-local" otherwise

Tricky cases:

Proxy content is problematic because proxied domain names cannot be guaranteed to resolve to any particular IP address. In particular, a corporate network may arrange for some addresses accessed via their proxy to access local servers when "inside" when they would otherwise resolve to "outside" servers on the public Internet. Similarly, and unfortunately, there's nothing to stop hostile external DNS operator from making their servers resolve to "internal" addresses. Rather than risk breaking things, it may be easier to regard proxied content as "undefined" and not subject to these rules, and hope that proxy operators know what they are doing. Users using proxies operated by hostile parties are not helped by this; but they are in already in serious trouble anyway.
 
* E-mail bodies: should probably be "undefined", otherwise, links to intranet sites in internal E-mail might not work. Unfortunately, this also provides a way to circumvent the intended protections.  This is, however, not a Firefox problem, although it is a problem for other Gecko applications like Thunderbird; however, unlike with web pages where even careful users will surf the web at random, the careful user is less likely to click on unsolicited links from E-mails.

* Internally-programatically-generated content: should probably be "local"

* URLs specified on firefox command line, or via other external APIs: "undefined"

Comment 46

[Neil Harris]

Another tricky case: HTTP redirects (an access to a "non-local" location HTTP-redirecting to a "local" location)

All this makes me think that something like a Microsoft-like "trusted zone" scheme for certain domain names might help this work better in some cases: but implementing this would be a nightmare from a usability vs. security point of view unless the UI implications were very carefully thought through. However, the new user interface for failed security certificates might be appropriate co-opted into working for this. Something like: 

 "Warning: an external web page is attempting to access 
  a resource on your local network. If you are sure you want it
  to be able to do this, please [etc. etc.]"

As ever: more thought is required.

All of this makes me certain this cannot be a blocker for FF3. 

Comment 47

[Mike Shaver (:shaver emeritus)]

Enhancement request: [*]
Research project: [*]
Unknown impact on compatibility: [*]
Not even a proof of concept patch: [*]
No test suite for verifying behaviour: [*]

Gotta be a minus.  (If the API to _permit_ such IP-classification blocking is small and safe and has tests, I could see it getting approval so that people can write extensions to prove out the concepts, but much would obviously depend on the patch itself.  If someone wants to go that route, which would be great, I recommend doing it in a new, dependent bug.)

Please do not renominate until at least one of the 5 reasons above has changed materially, or you get dveditz drunk enough to make this sg:critical.

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #46)
> All this makes me think that something like a Microsoft-like "trusted zone"
> scheme for certain domain names might help this work better in some cases: but
> implementing this would be a nightmare from a usability vs. security point of
> view unless the UI implications were very carefully thought through. However,
> the new user interface for failed security certificates might be appropriate

Please make sure that as you think about this more you consult with johnath, as he's got lots of good ideas on security UI.

Comment 49

[Johnathan Nightingale [:johnath]]

(In reply to comment #48)
> (In reply to comment #46)
> > All this makes me think that something like a Microsoft-like "trusted zone"
> > scheme for certain domain names might help this work better in some cases: but
> > implementing this would be a nightmare from a usability vs. security point of
> > view unless the UI implications were very carefully thought through. However,
> > the new user interface for failed security certificates might be appropriate
> 
> Please make sure that as you think about this more you consult with johnath, as
> he's got lots of good ideas on security UI.

Yeah, I don't really want to jump into the UI discussion (if any) until we have a clear idea what a solution would look like technologically.  The idea that a web page can use their browser as a trampoline to attack internal sites involves feats of mental model that I'd really rather not have to force our users through. A solution that worked in the cases discussed above, and which required no more UI than an "I'm an edge case, let it through" pref would make me happiest.

Incidentally, in case anyone's counting votes, I agree in the strongest terms that this is bad news and that a fix with clearly delineated compat implications and a strong test suite would be met with much rejoicing.  I also agree, though, with comment 47 - this isn't a blocker until the risk profile of any potential fix becomes clearer. 

Comment 50

[Neil Harris]

At the risk of generating more questions than answers, here's a start at looking at the matrix of possible scenarios, and cues available, singly or in combination, for a hypothetical security mechanism to use to make decisions:

* is this a location, or a piece of content accessed through a location?
* URL scheme?
* port number?
* known-inside IP address?
* proxy or non-proxy access?
* known-inside domain name, eg. ".local" or (user-whitelisted) "mycorp.com" or just a bare hostname?
* suspicious bare hostname or inside domain name, eg "my-router", "router.local"
* suspicious bare IP address in URL, particularly if it points to a "local" IP address?
* route by which access is triggered: eg. user click on link, URL bar entry, link from external app, via page content access (<img>, <script>, <iframe>, CSS...), programmatic from software itself...

[Note: "outside" domains can easily be made to resolve into "inside" addresses.]

There are clearly far more cases here than I had imagined earlier.

Comment 51

[Doug Turner (:dougt)]

Attachment: wip. adds a DNS API to ask "is this a private ip"

can anyone tell me if there are other IP address that we need to consider in ipv4.  also, would love a ipv6 code snippet.

Comment 52

[Doug Turner (:dougt)]

Attachment: wip. component

If you apply the patch, then drop this component into your dist/bin/components directory you should have a proof of concept.

So, this uses the callback http uses so that developers can modify headers before the write hits the network.  Cool thing about this callback is that at this point, our DNS cache already has the resulting lookup records.

I am also not sure if we can simply depend on the window's location to determine if the document is loaded by that url.  Dveditz?

Comment 53

[Doug Turner (:dougt)]

if it isn't perfectly clear, this is a work in progress.

Comment 54

[Giorgio Maone [:ma1]]

Sounds like Opera has got something like this now:

"Opera now distinguishes between local servers on localhost, intranet servers, and remote servers on the Internet. Local servers can use remote resources, but not vice versa."

http://www.opera.com/docs/changelogs/windows/950b2/

Comment 55

[Brandon Sterne (:bsterne)]

I looked at Doug's patches and the approach makes sense to me:

1) adds API to the DNS resolution object indicating whether or not it contains a private address
2) component adds a listener on the channel which, for each request, looks at its DNS resolution object and checks if it contains a private address
3) component cancels requests if the DNS resolution contained a private IP but the top-level location was public.

This approach also deals with requests initiated by redirects, per comment 46.  I like it.

Comment 56

[Mike Shaver (:shaver emeritus)]

How does the resolution aspect work if we're using a proxy?  I don't believe that we resolve addresses ourselves in most of those cases (which is important for privacy in some use cases that very much want to be protected from this attack).  Should we also restrict proxied pages from accessing non-proxied pages?

Comment 57

[Doug Turner (:dougt)]

right, this patch does not handle proxies in any way.  In fact, what I am doing will break most proxies (we are directly resolving hostnames without regard for the proxies being set!).  I am pretty sure we can fix things up so that if the proxy is outside the NAT, we can do the right thing to prevent public pages from accessing private content.  

However, I am not sure what we can do much if the proxy is inside the NAT.  Nor do I know much about what to do in the SOCKs case. Or how SSL changes the picture with HTTP.  More thoughts here are needed.


In case you are interested, there are other major issues with this patch including:

1) frame sets, and iframes aren't working.  I think I am just grabbing the wrong document
2) numberic IPs might not be handled correctly
3) ipv6 support isn't implemented.

Also, the approach I took using the "http-modify-request" is not ideal. It seems to be the only place where there is an existing notification from the network stack where DNS resolution has already taken place.  I looked at using the nsIWebProgressListener, but non of the methods had the right timing.  I also looked at using the load groups but I found that not all loads went into the right load group (image requests, iirc).  Kaie has some ideas about architecture changes in netwerk that would enable this and other security features to be allowed.  I was looking for something that didnt require large sweeping changes, that was low risk, and that was something that possibly soon.  I guess that is my justification for using this notification.

Comment 58

[Doug Turner (:dougt)]

Attachment: wip 2. component. fixes the frameset issue

Comment 59

[Doug Turner (:dougt)]

Attachment: wip 3. component. cleaner, more fixes.

cleans up code a bit, fixes pages transitions (going from a public page to a private page using the url bar), catches css loads.

Things to fix:
Favicons do not set a referrer or have document uri set on the http channel
Proxies -- if a proxy is set, we bail out of these checks

On some of this, I feel that I am the blind following the blind.  For example, I am not sure if this bit of code is right (the aSubject is a nsIHttpChannel):

var parent = aSubject.notificationCallbacks.getInterface( Ci.nsIDocShellTreeItem ).sameTypeRootTreeItem;
var toplevel_url  = parent.QueryInterface( Ci.nsIWebNavigation ).currentURI;

Or should I just use the nsIHttpChannelInternal's documentURI and fall back to the channel's referrer when there is a documentURI (as w/ css loading)?

Comment 60

[Doug Turner (:dougt)]

builds the dns patch are here:
https://build.mozilla.org/tryserver-builds/2008-05-02_16:[e-mail]-dns_private_method/

you can use these and then copy the component into <bin dir>/components/ to check out what this does.

Comment 61

[Neil Harris]

Attachment: Proposed list of addresses to be regarded as "local" for the purposes of this bug, v3

I've now added a note about processing IPv4-mapped IPv6 addresses to the address range list, since these could otherwise provide a way of "smuggling" local IPv4 addresses through the checking logic.

Comment 62

[Neil Harris]

And here's another possible loophole for accessing local resources: according to RFC 4942, "IPv6 Security Overview",

   One particular class of anycast addresses that should be given
   special attention is the set of Subnet-Router anycast addresses
   defined in "IP Version 6 Addressing Architecture" [RFC4291].  All
   routers are required to support these addresses for all subnets for
   which they have interfaces.  For most subnets using global unicast
   addresses, filtering anycast requests to these addresses can be
   achieved by dropping packets with the lower 64 bits (the Interface
   Identifier) set to all zeros.

Since this attack is specifically aimed at routers, presumably these anycast addresses are a possible attack vector. Does this imply that we should regard all IPv6 addresses with a zero interface identifier as "local", or are things more complex than this?

Comment 63

[Neil Harris]

...and there appears to be yet another family of IPv6 anycast addresses, defined by RFC 2526, again distinguished by special values in the Interface Identifier field. Might these addresses also provide a way to accessing local resources via anycast?

Comment 64

[Doug Turner (:dougt)]

thanks neil!  You want to code this up for the DNS patch?

Comment 66

[Doug Turner (:dougt)]

Attachment: wip 4. no requirement of dns changes.

This patch does the dns check in js allowing us to deploy this to even ff2 users.  it is still missing a ipv6 check.

Comment 67

[Brandon Sterne (:bsterne)]

Attachment: wip 5. dougt's component plus start of IPv6.

Comment 68

[Brandon Sterne (:bsterne)]

Attachment: wip 6. dougt's component plus correct start of IPv6.

Doh.  Uploaded wrong patch.  Here's the patch with correct handling of Link-Local unicast.

Comment 69

[Doug Turner (:dougt)]

Brandon, i am not sure this patch protects everything it needs to:

1) document.location
2) link click (to a lesser degree)

Comment 70

[Mike Shaver (:shaver emeritus)]

Yes, that's why this bug needs tests before it goes much further, IMO.  With tests we can see clearly what cases are covered, and discuss what the appropriate results are. 

Comment 71

[Doug Turner (:dougt)]

yup, building unit tests would help a bunch.  We would need at least:

frames
favicons
javascript src= loads
images src= loads
css src= loads
document.location changes
user clicking a link

Comment 72

[Neil Harris]

Presumably XMLHTTP loads are sufficiently by the same-origin policy, but there still might be a possibility of things like (for example) DNS rebinding attacks, even though DNS caching attempts to mitigate this? 

It probably won't hurt to filter these as well, on the belt-and-braces/defense-in-depth principle, since I can't think of a reasonable case in which a valid same-origin load should not also be a valid load under this policy. 

Comment 73

[Dan Mosedale (:dmosedale, :dmose)]

Tests that tickle the nsIExternalProtocolHandler's uses of both local and web apps would be nice too.

Comment 74

[Brandon Sterne (:bsterne)]

(In reply to comment #69)
> Brandon, i am not sure this patch protects everything it needs to:
> 
> 1) document.location
> 2) link click (to a lesser degree)

I agree that these cases should be accounted for in the patch, but I'm not the best person to add such code or to write the test that have been requested in other comments.  Perhaps this bug should be assigned to someone with more expertise in these areas.  Doug, do you have any time to take this bug back?

Comment 75

[Doug Turner (:dougt)]

what mike said, we need to setup test cases (more than people.mozilla.org/~dougt/nat_fun.html).  Was hoping you can help.

Comment 77

[Brandon Sterne (:bsterne)]

Attachment: WIP patch - all current tests passing

Comment 78

[Brandon Sterne (:bsterne)]

Attachment: tests for NAT policy

Comment 79

[Brandon Sterne (:bsterne)]

I posted a series of tests which all pass with the current WIP component (plus a small hack to caps/idl/nsIPrincipal.idl):
http://hackmill.com/nat/test-harness.html

The tests require access to a server which is on the Mozilla intranet.  For those without access to the Mozilla intranet, you can download the tests which unzip into two folders: one which needs to be placed on a "public" web server and the other to be placed on a "private" server.

Boris, I would be grateful if you would a look at the current WIP patch and letting me know if this is an approach that makes sense.

Comment 80

[Boris Zbarsky [:bzbarsky]]

Hmm.  So what policy are we trying to enforce here?  Is it documented somewhere?  I'm not sure why we're looking at the toplevel docshell instead of the one where the load actually originates.

Given the synchronous resolve() calls, I'm also not sure why this is http-specific instead of being done in CheckLoadURI or equivalent...  Is comment 57 the answer?  That is, in on-modify-request are we guaranteed that everything in sight has already been DNS-resolved and hence the resolve() calls will not in fact block?  That was the big worry we had with the CheckLoadURI approach.

Note that using CheckLoadURI would incidentally let us skip the check for things like url bar loads (which don't CheckLoadURI, since they have trusted origin).

I would be fine with making nsIPrincipal::URI scriptable now that it never returns a mutable reference to the principal's internal data structures.

How soon and where are we trying to land this?  I would really like (in the multi-process world) to make all the CheckLoadURI stuff async from the caller's perspective, at which point we could just delay it until we have the DNS resolution results.  But if we're trying to get something working really soon, I'm ok with the on-modify-request approach, I guess, and trying to figure out where our load "really" comes from....  That's a really hard problem; maybe as hard as the other.

Comment 81

[Giorgio Maone [:ma1]]

(In reply to comment #80)
> Hmm.  So what policy are we trying to enforce here?  Is it documented
> somewhere?  

http://databasement.net/labs/localrodeo/

> I'm not sure why we're looking at the toplevel docshell instead of
> the one where the load actually originates.

It's a bogus approach, in facts.

> Given the synchronous resolve() calls, I'm also not sure why this is
> http-specific instead of being done in CheckLoadURI or equivalent...  Is
> comment 57 the answer?  That is, in on-modify-request are we guaranteed that
> everything in sight has already been DNS-resolved and hence the resolve() calls
> will not in fact block? 

And it will generally block, indeed.
We're still in nsHttpChannel::asyncOpen(), while the actual resolution happens in transport on connect. When we're lucky we hit the cache because the we're recycling an existing connection or this host has been resolved by a different request in the past 3 mins, but generally we block.
It's been one of the most difficult issues to work-around in order to implement ABE ( http://noscript.net/abe ) and I'm not yet completely satisfied of the solution (using a 2nd level cache speculatively synchronized with nsIDNSService and asynchronous DNS resolution on cache misses, which means the policy is enforced during asyncOpen() most of the time, but sometimes a bit later (just before socket-level connnection).


> I would be fine with making nsIPrincipal::URI scriptable now that it never
> returns a mutable reference to the principal's internal data structures.

It would be great! I've got a lot of hacks, both for NoScript's XSS Filters and ABE, to guess the actual load origin in many edge cases where docShell.internalReferrer and documentURI don't make sense.
 
> How soon and where are we trying to land this?  I would really like (in the
> multi-process world) to make all the CheckLoadURI stuff async from the caller's
> perspective, at which point we could just delay it until we have the DNS
> resolution results.

And it would be great as well. As it is (DNS resolutions buried inside transport connection and with no guaranteed callback at the request level, especially when keep-alives are involved) implementing something like this is incredibly difficult: I've been there.

>  But if we're trying to get something working really soon,
> I'm ok with the on-modify-request approach, I guess, and trying to figure out
> where our load "really" comes from....  That's a really hard problem; maybe as
> hard as the other.

They're both really hard (been there for both, I've got more than 1500 lines of ABE code on these issues), and sync DNS resolution just can't stay the way it is.

Comment 82

[Masatoshi Kimura [:emk]]

Comment on attachment 386622 [details]
WIP patch - all current tests passing

> (addr[0] == "172" && addr[1] == "16")  || // private addressing (RFC 1918)
Not only 172.16/16 but also 172.17/16, 172.18/16, ... 172.31/16 (that is, 172.16/12) are private.
> parseInt(ipv6_addr[0], 16) >> 8 == 255 || // Multicast FF00::/8
Why v6 multicast is included while v4 multicast is not?
IMO you should include only multicast addresses whose scope is narrower than global.

Comment 83

[Neil Harris]

Just as more evidence that the threat represented by this bug is real, not theoretical, this Register article documents an apparent DD-WRT zero-day CSRF vulnerability of exactly the sort that this bug would prevent:

http://www.theregister.co.uk/2009/07/21/critical_ddwrt_router_vuln/

Comment 84

[Brandon Sterne (:bsterne)]

(In reply to comment #80)
> Hmm.  So what policy are we trying to enforce here?  Is it documented
> somewhere?

Giorgio linked to the LocalRodeo policy which is fine, but Neal summarizes the policy nicely in comment 1 and expands it in comment 45.  I can put together a policy document if that helps us move forward.

> I'm not sure why we're looking at the toplevel docshell instead of
> the one where the load actually originates.

You and Giorgio agree that this is a very hard problem, but do you have any specific guidance on a better approach to identifying the source of a given load?

> Given the synchronous resolve() calls, I'm also not sure why this is
> http-specific instead of being done in CheckLoadURI or equivalent...  Is
> comment 57 the answer?  That is, in on-modify-request are we guaranteed that
> everything in sight has already been DNS-resolved and hence the resolve() 
> calls will not in fact block?  That was the big worry we had with the 
> CheckLoadURI approach.

Yes, I was hoping to leverage a previously cached DNS resolution, but that was apparently debunked in comment 81.  If there is reduced benefit to going the on-modify-request route, does that make a CheckLoadURI-like solution more appealing?

> Note that using CheckLoadURI would incidentally let us skip the check for
> things like url bar loads (which don't CheckLoadURI, since they have trusted
> origin).

Sounds like another +1 in the CheckLoadURI column.

> How soon and where are we trying to land this?  I would really like (in the
> multi-process world) to make all the CheckLoadURI stuff async from the 
> caller's perspective, at which point we could just delay it until we have the 
> DNS resolution results.  But if we're trying to get something working really 
> soon, I'm ok with the on-modify-request approach, I guess, and trying to 
> figure out where our load "really" comes from....  That's a really hard 
> problem; maybe as hard as the other.

There are no hard deadlines to land this.  We obviously don't have a viable solution yet.  But this did come up again this year at Black Hat and we definitely need to develop a solution that protects the common case of home routers, etc. being attacked.  I'm okay with punting on the proxy issue for now if we can get something out there that protects the non-proxied user.

Does it seem viable to hack CheckLoadURI to account for internal network loads, even without async?  I'm happy to do the legwork if we can reach a consensus on the right design.  Help me, Gecko wizards!

Comment 85

[Boris Zbarsky [:bzbarsky]]

The CheckLoadURI approach is the right approach, modulo this sync DNS resolution thing.  The sync thing is not acceptable, since it'll block the UI thread on the DNS request (which can easily be very slow).  That would likely cause nontrivial performance issues, in addition to user-visible freezes.  Then again, the existing patch should be showing that behavior too, right?

It seems to me that the right approach here is to move CheckLoadURI away from being sync on the main thread (which we need to do anyway for electrolysis), and then do this check in CheckLoadURI.  That was why I asked the scheduling question; this problem becomes much more tractable once we get to the "all network access from parent process" stage of electrolysis.

Comment 86

[Giorgio Maone [:ma1]]

Regarding tracking the correct origin, what ABE is doing right now is "annotating" the aRequestOrigin parameter passed to nsIContentPolicy::shouldLoad(), and using it or the "docshell.internalReferrer" property attached to channels when available, plus some extra tricks for edge cases like redirections (the whole redirection chain gets annotated and checked) or meta refreshes, where the docshell URI is checked.

Of course most of these hacks would be unnecessary if CheckLoadURI() could be leveraged (since the principal passeda as a parametr is the correct origin by definition), assuming that it gets actually called in all the relevant places. 

BTW, I'd appreciate a lot if the new async DNS-aware CheckLoadURI() could trigger some extensibility mechanism (e.g. an observers list) allowed to veto the load for enforcing additional network-aware policies like ABE, or alternatively if the content policy system was refactored to be called from there (less advisable for backward compatibility).

Comment 87

[Justin Samuel]

Attachment: Store an IP address with http channels and use this information to restrict future requests.

I spoke with some folks about moving CheckLoadURI away from being sync on the main thread. It doesn't sound like it's a planned change for electrolysis at this point. Maybe that stage is further out. However, I think there's another solution that avoids needing to directly wait on DNS resolution (or involve CheckLoadURI at all, for that matter) and will be compatible with electrolysis.

Here's the three-step approach I'm working on:

1. When an http channel has already been connected to the remote server, store the remote address in the http channel. This address information will be used in step 2.

2. In http-on-modify-request, before the current channel has been connected, determine the referring http channel. Based on the IP address that the referring http channel communicated with, set a flag on the current channel that indicates which categories of addresses this channel should be allowed to communicate with. This information is used in step 3. For example, if the referring channel communicated with a public address, a flag is set which indicates that the current channel should only be allowed to communicate with public addresses.

3. When the current http channel is about to begin sending request headers (and thus already has a connection to the server), check the remote IP address of the socket against the address restriction flag that was set on the channel in step 2. If the address is forbidden, abort sending the request.

I've implemented this approach in the attached patch. The patch is not intended to be production ready. This is just to get feedback on the approach.

I talked to jduell a bit about this approach a few weeks go. He suggested looking for other places besides nsHttpTransaction to do the work of recording addresses and aborting requests. I haven't looked into that yet.

This is currently passing bsterne's tests at http://hackmill.com/nat/test-harness.html and I have a couple of other tests I need to add to his.

This patch includes a few things I don't believe were implemented in previous patches:
* Allows location bar navigation from public addresses to non-public addresses from the location bar.
* Accounts for http proxies and proxies that do host resolution (e.g. socks4a, socks5 with host resolution). No restriction is done when the referring channel was from such a proxy, but it recognizes the situation and doesn't accidentally block requests.
* Handle channels whose referrers are cached channels. It does this by storing the socket address in the cache metadata so that it will be available for referring channels that are read from cache.

Addresses are broken into three groups: public, private, and local. These are ordered such that public < private < local. That is, channels referred by local addresses aren't restricted, channels referred by private addresses are only forbidden from accessing local addresses, and channels referred by public addresses are restricted to only other public addresses.

These address groups and the corresponding addresses are currently hard coded. An alternate approach is to define private and local addresses in a pref. An additional approach could be to allow more than just a restriction flag to be set on the channel. Instead, the current channel could have set on it a list of addresses/subnets that are allowed or forbidden. This might be helpful for extension authors, but I'm not sure it's really necessary for the primary purpose of addressing this bug. I also don't believe changes to support that would conflict with the core changes required for this approach, so that could be a later enhancement.

Does this look like a reasonable approach, assuming any kinks can be worked out, code is cleaned up, tests are written, etc?

Comment 88

[Giorgio Maone [:ma1]]

(In reply to comment #87)
> Does this look like a reasonable approach?

Even though some other candidate clients of an IP-aware request mechanism may need to make decisions BEFORE the socket connection is made (e.g. privacy blacklists), this looks like a reasonable approach for the bug at hand and CSRF in general.

PLEASE PLEASE pretty PLEASE, send out a "http-before-send" or something like that broadcast notification to interested observers (with the ability of aborting the request if needed), so I can remove lots of hacks from ABE (and other extensions can leverage this change). Thanks.

Comment 89

[Justin Samuel]

Attachment: Notify observers when the channel's socket address is available.

I definitely agree, Giorgio. At the time I just didn't want to get hung up on figuring out how to call the topic observers on the main thread from the socket thread. I believe I have this sorted out now [1].

The updated patch adds an "http-on-connection" topic. The channel at this point has its socket remote address set. This information can be used by the topic observer to call channel.cancel(). The js module from the previous patch has been changed from being an http-on-modify-request observer to be an http-on-connection observer. There is no longer a need to set a flag on the channel indicating which address groups are allowed.

There's one issue I'm aware of that I haven't resolved yet: Requests are not restricted when the user opens the link in a new tab or window (e.g. from the context menu). So, a normal link click on a link with a target of _blank is restricted, but middle clicking the link isn't. If anyone has a good suggestion on how to get the referring channel information in the new tab/window in this case (it's not an opener), that would be welcome.

I'm now going to focus on testing, cleanup, and adding a pref for the address restrictions.

[1] I first tried using the nsIProxyObjectManager to make a sync call to the main thread from nsHttpTransaction::ReadSegments. The sync call happened fine but it seemed that the transaction would sometimes hang afterwards. I didn't figure out the cause of that. Instead, I took the approach of using an async call and then having ReadSegments indicate that the stream would block, leaving it up to the call on the main thread to post an event to cause reading to start again (on the socket thread) after observers are notified.

Comment 90

[Masatoshi Kimura [:emk]]

Comment on attachment 454175 [details] [diff] [review]
Notify observers when the channel's socket address is available.

> +      if (IPV6_IS_ADDR_LOOPBACK(v6Addr)  ||
> +          IPV6_IS_ADDR_LINKLOCAL(v6Addr) ||
> +          IPV6_IS_ADDR_LOCALCOMM(v6Addr))
> +        *aGroup = nsIAddressService::ADDRESS_GROUP_LOCAL;
Should ULA be in the same group as loopback and link local?

Comment 91

[Justin Samuel]

I believe you're right. The ULA ("LOCALCOMM") addresses should be treated as private rather than machine-local. That is, they're intended to be the equivalent of RFC1918 IPv4 addresses. I'll fix this in the next version of the patch.

Comment 92

[Justin Samuel]

Attachment: AddressService that identifies whether an address is local/private/public.

Comment 93

[Justin Samuel]

Attachment: Restrict requests based on channel's address and referring address.

Comment 94

[Justin Samuel]

Attachment: mq patch series

Comment 95

[Justin Samuel]

The previous approach, gathering address information of the channel from within the new http-on-connection topic, actually wasn't compatible with e10s. I had forgotten that since this would not be running in content processes, it wouldn't have access to content docshells and such. However, rather than just put the relevant code back in http-on-modify-request, it was time to reconsider the approach.

The idea I was using previously was to start with just a channel and to drill up through docshells and openers until a referring document channel with a socket address could be found. The code to do this was pretty ugly. Even after many attempts to make the logic clearer, it still wasn't easy to understand. Worse than that, I was still battling with figuring out how to detect certain situations such as the user being on a public-address site and then loading a private-address site through their location bar. The only ways I could detect and allow this had false-positives (matched other types of requests that needed to be restricted). Fundamentally, there seems to be not enough information available with that approach.

jduell suggested that I look for ways to tell the channel its referring address when it is created, thus preventing the need to go digging for it. I now have something working that leverages the docshell keeping track of referring addresses. It doesn't, however, set the referring address on the channel at channel creation time (which may have been part of what jduell was suggesting). Instead, in the current patch:

1) The docshell determines and store its own referring address when a new document is loaded. The docshell's referring address is the address of the docshell's parent/opener.
2) If the load doesn't have a referring address (e.g. it is a location bar load), this will be indicated to the document's http channel.
3) The docshell also stores its referring address in the appropriate nsISHEntry instance so that it's available for history loads, reloads, and other possible cases.
4) In the http channel's AsyncOpen, the channel asks its document's docshell what referring address it should use.
5) e10s only: the child channel (the channel on the content side) sends the referring address to the parent, which the parent receives in HttpChannelParent::RecvAsyncOpen(). The parent stores the referring address on the channel. This is the same as how the other arguments received in RecvAsyncOpen are stored on the channel.
6) Address restrictions are enforced in nsHttpTransaction::ReadSegments (this is the same as with previous versions of the patch). In the common case, when the request is allowed, nothing needs to be run on the main thread. In the uncommon case, if the request is rejected, the channel's Cancel is called async via a proxy on the the main thread.
7) e10s only: HttpChannelParent::OnStartRequest/SendOnStartRequest sends the socket's remote address via a new argument to HttpChannelChild::RecvOnStartRequest. This is how channel addresses make their way over to the child channels in the content process.

As for the part of jduell's suggestion for setting the referring address on the channel when it is created, I'm not sure if there's a reason to do this instead of having the channel ask later on. My concern with trying to do this on all http channel creations is that it would involve touching many more areas of the code (any place a channel is created). If I missed a type of request, that type of request would not be restricted. --- Suggestions on whether this seems necessary are welcome. Maybe there is something I'm overlooking or there's an easier way to do it than I realize.

I've split the latest patches up by functionality (address service is separate from address restrictions). With these patches applied, restrictions appear to be working well both with and without e10s. In addition to general cleanup, writing many more tests is still crucial.

I'll add a third patch to the series for allowing extensions to cancel requests when the socket address is known. As supporting extensions will always result in needing to notify observers on the main thread (even if none exist) and thus will require the initial call to nsHttpTransaction::ReadSegments to return early and wait to be told to run again (by an event posted back to the socket thread after the observers are finished), I think it will need to be pref'd off by default. That is, I don't believe there's a thread-safe way to determine whether observers exist, so there needs to be some other way to decide whether to notify observers and not send the request yet. A pref would solve this. --- Note that I don't believe this back-and-forth has much of a performance hit, but it seems like it would be a waste to always do it when it only matters for the 1% of users who have an extension that needs this.

Comment 96

[Neil Harris]

Forbes has reported an upcoming presentation at Black Hat that may well be relevant to this bug:

http://blogs.forbes.com/firewall/2010/07/13/millions-of-home-routers-vulnerable-to-web-hack/

There's not enough information in the article, but it looks like it would be worth reading the presentation when it's been delivered, to see if it's relevant to this.

Comment 97

[Neil Harris]

Here's another talk from Black Hat, this time demonstrating the use of a browser-based attack on a local router to extract the router's MAC address, and then using that to find the user's **physical location** via Google Streetview's MAC address geolocation lookup facility:

http://www.securityweek.com/hacker-uses-xss-and-google-streetview-data-determine-physical-location

The demo shows how the attacker first uses a mixture of IFRAME and onload() to sniff the make of the user's router, following which it launches a tailored attack on that particular router's user interface.

This is all launched from loading a single web page, with no other user interaction, and should not need to use any browser-specific or host-OS-specific facilities.

Comment 98

[Jason Duell]

The approach described in comment 95 sounds good.  There's no need to pass the referring IP in during channel creation if AsyncOpen works better (I assume via callbacks).  That's fine.

I'm a bit confused about which of the attachments in this bug are part of the fix, and which are ready to review.  Can we mark any attachments that are obsolete as such, and ask for review on the parts the need review?  I see one attachment is an "mq series", but is only 35 bytes and seems to have just two filenames.  Also it's marked as octet/application-stream.   If you've got a series of patches, just attach them separately as "part1", part2, etc. and mark each as a patch.

Looks promising--sorry about the lag time for my comments.

Comment 99

[Justin Samuel]

Jason, no worries about lag time. I've been working on writing tests for this bug, which took me down the path of bug 584155 and bug 585191. Now that those two bugs are under control for the moment, I'm going to work on finishing up what I know still needs to be done for this one (e.g. add a preference to enable/disable the address restrictions).

I'll take your advice on how to post separate patch files. I was essentially trying to post my mercurial queue before, but I didn't realize that bugzilla doesn't keep filenames and that I shouldn't let bugzilla auto-detect file types.

Comment 100

[Justin Samuel]

Attachment: address restriction implementation (patch file 1 of 2)

Comment 101

[Justin Samuel]

Attachment: address restriction tests (patch file 2 of 2)

These are the mochitests and xpcshell tests for attachment 466816 [details] [diff] [review].

Comment 102

[Justin Samuel]

Attachment: address restriction implementation (patch file 1 of 2)

Fixes bad code and compiler warnings I meant to fix before posting the last version of this patch.

Comment 103

[Justin Samuel]

Attachment: address restriction implementation (patch file 1 of 2)

More cleanup: I had changed the pref name without considering the pref observer.

Comment 104

[Justin Samuel]

Attachment: address restriction tests (patch file 2 of 2)

Added a test for the "security.address-restrictions.enable" preference.

Comment 105

[Justin Samuel]

The current patches are divided into implementation and tests. In order to run the tests, the patches from bug 584155 and bug 585191 need to be applied (those are blocking this bug at the moment). The implementation is complete and I feel is ready for review once it can make it through tryserver.

I tried running it through tryserver yesterday but my current patch for bug 585191 didn't get the socks proxy into the build server's test environment correctly (when built locally socksd.js is available and the tests do run). I'm working on fixing that at the moment. I'll update this bug when it has gone through tryserver.

Comment 106

[Justin Samuel]

Jonas asked an awesome question which moves this back to being a WIP: "what about redirects to data URIs?" I missed those. In the current patch, top-level page loads can redirect to data URIs that can load private resources without restriction.

Comment 107

[Brandon Sterne (:bsterne)]

Hey jsamuel, any progress on comment 106?

Comment 108

[Justin Samuel]

Not yet, but I haven't lost sight of it. Unfortunately, I'm going to be mostly out of commission until mid-October. Until then I'm just poking around in spare moments.

Comment 109

[Neil Harris]

I think that for the purpose of this bug data URIs can probably be thought of as being resources that have the same origin as the web page in which they are embedded, just as if they were separate web pages on the same server. 

Can anyone confirm whether or not this also fits the general Mozilla security model for same-origin filtering etc.?

Comment 110

[Neil Harris]

Hmm. On second thoughts, this may be a bit more complex than I thought: comment 109 necessarily implies that different instances of "the same" (in a variety of senses) data: URI may reasonably be considered to different origins at the same time, when they are loaded from different source web pages -- or, alternatively, that a common object representing that object may have more than one valid origin.

Comment 111

[Neil Harris]

I've added a dependency on bug 255107, which to my non-expert eyes looks like it might be the right security architecture bug for data: URIs and same-origin security -- if I'm wrong about this, there should really be a new bug filed just for this case.

Comment 112

[Justin Samuel]

Attachment: address restriction implementation (patch file 1 of 2)

Comment 113

[Justin Samuel]

Attachment: address restriction tests (patch file 2 of 2)

Comment 114

[Justin Samuel]

I've rebased the patches and have added a first attempt at protecting against top-level redirection to data URIs that initiate requests to restricted addresses. The approach used is:
  a) in nsHttpChannel::SetupReplacementChannel, if redirecting to a data URI, store the referring IP address in the new channel's property bag.
  b) in nsDocShell::OnNewURI, if it's a data URI, look for the address in the channel's property bag and set the docshell's |mReferrerAddress| using that value.
  c) in nsDocShell::DoURILoad, modify the logic from the earlier patch so that we're more careful about when the value of |mReferrerAddress| gets cleared. This is to handle both the case of the top-level redirect to a data URI as well as a user reloading a data URI document after being redirected to it.

This implementation leaves open the following bypass: A malicious site redirects to a data URI that makes requests to private addresses and also crashes the browser through an unpatched bug. The user restarts the browser, restores their session, and now the data URI document is allowed to make requests without address restriction. I'm not sure how much of a concern this is to other people, but I feel it's acceptable to allow this case for now.

My concerns with this patch at the moment are:
  1) I haven't tested with e10s and so some changes (or even fundamentally different approaches) will be needed. It looks like m-c hasn't been merged into e10s in a while. I may just wait for the next m-c -> e10s merge.
  2) My impression is that using channels as property bags is frowned upon for maintainability reasons, among others. Adding an actual property to data channels would require creating an interface for data channels (there's currently no nsIDataChannel, just the nsDataChannel implementation). That's probably not a big deal, but I didn't want to go down that path until I'm more confident the general approach is correct.

I've added mochitests for top-level data URI redirects. I've believe I've also fixed the patches for bug 584155 and bug 585191 that caused the previous runs through tryserver to fail.

(In reply to comment #109)
> I think that for the purpose of this bug data URIs can probably be thought of
> as being resources that have the same origin as the web page in which they are
> embedded, just as if they were separate web pages on the same server. 
> 
> Can anyone confirm whether or not this also fits the general Mozilla security
> model for same-origin filtering etc.?

(In reply to comment #111)
> I've added a dependency on bug 255107

I don't think this is directly dependent on bug 255107. The private address cross-site request issues are outside of the same-origin realm. In some sense there's a different concept of an origin (an origin address) and a different concept of what should be restricted (request initiation rather than access to content).

Comment 115

[Matt McCutchen]

(In reply to comment #114)
> This implementation leaves open the following bypass: A malicious site
> redirects to a data URI that makes requests to private addresses and also
> crashes the browser through an unpatched bug. The user restarts the browser,
> restores their session, and now the data URI document is allowed to make
> requests without address restriction.

Alternatively, the user clicks a data: link in some other application and it opens in Firefox.  Are there any desktop environments in which that can happen?

It seems like the solution would be to give data: URLs of unknown provenance the least internal-network privilege rather than the greatest.

Comment 116

[Neil Harris]

(In reply to comment #115)
> Alternatively, the user clicks a data: link in some other application and it
> opens in Firefox.  Are there any desktop environments in which that can happen?
> 
> It seems like the solution would be to give data: URLs of unknown provenance
> the least internal-network privilege rather than the greatest.

I agree. Doing that would be simple, elegant and safe. I don't think any of the scenarios that it would break are likely to be used in practice: coming up with a fix for those cases, if they ever do become important, can be left to another bug.

Comment 117

[Matt McCutchen]

So, the changes in this bug will prevent "public" sites from accessing "private" space directly, but the user might still follow a malicious link to "private" space from another application or from a "private" site that renders untrusted links (e.g., webmail).  I would rather not count on the user to identify malicious links as such in all cases.  A GET request may seem a lesser risk, but if there is any "private" site on which it can produce XSS, you have port scanning again.

I'm not sure what to do about this.  The user's expectation that "private" links work seems to be fundamentally in conflict with the sysadmin's wish that "private" services not be exposed to malicious HTTP requests from outsiders via the user's browser.

Comment 118

[Neil Harris]

(In reply to comment #117)

> I'm not sure what to do about this.  The user's expectation that "private"
> links work seems to be fundamentally in conflict with the sysadmin's wish that
> "private" services not be exposed to malicious HTTP requests from outsiders via
> the user's browser.

Nevertheless, that does not mean that the measures discussed here are not effective at increasing safety; they make things *much* better than before, with a much smaller attack surface remaining compared to the previous situation, where the default was to be absolutely wide open to attacks from all URLs, from anywhere, automatically.

Unfortunately, once the user has decided to click on a link, or type, or cut-and-paste a link, within the local security context, divining user intention is outside the scope of what is possible within the browser. Hence the use of the term "mitigate", not "prevent" in the bug title. 

There are things which could be done to prevent this sort of attack, but they would have to be applied on the other applications and web apps that might let these links into the local security context in the first place. 

(Hypothetical example: within a security-conscious company, DNS resolvers might be set up to refuse to resolve internal addresses from non-internal domain names, and webmail apps might be set up to suppress links using internal addresses or domain names from emails that have not been cryptographically verified to come from within the company. And so on.)

Comment 119

[Jason Duell]

>  I haven't tested with e10s and so some changes (or even fundamentally
>  different approaches) will be needed. 

Well, the nsHttpChannel is in the chrome process under e10s, but the DocShell is in the child process, and while the Docshell will have a pointer to an HttpChannelChild which *looks* a lot like the chrome nsHttpChannel, that does *not* include hashbag properties.  So you'll probably need a way to propagate the info explicitly to the HttpChannelChild, either in one of the existing IPDL messages in PHttpChannel.ipdl (OnStartRequest is the most obvious candidate, provided that's not too late: IP resolution must have happened by then).  Or you could add a new IPDL message if there's some other logical time to send the info.

Haven't looked at the patch enough to know more details, and don't have time right this second (sorry)

> It looks like m-c hasn't been merged into e10s in a while. I may just wait for
> the next m-c -> e10s merge.

The e10s repo is now being used for multi-process Firefox, i.e. it's not the droid you're looking for.   (Sad to think the youngsters these days might think I'm talking about mobile phones).  All the e10s code you care about has landed in m-c, and is maintained and updated there.

Comment 120

[Zack Weinberg (:zwol)]

Some of the infrastructure added by the patch for this bug appears to have substantial overlap with the work being done in bug 526207, I wanted to make folks aware of that.

Is there anything I can do to help get this one done?

Comment 121

[Brandon Sterne (:bsterne)]

Justin, can you give us a sense of what still needs to happen to land this?  Do you have bandwidth to close this out or should we bring someone else in?  We really want to fix this once and for all.

Comment 122

[Matt McCutchen]

(In reply to Brandon Sterne (:bsterne) from comment #121)
> We really want to fix this once and for all.

If only we could.  The present proposal is at best a mitigation, as discussed in comment 117 and comment 118.

Comment 123

[Brandon Sterne (:bsterne)]

(In reply to Matt McCutchen from comment #122)
> If only we could.  The present proposal is at best a mitigation, as
> discussed in comment 117 and comment 118.

I fully understand the scope of this bug and specifically the change that Justin's patch makes.  Nowhere does this bug claim to solve CSRF on internal networks 100%.  Note "Mitigate" is the first word in the bug summary.  Rehashing previously made comments is not productive and this bug's comment history is quite long enough already, thank you.

Comment 124

[Matt McCutchen]

(In reply to Brandon Sterne (:bsterne) from comment #123)
> Nowhere does this bug claim to solve CSRF on internal
> networks 100%.

Except in comment 121, depending on how one interprets the "this" in the last sentence.  I thought that remark could be misleading, so I commented.

Comment 125

[Justin Samuel]

(In reply to Brandon Sterne (:bsterne) from comment #121)
> Justin, can you give us a sense of what still needs to happen to land this? 

The main issues are:
* Bring the patch up to date.
* Make the patch work with content processes. The end of summer 2010 was the last time I attempted this and content processes weren't far enough along for me to do more than proof of concept testing. When I updated this patch in January, I didn't realize all of the e10s code was in m-c, which jduell pointed out in comment 119.
* Get the patches to bug 584155 and bug 585191 landed so that the mochitests and xpcshell tests can run on the build/try servers.

I believe that would be sufficient get the patch landed assuming no fundamental design issues are discovered in review.

Whether this can be enabled by default in its current form is not something I know the answer to. Given that it would spend time in beta, my feeling is that it should start out enabled by default. The major place where breakage is expected for most users is when they resume/restore at a wifi hotspot which redirects them to rfc1918 addresses in order to show the user a wifi hotspot login/TOS page. Not all wifi hotspots use that approach but some definitely do. However, users are probably already used to breakage in this situation (e.g. SSL cert warnings). The other likely places where legitimate requests will get blocked are certain use cases by web developers and possibly some enterprise web applications.

> Do you have bandwidth to close this out or should we bring someone else in?

My only realistic hope of getting this done was over the summer. The summer has now clearly passed. I think the best path forward is for someone besides me to take charge of this. (Based on comment 120, I let zwol know when I saw him a few months ago that he was more than welcome to take over here. My impression at the time was that he also has too much going on. So much for my hope of passing it off to him. :)

Unrelated: does bug 255107 really block this one?

Comment 126

[Josh Aas]

bsterne - should this bug remain assigned to you or should we find a new owner?

Comment 127

[Brandon Sterne (:bsterne)]

It definitely should not be assigned to me, but I'm in the process of trying to find a new owner.  I spoke with zwol last week about either him or one of his colleagues picking up the ball here.  Once I hear definitively if they can own this (Zack, feel free to pipe up here or via email) we'll assign it to the right person.  Thanks for checking in, Josh.

Comment 128

[Neil Harris]

Class E IPv4 network addresses (240.0.0.0/4) are currently reserved. 

For many reasons, these addresses are unlikely ever to be used on the public Internet, as they are blocked on many operating systems and router implmentations, and making sure everywhere on the Internet could use them would be almost as hard as implementing the IPv6 transition, for a gain of only a few more months of life for IPv4.

There have been proposals to allow their allocation for private use only, in a similar way to RFC 1918, with the primary use envisaged being its use for ISP internal networks during the IPv6 transition. See RFC 6319 for more details. 

I'm not sure, however, how these addresses should be considered for the purposes of this bug -- so I'm just adding this as a comment for now, to mark this for future discussion should the class E addresses ever get allocated.

Comment 129

[David Bruant]

The URL redirects to a generic page, not an article.
Can someone change the URL to the article or/and add snippets of JavaScripts (since it seems to be the issue) as attachement to expose the threats that this bug is trying to mitigate.

Comment 130

[Randell Jesup [:jesup] (needinfo me)]

http://www.gnucitizen.org/blog/javascript-port-scanner/
http://blog.andlabs.org/2010/12/port-scanning-with-html5-and-js-recon.html (may be a slightly different issue)
http://www.thespanner.co.uk/2007/07/25/javascript-port-scanner/
http://jeremiahgrossman.blogspot.com/2006/11/browser-port-scanning-without.html
http://web.sec.uni-passau.de/members/martin/docs/070906_HITB_Martin_Johns.pdf

This appears to be the original article (but has less info than many of the above links):
http://docs.rapidlibrary.com/document.php?file=659451&desc=JSportscan+.pdf

Comment 131

[Josh Aas]

Moving to sg:want since this is an enhancement to our security, not a proper bug in our code.

Comment 132

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

As we discussed in the networking team meeting, [sg:want <priority>] is how the security team indicates what priority it thinks a bug should be, and [sg:want] is a flag that is not on the [sg:low]-[sg:critical] rating scale. So, I am restoring the [sg:moderate].

Also, we need a workable design for this feature. When I last talked to Brandon about this, we realized that there were many considerations that were not taken into account--the HTTP cache, how to determine the compatibility impact (how do we avoid breaking the web), how to handle DNS responders that return local AND non-local responses, mixed IPv4 and IPv6 responses, etc.

Further, although this almost definitely needs support from Necko, so that higher layers can ask "is this response from a a local server" and state "only issue this request to a non-local server", some higher layer (docshell?) needs to be responsible for the actual "don't mix local and non-local resources" logic. 

I am throwing this into the Security component now, with the expectation that somebody on secteam will take over Brandon's role on the design work and/or that Brandon will volunteer to work on the design. I am happy to work with secteam on it too. If Core:Security isn't correct, the next logical place would be Core: Document Navigation, AFAICT.

The changes to Necko that will be needed (as determined by the as-yet-undone design) should be filed as a separate bug or separate bugs in Core:Networking.

Comment 133

[Neil Harris]

Just for the sake of completeness, here's advance warning of another address range that looks likely to become "special": http://www.ietf.org/mail-archive/web/ietf-announce/current/msg09959.html

Comment 134

[Masatoshi Kimura [:emk]]

(In reply to Neil Harris from comment #133)
> Just for the sake of completeness, here's advance warning of another address
> range that looks likely to become "special":
> http://www.ietf.org/mail-archive/web/ietf-announce/current/msg09959.html
Where the address block come from while IPv4 addresses exhausted far back?

Comment 135

[Neil Harris]

It all depends on what you mean by "exhausted". The IANA global /8 pool _is_ exhausted, but individual regional registries still have unallocated space, and are hence still able to make allocations like this.

http://whois.arin.net/rest/net/NET-100-64-0-0-1/pft

This allocation will actually slow down IPv4 exhaustion, by allowing reuse of this address space across provider networks. Used correctly, end users should never see these addresses. However, given past history, it is also very likely to be misused for internal networking purposes by end users.

Comment 136

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Pony request:  Over in TestPilot, we got an outside request to help determine the likely impact (the 'breaking the web') part of changes to this policy.  We don't know how to evaluate either the difficulty, or the value of doing so.  Could someone spend a few minutes sometime soon talking through it?  (If this is off-topic and needs a 'new bug', let me know).

Comment 137

[[Baboo]]

You could collect sites that are affected and estimate how many users these have.
For what services that are broken by NoScript ABE's default rule see http://noscript.net/faq#faqsec8 and http://forums.informaction.com/viewforum.php?f=23

Comment 138

[[Baboo]]

Could be marked "parity-opera"?
http://my.opera.com/securitygroup/blog/2012/07/03/operas-cross-network-protection

Comment 139

[Eric Lawrence (@ericlaw)]

Could be marked "parity-IE". 

http://blogs.msdn.com/b/ieinternals/archive/2012/03/23/understanding-ie10-enhanced-protected-mode-network-security-addons-cookies-metro-desktop.aspx

IE10+ uses Windows 8+ AppContainer network isolation to combat this attack.

WhiteHat security's new "Aviator" browser blocks all use of RFC1918 space, including top-level navigations.

Comment 140

[Neil Harris]

Yet more examples of CSRF attacks against personal 3G hotspots, of the sort this bug is intended to prevent, here:

http://www.theregister.co.uk/2014/01/30/3gmodem_security_peril/

Comment 142

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.0 Add and support nsINetworkZonePolicy to protect resources loaded from private IPs

Hi all,

Here is a design and implementation based on offline discussions with members of the Necko team and :bz. Note that we’re not going to catch all the corner cases here, but hopefully this design deals with the questions about DNS records and cached HTTP responses.

First, a clarification: the focus is on protecting embedded documents (i/frames) and sub-resources (images, media, XHRs etc.) which are stored on private networks, as well as HTTP redirects. Top-level navigations are not to be included.

Now, on to the details: This patch sees the introduction of nsINetworkZonePolicy (NZP), a new service which sets and gets a channel’s permission to load resources from private addresses; a new attribute in ns(I)LoadGroup to store the private/public permission at the document load level, and several changes in Necko code (HTTP, Sockets, DNS) to support the nsILoadGroup attribute.

1. Channels, Load groups and NZPService.

nsHttpChannel queries the NZPService to determine if it can load from private addresses. NZPService then gets the channel’s load group and queries the new attribute. It will also check the load group’s parent hierarchy (my term, may be wrong) in this order: parent load groups, owning load groups, and finally the load group of the docshell’s parent docshell. The query is recursive and stops when it reaches the top most load group of the same type, or a load group that does not permit private loads. This should result in the children of a public document being restricted to public loads only. 

The result of this query performed by NZPService is returned to nsHttpChannel where it sets a flag in the new nsHttpConnectionInfo object it has created for this channel.
For new connections, this flag is used to set the nsSocketTransport object, and subsequently the nsDNSRecord.
For existing connections, the flag is used in the nsHttpConnectionInfo hash key, so that existing private and public connections are isolated from each other.

Note: For the nsDNSRecord, if the public-only flag is set, only public IP addresses will be accessible.

Also note: FTP will be something similar, but of course modified for that code. FTP is not covered in this WIP patch.

When we eventually have an nsSocketTransport object, notified in OnTransportStatus, we call upon NZPService again to verify permissions with the new address AND set permissions in the load group if possible. Only document loads will be able to set permissions in the load group via NZPService. And load groups will not be able to override the permissions set by their parent load group. This should result in non-document loads being dependent on what was set by their owning document. Please note that I am not a doc shell expert, so my understanding of load groups and documents may need to be corrected here.

2. HTTP Cache

For the HTTP cache, our goal is to protect entries which were loaded from private IPs. Such entries will have a metadata element set on them, the value of which will be a generation ID that is updated when a network topology change is detected (See bug 939318).
In this way, when a cached entry is loaded and it contains the privately-loaded metadata element, the entry will only be accepted if the ID matches the current generation’s ID, i.e. there has not been a network change event. Otherwise, the entry will be doomed for eviction from the cache.
At present there are no plans to have separate public and private entries in the cache, nor a background mechanism to purge the private entries on network change events. This is to keep this initial solution simple and performant while still aiming to protect the cached entries.

Note: The cache work in the patch is still very basic; very much a WIP. But it should help to give an idea of our thinking.


3. Images and the Image Cache

We will work on tying together image cache entries and HTTP cache entries as a follow-up to this bug. This type of rewrite is preferred to an interim one in which the image cache would have a public/private flag set. As such, the current plan is not to block the rest of the work on this task. This means that images may not work like other sub-resource loads for now; the plan is to work on this right after.


4. Addons' Channels and Sockets:

For now, we want to focus on HTTP and FTP only. It’s possible that we could support non-HTTP or -FTP based protocols (e.g. Gopher) in the future, but this will require a concerted effort, e.g. changes to nsIChannel, or requiring support for a new nsIProtectedChannel. This means that such protocols will still be vulnerable to CSRF, but we should be covering the vast majority of use cases by working on HTTP and FTP.


Thoughts and comments please.

Thanks,
Steve.

Comment 143

[Boris Zbarsky [:bzbarsky]]

I'm not likely to have time to look at this for at least a week.  :(

Comment 144

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #143)
> I'm not likely to have time to look at this for at least a week.  :(

Not a problem, Boris. I know you're very busy. Is there someone else you could suggest? Someone familiar with loadgroups and how they relate to docshells and channels?

Comment 145

[Boris Zbarsky [:bzbarsky]]

Apart from the people you already asked for needinfo, no.

Comment 146

[Steve Workman [:sworkman] (INACTIVE)]

:) Okie doke then. We'll just wait for when you free.

Comment 147

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.1 Add and support nsINetworkZonePolicy to protect resources loaded from private IPs

Patch updates; still WIP.
Switched out :mcmanus for :mayhemer - I'd like Honza's feedback on the cache parts. Pat, you can still chime in, of course.

-- Fixed: Null nsIDNSRecord returned if private addresses disabled, and no public addresses in record.
-- Added: Assertion in nsSocketTransport that nsIDNSRecord has entries if lookup was reported as successful.
-- Added: Some more work on integration with the cache.

Note: Hoping to use :bagder's work in Bug 939318 to ID private networks for cache metadata. Maybe we can keep these private entries in the cache and only load them if the metadata value matches the network checksum... Code not complete for this yet.

Comment 148

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Adding Tanvi as this is somewhat related to the security hooks work.

Comment 149

[Honza Bambas (:mayhemer)]

I think nsINetworkZonePolicy iface is missing from the patch ;)

Comment 150

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.1 Add and support nsINetworkZonePolicy to protect resources loaded from private IPs

Ooops. Yup, NZPService.h/cpp and nsINetworkZonePolicy.idl were omitted in the last patch. Should be added now.

Comment 151

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8426540 [details] [diff] [review]
WIP 1.1 Add and support nsINetworkZonePolicy to protect resources loaded from private IPs

The new API needs some documentation.  It's pretty hard to provide feedback on it otherwise.

Comment 152

[Honza Bambas (:mayhemer)]

Comment on attachment 8425145 [details] [diff] [review]
WIP 1.1 Add and support nsINetworkZonePolicy to protect resources loaded from private IPs

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

This is some first feedback from me...  I'll check the new patch as well now.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2740,5 @@
> +                 this, privateLoadOK ? "allowed" : "to be doomed"));
> +        }
> +        if (!privateLoadOK) {
> +            entry->AsyncDoom(nullptr);
> +            return NS_OK;

set *aResult = ENTRY_NOT_WANTED before this return
why are you dooming the entry?  I don't say it's wrong, but justification and comments are needed.

anyway, with either of these configs we won't have an entry to write to, so dooming is wasting of an existing entry that may survive

@@ +3165,5 @@
> +        // For cached doument loads, we must set Private/Public Network load
> +        // persmissions for the document's sub-resources.
> +        // By this stage, if the entry is private, we will have checked if private loads are ok.
> +        // So, we just need to forbid private loads if the document was cached publicly.
> +        if (!mCacheEntryIsWriteOnly &&

maybe check directly the aNew argument value here, this may be missleadinng

@@ +5535,5 @@
> +
> +            if (peerHasPrivateAddr && !mAllowPrivateIPLoads) {
> +                NS_WARNING("This channel not permitted to load from "
> +                           "private IP address! Canceling...");
> +                return Cancel(NS_ERROR_CONNECTION_REFUSED);

For reused connections you will get here way after the request had been sent out.  Is that OK?  IMO no.  You should carry the mAllowPrivateIPLoads down to the transaction IMO and kill it ASAP it gets a live connection with a private IP.

::: netwerk/protocol/http/nsHttpConnectionInfo.h
@@ +76,5 @@
>      bool          GetPrivate() const     { return mHashKey.CharAt(3) == 'P'; }
> +    bool          PrivateIPLoadsAllowed() const
> +                                         { return mHashKey.CharAt(4) == 'P'; }
> +    void          AllowPrivateIPLoads(bool allow)
> +                                         { mHashKey.SetCharAt(allow ? 'P' : '.', 4); }

hmmm. confusion with Private Browsing...  maybe use lower case p?  or L as local network?

Comment 153

[Honza Bambas (:mayhemer)]

Comment on attachment 8426540 [details] [diff] [review]
WIP 1.1 Add and support nsINetworkZonePolicy to protect resources loaded from private IPs

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

f+ on the cache bit, some other comments added too

::: netwerk/base/public/nsINetworkZonePolicy.idl
@@ +13,5 @@
> +{
> +  bool checkPrivateNetworkPermission(in nsIRequest aRequest);
> +
> +  void setPrivateNetworkPermission(in nsIRequest aRequest,
> +                                   in bool aAllowed);

comments, comments, comments!  even you want just feedback, this must be documented.  btw, writing comments is also a test for you whether the API or a change you are doing makes sense.  indescribable stuff is usually bad...  it has helped me few times to verify my approach is correct very soon.

::: netwerk/base/src/nsNZPService.cpp
@@ +276,5 @@
> +                                   nsIChannel *newChannel,
> +                                   uint32_t flags,
> +                                   nsIAsyncVerifyRedirectCallback *callback)
> +{
> +  // Call CheckPrivateNetworkPermission

do you really need this? the new channel goes through the same check paths (and is added to the same loadgroup), so you probably don't need to check on redirects I think.

::: netwerk/dns/nsDNSService2.cpp
@@ +135,5 @@
>              }
>          }
> +        while (mIter &&
> +               (mHostRecord->Blacklisted(&mIter->mAddress) ||
> +               (mHidePrivateIPAddresses && IsIPAddrPrivate(&mIter->mAddress))));

please put the ; on a new line or add (preferred) an empty { } block.

@@ +322,5 @@
>      nsCOMPtr<nsIDNSRecord> rec;
>      if (NS_SUCCEEDED(status)) {
>          NS_ASSERTION(hostRecord, "no host record");
> +        nsRefPtr<nsDNSRecord> recImpl = new nsDNSRecord(hostRecord);
> +        if (mFlags &= nsIDNSService::RESOLVE_DISABLE_RFC1918) {

&=  ????

@@ +329,5 @@
> +                status = NS_ERROR_UNKNOWN_HOST;
> +                recImpl = nullptr;
> +            }
> +        }
> +        rec = recImpl.forget();

are you sure this works as swap?  rather do rec.swap(recImpl) ?

@@ +858,5 @@
>              rv = syncReq.mStatus;
>          else {
>              NS_ASSERTION(syncReq.mHostRecord, "no host record");
> +            nsRefPtr<nsDNSRecord> rec = new nsDNSRecord(syncReq.mHostRecord);
> +            if (flags &= nsIDNSService::RESOLVE_DISABLE_RFC1918) {

&=  again?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3936,5 @@
> +        LOG(("nsHttpChannel::AddCacheEntryHeaders %p setting loaded-from-"
> +             "private-IP=%s",
> +             this, gHttpHandler->NetworkID()));
> +        rv = entry->SetMetaDataElement("loaded-from-private-IP",
> +                         gHttpHandler->NetworkID());

what if the netid has changed before we get here?  we are pretty async, so it could happen IMO.  best the id should be stored on the connection and you should query the connection for it here.  but this might be too strict and overcomplicated, Daniel may know more

Comment 154

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.2 Add and support nsINetworkZonePolicy to protect resources loaded from private IPs

Added comments; split up patches.

Haven't attended to Honza's initial feedback yet.

Comment 155

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.2 Support nsINetworkZonePolicy in nsILoadGroup

Comment 156

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.2 Support nsINetworkZonePolicy in Necko's HTTP code

No changes in this from WIP 1.1; just splitting up the patches.

Comment 157

[Honza Bambas (:mayhemer)]

If this is identical to WIP 1.1 then you already have my feedback.

Steve, please be on IRC as everyone else, now this bugzilla comment will just spam everyone.

Comment 158

[Honza Bambas (:mayhemer)]

Comment on attachment 8427926 [details] [diff] [review]
WIP 1.2 Support nsINetworkZonePolicy in Necko's HTTP code

Feedback provided in comment 153

Comment 159

[Patrick McManus [:mcmanus]]

Comment on attachment 8427926 [details] [diff] [review]
WIP 1.2 Support nsINetworkZonePolicy in Necko's HTTP code

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

very cool

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +1712,3 @@
>              mDNSRecord = static_cast<nsIDNSRecord *>(param);
> +            nsresult rv = mDNSRecord->GetNextAddr(SocketPort(), &mNetAddr);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv),

I don't think that's right, given the open ended nature of xpcom.

you could change status to a fail code if rv failed..

::: netwerk/dns/nsIDNSService.idl
@@ +140,5 @@
>      const unsigned long RESOLVE_DISABLE_IPV4 = (1 << 7);
> +
> +    /**
> +     * If set, Private (RFC1918) addresses will NOT be returned from resolve/
> +     * asyncResolve.

1918 OR Loopback, right? should probably name it to reflect that

::: netwerk/protocol/http/nsHttpConnectionInfo.h
@@ +82,5 @@
>      void          SetPrivate(bool priv)  { mHashKey.SetCharAt(priv ? 'P' : '.', 3); }
>      bool          GetPrivate() const     { return mHashKey.CharAt(3) == 'P'; }
>  
> +    bool          PrivateIPLoadsAllowed() const
> +                                         { return mHashKey.CharAt(4) == 'P'; }

use a new letter to make reading the string easier :)

Comment 160

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8427922 [details] [diff] [review]
WIP 1.2 Add and support nsINetworkZonePolicy to protect resources loaded from private IPs

The API looks reasonable (though I didn't think too hard about the naming).  setPrivateNetworkPermission should document what happens if it can't set the permission (does it no-op or throw?) and what happens if a non-document request is passed in.

I'm assuming the actual wording of the API comments will get fixed up later.

>+++ b/netwerk/base/src/nsNZPService.cpp

How about NetworkZonePolicyService.cpp?

Or even NetworkZonePolicy.cpp.

>+nsresult
>+NZPService::GetNextLoadGroupAncestor(nsILoadGroup *aLoadGroup,

This method always returns NS_OK.  Just return already_AddRefe<nsILoadGroup> or something?

>+nsresult
>+NZPService::CheckLoadGroupHierarchy(nsILoadGroup *aLoadGroup,

Likewise.

SetPrivateNetworkPermission seems to be broken.  Since it consults the loadgroup whose state we want to change, I don't see how it can ever set the permission to "true".

Comment 161

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8427923 [details] [diff] [review]
WIP 1.2 Support nsINetworkZonePolicy in nsILoadGroup

>+nsresult
>+nsLoadGroup::GetAllowLoadsFromPrivateNetworks(bool *aAllowed)

NS_IMETHODIMP.

And imo no need to null-check aAllowed.

The rest looks fine.

Comment 162

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.2 Add nsINetworkZonePolicy to protect resources loaded from private IPs

Thanks for the feedback :bz. I’ve done some renaming and reviewing of code comments per your comments above - please take a look.
(Note: I merged the nsINetworkZonePolicy and nsLoadGroup patches together since they're quite small).

Comment 163

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.3 Add support for nsINetworkZonePolicy in nsHttpChannel and related code

Pat, Honza, thanks for the feedback. Here are the main changes from the previous version.

HTTP Cache: Set metadata in “loaded-from-private-network” with value = current networkLinkID. This is obtained from nsIOService; currently, it’s derived from the host’s IP address, and is updated when a new connection is established and the host has a different IP address. This is temporary; the goal is to use the checksum generated from multiple network config data points based on work in bug 939318.

Existing connections: nsHttpTransaction checks the peer IP address in ReadSegments. It closes with NS_ERROR_CONNECTION_REFUSED if the peer’s IP address is private but the transaction does not have private connection permission.

New connections are much the same as the previous patch.

Comment 164

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 XPCShell test cases for nsINetworkZonePolicy

This is an XPCShell test script which tests the interaction of nsINetworkZonePolicy, load groups and channels. Please look towards the end of the file at setup_tests for a list of the test cases including descriptions.

Currently, all tests are passing.

Comment 165

[Patrick McManus [:mcmanus]]

Comment on attachment 8444211 [details] [diff] [review]
v1.3 Add support for nsINetworkZonePolicy in nsHttpChannel and related code

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

awesome to see this. I think either Honza or I can do this review - and I'm still catching up from my time away so I'll leave it to honza unless he wants to toss it back.

Comment 166

[Patrick McManus [:mcmanus]]

Comment on attachment 8444211 [details] [diff] [review]
v1.3 Add support for nsINetworkZonePolicy in nsHttpChannel and related code

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

thanks steve.

I'll make re-reviewing this my top priority if you want to respond to these comments quickly.

for clarity - because the network id is generated for each session (right now) - this effectively disables inter-session caching of resources on private networks, right? That might  be tolerable if we assume private/local networks benefit from caching the least.

::: netwerk/base/public/nsIIOService2.idl
@@ +44,5 @@
> +  /**
> +   * Returns a string ID for the current network. This ID should be
> +   * refreshed when the network link changes.
> +   */
> +  readonly attribute ACString networkLinkID;

is this going to be compat with bagder's changes? (it looks like it)

::: netwerk/base/src/nsIOService.cpp
@@ +1090,5 @@
> +    return mNetworkLinkID;
> +}
> +
> +NS_IMETHODIMP
> +nsIOService::GetNetworkLinkID(nsACString& aNetworkLinkID)

space after string and not before aNetworkLinkID

aNetworkLinkID = mNetworkLinkIDStr (see below)
return NS_OK

@@ +1109,5 @@
> +        return;
> +    }
> +    mNetworkLinkSelfAddr = aCurrentSelfAddr;
> +
> +    mNetworkLinkID = rand();

comment that right now this is just 64 bits of random

you don't want rand() it doesn't actually give you 32 bits of randomness. use nsHttpHandler::Get32BitsOfPseudoRandom() instead. Right now that has an assert and a comment in the h file that limits it to the socket thread because of concerns about seeding. I've looked into the seeding and we're fine - so you can just remove the assert and comment and use it.

::: netwerk/base/src/nsIOService.h
@@ +147,5 @@
> +private:
> +    // XXX Maybe remove these once Bug 939319 and associated bugs for
> +    // NS_NETWORK_LINK_DATA_CHANGED are complete on all supported platforms.
> +    //
> +    // ID of the current network link.

these new members appear uninitialized and can be used at any time

::: netwerk/dns/nsDNSService2.cpp
@@ +323,5 @@
>      // destroying the record prematurely.
>      nsCOMPtr<nsIDNSRecord> rec;
>      if (NS_SUCCEEDED(status)) {
>          NS_ASSERTION(hostRecord, "no host record");
> +        nsRefPtr<nsDNSRecord> recImpl = new nsDNSRecord(hostRecord);

I don't understand the purpose of recImpl - why isn't this new just assigned to rec? Then you don't need that forget() logic.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4563,3 @@
>          mLoadGroup->AddRequest(this, nullptr);
>  
> +        // Check if the loadgroup is allowed to load from private addresses.

are we sure we want to make the nzp conditional on there being a load group? It seems plausible to have an implementation that worked a different way.

@@ +4568,5 @@
> +
> +        if (nzp) {
> +            bool privateIPAddrOK = false;
> +            rv = nzp->CheckPrivateNetworkPermission(this, &privateIPAddrOK);
> +            if (privateIPAddrOK) {

if (NS_SUCCEEDED(rv) && privateIPAddrOK) {}

@@ +4571,5 @@
> +            rv = nzp->CheckPrivateNetworkPermission(this, &privateIPAddrOK);
> +            if (privateIPAddrOK) {
> +                mCaps |= NS_HTTP_ALLOW_PRIVATE_IP_ADDRESSES;
> +            }
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

not a fan of errors?- you can't assert that across an xpcom boundary. I think in the case of tihs error we just log it and move on.

@@ +4575,5 @@
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));
> +#ifdef PR_LOGGING
> +            nsAutoCString host;
> +            rv = mURI->GetAsciiHost(host);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

same

@@ +5534,5 @@
> +            // XXX Remove this call to UpdateNetworkLinkID once Bug 939319 and
> +            // associated bugs for NS_NETWORK_LINK_DATA_CHANGED are complete on
> +            // all supported platforms. For now, update the network link ID with
> +            // mSelfAddr.
> +            gIOService->UpdateNetworkLinkID(mSelfAddr);

do you think UpdateNetworkLinkID should ignore loopback? That's going to be the most common multihomed addr.

@@ +5543,5 @@
> +                // saved in the cached headers later.
> +                mPrivateNetworkID = gIOService->GetNetworkLinkID();
> +
> +                if (!(mCaps & NS_HTTP_ALLOW_PRIVATE_IP_ADDRESSES)) {
> +                    NS_WARNING("This channel not permitted to load from "

LOG() please. (or also)

@@ +5553,5 @@
> +            // If this is a document load, set permissions for the rest of the
> +            // loadgroup's requests.
> +            if (mLoadFlags & nsIChannel::LOAD_DOCUMENT_URI) {
> +                nsCOMPtr<nsINetworkZonePolicy> nzp =
> +                    do_GetService(NS_NETWORKZONEPOLICY_CONTRACTID);

this is at least the 3rd getservice() for this. can the channel just do it once and hold a ref?

@@ +5562,5 @@
> +                         "%s sub-resource loads from private networks.",
> +                         this, privateIPAddrOK ? "allows" : "forbids"));
> +                    nsresult rv =
> +                        nzp->SetPrivateNetworkPermission(this, privateIPAddrOK);
> +                    MOZ_ASSERT(NS_SUCCEEDED(rv));

no

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1710,5 @@
> +bool
> +nsHttpConnection::PeerHasPrivateIP()
> +{
> +    MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
> +    MOZ_ASSERT(mSocketTransport);

I'm not sure that's a good assert. if (!mSocketTransport) return false;

@@ +1714,5 @@
> +    MOZ_ASSERT(mSocketTransport);
> +
> +    NetAddr peerAddr;
> +    nsresult rv = mSocketTransport->GetPeerAddr(&peerAddr);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

if (NS_FAILED(rv)) return false;

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +634,5 @@
>  
> +    // Verify permission to load from private (RFC1918-like) addresses.
> +    if (!mCaps & NS_HTTP_ALLOW_PRIVATE_IP_ADDRESSES &&
> +        mConnection->PeerHasPrivateIP()) {
> +        Close(NS_ERROR_CONNECTION_REFUSED);

LOG

Comment 167

[Patrick McManus [:mcmanus]]

Comment on attachment 8444212 [details] [diff] [review]
v1.0 XPCShell test cases for nsINetworkZonePolicy

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

::: netwerk/test/unit/xpcshell.ini
@@ +237,5 @@
>  [test_multipart_streamconv.js]
>  [test_multipart_streamconv_missing_lead_boundary.js]
>  [test_nestedabout_serialize.js]
>  [test_net_addr.js]
> +[test_networkZonePolicy.js]

the skip-if line below was moved from net_addr to networkZonePolicy by your change.. I don't think that's what you meant. r+ with that fixed.

Comment 168

[Patrick McManus [:mcmanus]]

Comment on attachment 8444210 [details] [diff] [review]
v1.2 Add nsINetworkZonePolicy to protect resources loaded from private IPs

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

this patch actually all lives in necko now, so I'll steal the review from bz's queue.. boris is always welcome to add a review (to anything!).

Comment 169

[Patrick McManus [:mcmanus]]

Comment on attachment 8444210 [details] [diff] [review]
v1.2 Add nsINetworkZonePolicy to protect resources loaded from private IPs

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

r+ assuming you're ok with the nits

::: netwerk/base/src/nsNetworkZonePolicy.cpp
@@ +54,5 @@
> +  nsCOMPtr<nsILoadGroup> parent;
> +  nsCOMPtr<nsILoadGroupChild> loadGroupAsChild = do_QueryInterface(aLoadGroup);
> +  if (loadGroupAsChild) {
> +    rv = loadGroupAsChild->GetParentLoadGroup(getter_AddRefs(parent));
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

again - xpcom errors can't be asserts

@@ +73,5 @@
> +
> +  nsresult rv = NS_OK;
> +  nsCOMPtr<nsILoadGroup> owner;
> +  rv = aLoadGroup->GetLoadGroup(getter_AddRefs(owner));
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

again - xpcom errors can't be asserts

@@ +90,5 @@
> +  nsresult rv = NS_OK;
> +  nsCOMPtr<nsILoadGroup> dsParent;
> +  nsCOMPtr<nsIRequestObserver> observer;
> +  rv = aLoadGroup->GetGroupObserver(getter_AddRefs(observer));
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

again - xpcom errors can't be asserts

@@ +101,5 @@
> +      do_QueryInterface(parentAsTreeItem);
> +
> +    if (parentAsDocLoader) {
> +      rv = parentAsDocLoader->GetLoadGroup(getter_AddRefs(dsParent));
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

again - xpcom errors can't be asserts

@@ +189,5 @@
> + */
> +NS_IMETHODIMP
> +nsNetworkZonePolicy::CheckPrivateNetworkPermission(nsIRequest *aRequest,
> +                                                   bool *aAllowed)
> +{

NS_ENSURE_ARG_POINTER instead of the asserts on a xpcom interface

@@ +236,5 @@
> +NS_IMETHODIMP
> +nsNetworkZonePolicy::SetPrivateNetworkPermission(nsIRequest *aRequest,
> +                                                 bool aAllowed)
> +{
> +  MOZ_ASSERT(aRequest);

NS_ESNSURE_ARG_POINTER

Comment 170

[Steve Workman [:sworkman] (INACTIVE)]

Awesome, Pat. Thanks for all the reviews. The comments all make sense to me, and in fact some of them had been causing failures on try (only checking NetworkZonePolicy if nsHttpChannel has a loadgroup; asserting on success, asserting on loadgroups ... yeah, I was over-eager on the MOZ_ASSERTs).

I will attend to your comments and get a clean try run before uploading the next versions.

Comment 171

[Boris Zbarsky [:bzbarsky]]

Totally happy to have reviews stolen.  ;)

Comment 172

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.4 INTERDIFF Add support for nsINetworkZonePolicy in nsHttpChannel and related code

Pat, here's an interdiff patch with changes based on your comments and fixes for test failures on try. I don't trust the interdiff completely, but it should help give you an idea of the changes. I'll upload the full patch next and set r? on that. Here's the list of changes.

-- Initialized nsIOService::mNetworkLinkID to a 64bit random value.
-- Moved nsHttpHandler::Get32BitsOfPseudoRandom to NS_Get32BitsOfPseudoRandom. The nsHttpHandler version doesn't seem to be called anywhere and isn't an IDL method, so it shoudln't be called in JS somewhere. And nsNetUtil.h is already included in nsIOService.cpp where I want to use the function.
-- snprintf --> PR_snprintf
-- UpdateNetworkLinkID now ignores loopback addresses; thanks for the suggestion.
-- Added NetAddr::EqualsIP for use in UpdatedNetworkLinkID; operator== considers all values in the union/structure including port, but UpdateNetworkLinkID should only consider the IP address.

-- nsHttpChannel::mNZP = do_GetService once for the channel's lifetime; subsequent checks for non-null mNZP.
-- Backed off on the assertions :) What can I say? I was a little trigger happy ;)
-- NZP check in nsHttpChannel::AsyncOpen is no longer dependent on there being a loadgroup; I actually saw test failures because of this.
-- Check for NS_SUCCEEDED(rv) for nsSocketTransport::GetSelfAddr; in that branch in nsHttpChannel::OnTransportStatus, the status could be WAITING, in which case GetSelfAddr isn't ready yet; so, UpdateNetworkLinkID should only be called if GetSelfAddr succeeded.
-- Also initialized the data fields of nsHttpChannel::mPeerAddr and mSelfAddr to 0s.

-- winsock.h and winsock2.h inclusions were causing Windows build errors. They don't like to be included at the same time. DNS.h includes winsock2.h and nsDiskCache.hincluded winsock.h - I upgraded the latter to winsock2.h since the comment says it is for htonl/ntohl. Build and tests seem fine with this change.

-- Disabled test_httpResponseTimeout.js on Android; getting NET_RESET instead of TIMEOUT, but this feature is not enabled by default, so the breakage can be examined in a follow-up. For non-android platforms, removed a redundant call to do_test_finished.

-- test_speculative_connect.js was failing since its positive test makes a connection to loopback; so, I split up private addresses into loopback and local addresses in STS and DNS; and I provided a pref, "network.http.speculative.allowLoopback", to allow loopback addresses for spec connect. Not pretty, but it allows us to keep testing spec connect functionality. Enabled the pref in test_speculative_connect.js.

You asked in comment #166 about UpdateNetworkLinkID: yes, because the network id is generated for each session (right now), this does effectively disable inter-session caching of resources on private networks. I think I'd rather have that than cached resources leaking across sessions, and it should be a temporary measure that we cn fix once Bug 939318 and related bugs land.

Speaking of which, NetworkLinkID should be compatible with Daniel's changes yes. There'll be some minor changes to integrate them, but should be very simple.

nsRefPtr<nsDNSRecord> recImpl is used in the DNS callback because the two Hide functions are only in the concrete class, not in the IDL.

Comment 173

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.4 Add support for nsINetworkZonePolicy in nsHttpChannel and related code

Comment 174

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: NZPService

Changes based on comment #169. Carrying r+.

Comment 175

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 XPCShell test cases for nsINetworkZonePolicy

Minor updates; carrying r+.

Comment 176

[Steve Workman [:sworkman] (INACTIVE)]

Ok, finally try is clean, so let's try landing this.

try: https://tbpl.mozilla.org/?tree=Try&rev=ccb344b21a4d

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b5a6cc2a244

Comment 177

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/698c1f877c13 for being the likely cause of recent frequent xpcshell failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=43482247&tree=Mozilla-Inbound

Comment 178

[Steve Workman [:sworkman] (INACTIVE)]

Clearing needinfo; been looking into it today; no update as yet.

Comment 179

[Steve Workman [:sworkman] (INACTIVE)]

Try again: Fix test_TelemetryPing.js in bug 1039748

https://hg.mozilla.org/integration/mozilla-inbound/rev/b786f0b2d1bd

Comment 180

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b786f0b2d1bd

Comment 181

[David Bruant]

Total nit, but RFC1918 is obselete and has been updated by RFC6761
http://tools.ietf.org/html/rfc6761
No idea if this update affects the work in this bug.

Comment 182

[Marty]

This appears responsible for a new problem I just started experiencing when using localhost 8080. Bug 1041526 addressing it was filed.

Comment 183

[Patrick McManus [:mcmanus]]

backed out this patch and its followon (1041511) here:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/973c6a19f142
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/96339f5d6392

try for it here:
https://tbpl.mozilla.org/?tree=Try&rev=481ece1facc2

steve will take a look at the underlying issues when he returns. (marking NI)

Comment 184

[Patrick McManus [:mcmanus]]

Attachment: TPP

this is a request to back this bug out of aurora/33. it has already been removed from nightly/34

Approval Request Comment
[Feature/regressing bug #]: this bug!
[User impact if declined]: problems with localhost and 1918 hosts
[Describe test coverage new/current, TBPL]:
[Risks and why]: 
[String/UUID change made/needed]:none

Comment 185

[Bill Gianopoulos [:WG9s]]

OK now that his has been backed out I am going to raise a question about the whole approach here.
Blocking rfc1918 addresses has no relation to the actual vulnerability.  It IPs that really need to be restricted are the entire 127 class A, all IP addresses that pertain to physical interfaces on the client system and the clients default gateway.

Whether or not any of these are or are not RFC 1918 addresses is entirely irrelevant.

Comment 186

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/96339f5d6392

Comment 187

[Patrick McManus [:mcmanus]]

i should have marked this leave-open

Comment 188

[Bill Gianopoulos [:WG9s]]

Please do not reland this without fixing the tests that were just skipped last time.  I suspect this is why we had no idea that sites with rfc1918 IP addresses did not work at all correctly with this patch applied.

Comment 189

[Patrick McManus [:mcmanus]]

backed out of 34 now as well
  https://hg.mozilla.org/releases/mozilla-aurora/rev/396bd1c9ad33

Comment 190

[Olli Pettay [:smaug][[e-mail]]]

(changing the component so that this might show in the right bugzilla queries)

Comment 191

[Zac Spitzer]

Chrome has been working on this
CORS restrictions on internet-to-intranet connections.
https://www.chromestatus.com/feature/5733828735795200

Comment 192

[Frederik Braun [:freddy]]

Unassigning sworkman, he no longer works here.
The Chrome block mentioned in comment 191, is also discussed in this almost-a-spec document at https://wicg.github.io/cors-rfc1918/ .

Comment 193

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 194

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 195

[Jonathan Watt [:jwatt]]

That this was marked as parity-opera before Opera switched to Blink in 2013 indicates that was for the Presto engine, not current Opera.

Comment 196

[David Bruant]

(In reply to Jonathan Watt [:jwatt] from comment #195)
> That this was marked as parity-opera before Opera switched to Blink in 2013
> indicates that was for the Presto engine, not current Opera.

As far as i know, presto isn't part of a web browser with relevant market share, so i'm not clear on why tracking parity-presto matters at all

Comment 197

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 198

[u622804]

On systemd linuxes the hostname "_gateway" can be pinged, however since underscore ("_") is not a valid hostname character, does this mean that eg. Firefox won't treat it as a valid hostname and can thus never be accessed if it's part of a webpage's resources or even via javascript etc. and thus prevent it from being potentially abused (eg. trying to brute force router's password page or anything like that) ?

Comment 200

[Neil Harris]

I should also note that something this (although in this case localhost, not RFC 1918) would have at the very least made the recent Zoom videoconferencing apparent to end-users, and perhaps raised the alarm about it earlier.

Comment 201

[Neil Harris]

(In reply to Neil Harris from comment #200)

I should also note that something this (although in this case localhost, not RFC 1918) would have at the very least made the recent Zoom videoconferencing apparent to end-users, and perhaps raised the alarm about it earlier.

That should read "Zoom videoconferencing security problem"

Comment 207

[Ellie]

I would like to just add here that every web app dev that I tell about this is very surprised that browsers allow this. syncthing wasn't password protected for a long while and uses a localhost web interface that is exposed on localhost (hence reachable via this exploit), same for a gdb debugger I'm working with right now, and allowing ANY website from anywhere in the web to just silently find and access these things running on my PC just seems like a needlessly reckless default.

I think doing an opt-in header check as Chromium does it to at least abort the connection early would be the absolute minimum to make this somewhat secure, but it could still cause security issues for services that don't expect HTTP to come in and somehow crash/misbehave when any random website through my browser can just throw things at them via HTTP at localhost when it wasn't designed to be world reachable like that.

So I would suggest that a per site permission is reconsidered, with the default being off and a permission pop up any time a new site wants to access a computer-local or network local address. This should be preferably on top of the opt-in header that Chromium is already proposing.

Comment 208

[Ellie]

Nevermind, seems like same origin would at least limit it to "only" intranet attacks I assume...? So not as bad as I thought. Still, seems like a permission would be the best way to go to me, since a partial HTTP request aborted by no same origin headers is still not the nicest thing to allow for anyone to poke around with in the local network. (Which I assume would remain possible in some fashion unless this is a per site permission where the user needs to allow any such access first.) My apologizes if that is an incorrect assumption still on my side

Comment 211

[Randell Jesup [:jesup] (needinfo me)]

I think this is what Chrome is doing: requiring CORS on internet->intranet connections:
https://bugs.chromium.org/p/chromium/issues/detail?id=590714
"We'll begin requiring servers on a user's machine (127.0.0.1) or intranet (as defined by RFC1918) to explicitly opt-in to connections originating from the public internet."
Some useful discussion there

Comment 212

[Master ? [:masterquestionable]]

    See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1872502

    I think to address these problems: what CORS be should be better defined.
    https://bugzilla.mozilla.org/show_bug.cgi?id=1872569#c6

Comment 215

[Ellie]

I think CORS might be insufficient, because:

• It could still be used by a malicious website to send a HTTP request to check those headers to any local port, which could disrupt or manipulate non-HTTP services in some cases.

• Many of the services that use localhost access in a way CORS would likely still allow, e.g. Discord for invite links, or mega.nz for identifying a local mega daemon, will also break private mode and make the user identifiable despite a private window.

Both of this would likely be stopped if, how I proposed in #1890701 , there was a site setting, similar to web cam access, microphone access, etc. This would prevent drive-by HTTP disruptions and private mode disruptions unless the user explicitly opts in. CORS could still be checked in addition.

Comment 217

[AS]

Two things this would break:

• Anything reachable on both public and private addresses that accesses itself on the private address even from the public side. Thinking about network device admin pages here. Even though it's an inherently broken behavior I wouldn't be very surprised to encounter it.

• Publicly hosted apps used as a client to access devices on the LAN. This is a pattern I'm seeing increasingly often both in IT administration (central console managing multiple hosts) and with various "smart" gadgets (device exposing only an API to be used with either a mobile app or a hosted webapp).

CORS already prevents sending requests to services that do not want them. I'd argue anything these days that misbehaves upon receiving HTTP headers (or any kind of traffic that's not its intended protocol), or that sends CORS headers while being vulnerable to malicious/unauthenticated HTTP requests, is inherently insecure and is itself what needs to be fixed, not the browser. This isn't 2005 anymore. If you're using a firewall as your sole access control method, you already lost.

Comment 218

[Ellie]

There is plenty of software that listens on localhost that doesn't expect to be prepared against arbitrary internet attackers, so I think the browser does need to be fixed if it breaks that.

For what it's worth, both breakages listed above shouldn't be a problem if it was a per-site permission. It would just require the user to allow it.

Comment 219

[michael]

I find it mind boggling that such problem persists over 18 years.

When a user opens a connection to a website, they expect to download data from that website. They cannot reasonably expect that the website is capable to access the users computer or private networks (unless that website is on that private network and then it doesn't need the browser to access it).

This clearly is a breach of trust at such a monumental scale that I would be speechless if it would not make me so angry.

I worked for one of the big pharma companies. I had access to their network whenever I was connected to their VPN using certified hard- and software. I used Firefox at the time. Does that mean that all this data is potentially breached? By me? By any of the thousands of coworkers?

I already have rather low expectations towards browser security, sandboxing and all these things. But I really did not expect that each website, potentially all of their advertizers and tracker have unmitigated access to my computers local network.

I also used Firefox when I worked for a national social security agency. I worked for energy providers. I worked for banks. I now hear that my customers were safest when they used Internet Explorer and Windows.

Over 30 years of professional experience working with computers, I never expected to have to say something like that, be safe with Microsoft. Wow.

How bad does a security vulnerability have to be in order to be fixed?

How careless or malicious does a software vendor have to be in order to leave such a problem unmitigated for 18 years?

Or does any Firefox presentation contain the warning: "Using this software grants access to your network to each website you visit, even if you have a firewall"?

I'm not even talking about the 0.0.0.0 problem. An address that no website can possibly claim to have any valid reason to use.

This is the worst vulnerability I saw in more than 30 years.

The fact that people try to fix this NOW, because somebody posted a blog post describing in detail how this can be exploited to great effect, how incredibly easy it is, how dramatic the scale and number of affected users is - and seeing that the current generation of maintainers almost certainly are as surprised as I am that this problem exists, all of this makes it so much worse.

I wonder why there is no public outrage. I wonder why the NSA even bothers to write sophisticated software to peek into our guts. With Mozilla around, they could just lean back and wait on for a couple of decades.

Comment 220

[Ellie]

Since many people seem to be dropping by, this seems to be trying to mitigate the issue: https://addons.mozilla.org/en-US/firefox/addon/port-authority/ Although I don't know how reliably if somebody really tried to get around it, and sadly it doesn't seem to have a per-site permission but only a global toggle but that still seems better than nothing. Disclaimer: I'm neither affiliated with that addon nor do I have the time resources to fully review if it's safe to use.

Comment 221

[Bob Hill]

Now that this issue has suddenly become so widely and publicly recognized as a severe vulnerability, might it be appropriate to:
change "Type" from "Enhancement" to "Defect", and
change "Priority" from "P3" to "P1" or "P2", and
change "Severity" from "S3" to "S1" or "S2"?

Comment 222

[oneofthedamons]

(In reply to Ellie from comment #220)

Since many people seem to be dropping by, this seems to be trying to mitigate the issue: https://addons.mozilla.org/en-US/firefox/addon/port-authority/

This is probably OT, but since it was raised, the Port Authority extension doesn't appear to mitigate this issue. I'm indebted to this comment which points to the relevant code in the extension.

Comment 223

[Ellie]

Good catch! It mitigates any other access to LAN or localhost from external sites however, which seems to match the bug title and seems like at least a partial mitigation. Many users don't seem to think this type of access should be possible without an opt-in.

Comment 224

[Ellie]

(Correction: at least that's my best guess from the extension code. I'm not a qualified to make a definite statement, I'm not a firefox dev.)

Comment 225

[Giorgio Maone [:ma1]]

FWIW, NoScript does provide configurable protection against this.

Comment 226

[u Ltd.]

uMatrix can protect against this, with following extra rule lines:

* 0.0.0.0 * block
* 127.0.0.1 * block
* localhost * block
127.0.0.1 127.0.0.1 * inherit
localhost localhost * inherit

Note that after typing, the ordering changes alphabetically, which does not affect the effect.

Comment 227

[Bob Hill]

Here is a recent analysis of this entire issue:
https://www.oligo.security/blog/0-0-0-0-day-exploiting-localhost-apis-from-the-browser
It contains several references to Firefox.

Comment 228

[Michel Nallino]

Hi,
Three comments:

1. A workaround is available using uBlock Origin extension, with the filter list "Block Outsider Intrusion into LAN" (Settings / Filter Lists / Privacy, select "Block Outsider Intrusion into LAN").

2. Severity, and as a consequence priority, seems underestimated. It is a security vulnerability, actively exploited, allowing malicious scripts on malformed webpage to reach localhost servers or LAN devices. It is a way to circumvent firewalls:

• browser accesses a webpage with allowed outgoing connection on HTTP or HTTPS ports,
• then a malicious script is executed on this page, and reaches servers listening on localhost (directly with 127.0.0.1 calls; or indirectly with 0.0.0.0 calls on Linux and macOS, not on Windows),
• or reaches devices on LAN (with addresses such as 192.168.1.x, as an example).

3. As per https://www.oligo.security/blog/0-0-0-0-day-exploiting-localhost-apis-from-the-browser, Firefox has two different problems:

• no Private Network Access policy, that would allow access to localhost and LAN addresses from private network only, and block it from public/internet network,
• no blocking of "0.0.0.0" IP address, that is identical to "127.0.0.1" on Linux and macOS but not on Windows (blocked at the system level).

Regards,

MN

Comment 229

[Valentin Gosu [:valentin] (he/him)]

(In reply to Michel Nallino from comment #228)

3. As per https://www.oligo.security/blog/0-0-0-0-day-exploiting-localhost-apis-from-the-browser, Firefox has two different problems:

• no Private Network Access policy, that would allow access to localhost and LAN addresses from private network only, and block it from public/internet network,

We'll be starting work on this soon in bug 1481298.

• no blocking of "0.0.0.0" IP address, that is identical to "127.0.0.1" on Linux and macOS but not on Windows (blocked at the system level).

This was already fixed in bug 1889130. It's behind a pref and will be rolling out to release soon.

Comment 230

[Ellie]

I'm really sorry for repeating myself, but is there still any chance that a per site permission will be considered? Just limiting the LAN access to services that want to be reachable from internet browsers as far as I can tell still 1. seems to break private mode by letting services like discord identify you and your specific account by contacting your local desktop client instance even if you're in private mode, and still 2. seems to allow fingerprinting users by scanning all the LAN services that are enabled for internet browser access. A per-site permission like camera access for LAN-and-localhost access would easily allow the user to avoid this.