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

REOPENED
Unassigned

Status

()

P3
enhancement
REOPENED
13 years ago
7 months ago

People

(Reporter: usenet, Unassigned)

Tracking

(Depends on: 2 bugs, Blocks: 3 bugs, {parity-opera-mini, sec-moderate, sec-want})

unspecified
mozilla34
parity-opera-mini, sec-moderate, sec-want
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +
blocking1.9 -
in-testsuite +

Firefox Tracking Flags

(firefox33 affected, firefox34 affected)

Details

(Whiteboard: [sg:moderate][sg:want vector][necko-backlog], URL)

Attachments

(7 attachments, 32 obsolete attachments)

2.54 KB, text/plain
Details
251.88 KB, application/zip
Details
25.06 KB, patch
Details | Diff | Splinter Review
55.66 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
25.86 KB, patch
sworkman
: review+
Details | Diff | Splinter Review
26.04 KB, patch
sworkman
: review+
Details | Diff | Splinter Review
TPP
108.55 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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.

Updated

13 years ago
Whiteboard: [sg:want?]
(Reporter)

Comment 1

13 years ago
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?
(Reporter)

Comment 2

13 years ago
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?)
(Reporter)

Comment 3

13 years ago
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.


(Reporter)

Comment 4

13 years ago
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
(Reporter)

Comment 5

12 years ago
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.
(Reporter)

Comment 6

12 years ago
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?
Severity: major → critical
Assignee: nobody → dveditz
Component: Security → Security
Product: Firefox → Core
QA Contact: firefox → toolkit
Whiteboard: [sg:want?] → [sg:want]
Assignee: dveditz → nobody
Component: Security → Networking
Flags: blocking1.9?

Updated

12 years ago
Blocks: 322301
Summary: JavaScript can be abused to scan and manipulate internal networks → JavaScript can scan internal networks (block rfc 1918 local addresses from non-local addresses)

Updated

12 years ago
Flags: blocking1.9? → blocking1.9-
Whiteboard: [sg:want] → [sg:want][wanted-1.9]
QA Contact: toolkit → networking
Flags: wanted1.9+
Whiteboard: [sg:want][wanted-1.9] → [sg:want]

Comment 7

11 years ago
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.
Doug said he'd look at this.
Assignee: nobody → dougt
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
Flags: blocking1.9- → blocking1.9?
(Reporter)

Comment 10

11 years ago
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?

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Whiteboard: [sg:want] → [sg:moderate vector]
Flags: wanted1.9+
Flags: wanted-next+
Flags: tracking1.9+

Comment 11

11 years ago
Read this <http://www.0x000000.com/?i=524> and please reconsider whether this blocks 1.9. 
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?
Flags: blocking1.9?
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.
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

11 years ago
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.
Flags: blocking1.9? → blocking1.9-
(Reporter)

Comment 16

11 years ago
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
(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

11 years ago
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.
Flags: blocking1.9- → blocking1.9?
Summary: JavaScript can scan internal networks (block rfc 1918 local addresses from non-local addresses) → Firefox can scan internal networks (block rfc 1918 local addresses from non-local addresses)
(Reporter)

Comment 19

11 years ago
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.
Summary: Firefox can scan internal networks (block rfc 1918 local addresses from non-local addresses) → Firefox can be used as a vector to attack internal networks (block rfc 1918 local addresses from non-local addresses)

Comment 20

11 years ago
Another question is: what does this have to with the release schedule? We take security fixes on branches.
(Reporter)

Comment 21

11 years ago
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.
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).
Summary: Firefox can be used as a vector to attack internal networks (block rfc 1918 local addresses from non-local addresses) → CSRF attacks to internal networks should be mitigated (block rfc 1918 local addresses from non-local addresses)
(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.
(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...
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.
Agreed.

Updated

11 years ago
Severity: critical → major
Summary: CSRF attacks to internal networks should be mitigated (block rfc 1918 local addresses from non-local addresses) → Mitigate CSRF attacks against internal networks (block rfc 1918 local addresses from non-local addresses)

Comment 27

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

you are not describing a blocker, imho

(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

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

You mean like renominating everything that gets minused?

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

11 years ago
(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

11 years ago
(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.
(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.
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.





Posted patch very much a work in progress. (obsolete) — Splinter Review
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)
thanks for the links.

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

Comment 38

11 years ago
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."
(Reporter)

Comment 39

11 years ago
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.
(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).
(Reporter)

Comment 41

11 years ago
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.
(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.
(Reporter)

Comment 43

11 years ago
Address list added to attachments list, to avoid having to repeat it over and over in comments.
(Reporter)

Comment 44

11 years ago
added 255.255.255.255, and more detailed rationale as to narrow scope of this list; this is not a "bad addresses" list
(Reporter)

Comment 45

11 years ago
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"



(Reporter)

Comment 46

11 years ago
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. 
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.
Flags: blocking1.9? → blocking1.9-
(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.
(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. 
(Reporter)

Comment 50

11 years ago
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.
can anyone tell me if there are other IP address that we need to consider in ipv4.  also, would love a ipv6 code snippet.
Attachment #307154 - Attachment is obsolete: true

Updated

11 years ago
Attachment #317620 - Attachment is patch: true
Attachment #317620 - Attachment mime type: application/octet-stream → text/plain
Posted file wip. component (obsolete) —
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?
if it isn't perfectly clear, this is a work in progress.
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/
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.
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?
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.
Attachment #317621 - Attachment is obsolete: true

Updated

11 years ago
Attachment #318536 - Attachment description: fixes the frameset issue → wip 2. component. fixes the frameset issue
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)?
builds the dns patch are here:
https://build.mozilla.org/tryserver-builds/2008-05-02_16:08-dougt@mozilla.com-dns_private_method/

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

Comment 61

11 years ago
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.
Attachment #307213 - Attachment is obsolete: true
Attachment #307215 - Attachment is obsolete: true
(Reporter)

Comment 62

11 years ago
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?
(Reporter)

Comment 63

11 years ago
...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?
thanks neil!  You want to code this up for the DNS patch?

Updated

11 years ago
Duplicate of this bug: 437045
This patch does the dns check in js allowing us to deploy this to even ff2 users.  it is still missing a ipv6 check.
Attachment #317620 - Attachment is obsolete: true
Attachment #318536 - Attachment is obsolete: true
Attachment #319087 - Attachment is obsolete: true
Doh.  Uploaded wrong patch.  Here's the patch with correct handling of Link-Local unicast.
Attachment #329492 - Attachment is patch: true
Attachment #329492 - Attachment mime type: application/x-javascript → text/plain
Attachment #329489 - Attachment is patch: false
Attachment #329492 - Attachment description: wip 6. dougt's component plus start of IPv6. → wip 6. dougt's component plus correct start of IPv6.
Attachment #329492 - Attachment is patch: false
Brandon, i am not sure this patch protects everything it needs to:

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

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. 
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



Updated

11 years ago
Assignee: doug.turner → bsterne
(Reporter)

Comment 72

11 years ago
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. 
Tests that tickle the nsIExternalProtocolHandler's uses of both local and web apps would be nice too.
(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?
what mike said, we need to setup test cases (more than people.mozilla.org/~dougt/nat_fun.html).  Was hoping you can help.

Updated

11 years ago
Duplicate of this bug: 453230
Attachment #324108 - Attachment is obsolete: true
Attachment #329489 - Attachment is obsolete: true
Attachment #329492 - Attachment is obsolete: true
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.
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.
(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 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.
(Reporter)

Comment 83

10 years ago
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/
(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!
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.
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

9 years ago
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?
(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

9 years ago
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.
Attachment #452943 - Attachment is obsolete: true
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

9 years ago
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 94

9 years ago
Posted file mq patch series (obsolete) —

Comment 95

9 years ago
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.
(Reporter)

Comment 96

9 years ago
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.
(Reporter)

Comment 97

9 years ago
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.

Updated

9 years ago
Depends on: 584155

Comment 98

9 years ago
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.

Updated

9 years ago
Depends on: 585191

Comment 99

9 years ago
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

9 years ago
Attachment #457633 - Attachment is obsolete: true
Attachment #457636 - Attachment is obsolete: true
Attachment #457638 - Attachment is obsolete: true

Comment 101

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

Comment 102

9 years ago
Fixes bad code and compiler warnings I meant to fix before posting the last version of this patch.
Attachment #466816 - Attachment is obsolete: true

Comment 103

9 years ago
More cleanup: I had changed the pref name without considering the pref observer.
Attachment #466834 - Attachment is obsolete: true

Comment 104

9 years ago
Added a test for the "security.address-restrictions.enable" preference.
Attachment #466817 - Attachment is obsolete: true

Comment 105

9 years ago
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

9 years ago
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.

Updated

9 years ago
Blocks: 371598
Hey jsamuel, any progress on comment 106?

Comment 108

9 years ago
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.
(Reporter)

Comment 109

8 years ago
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.?
(Reporter)

Comment 110

8 years ago
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.
(Reporter)

Updated

8 years ago
Depends on: 255107
(Reporter)

Comment 111

8 years ago
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

8 years ago
Attachment #466844 - Attachment is obsolete: true

Comment 113

8 years ago
Attachment #466846 - Attachment is obsolete: true

Comment 114

8 years ago
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

8 years ago
(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.
(Reporter)

Comment 116

8 years ago
(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

8 years ago
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.
(Reporter)

Comment 118

8 years ago
(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

8 years ago
>  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.
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?
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

8 years ago
(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.
(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

8 years ago
(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

8 years ago
(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

8 years ago
bsterne - should this bug remain assigned to you or should we find a new owner?
Assignee: bsterne → nobody
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.

Updated

8 years ago
Assignee: nobody → sjhworkman
(Reporter)

Comment 128

8 years ago
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

7 years ago
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.

Updated

7 years ago
Severity: major → enhancement

Comment 131

7 years ago
Moving to sg:want since this is an enhancement to our security, not a proper bug in our code.
Whiteboard: [sg:moderate vector] → [sg:want vector]
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.
Assignee: sworkman → nobody
Component: Networking → Security
QA Contact: networking → toolkit
Whiteboard: [sg:want vector] → [sg:moderate][sg:want vector]
(Reporter)

Comment 133

7 years ago
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
(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?
(Reporter)

Comment 135

7 years ago
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.
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

7 years ago
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

Updated

7 years ago
Whiteboard: [sg:moderate][sg:want vector] → [sg:moderate][sg:want vector][parity-opera]
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.
(Reporter)

Comment 140

5 years ago
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/
Duplicate of this bug: 962017
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.
Assignee: nobody → sworkman
Attachment #386622 - Attachment is obsolete: true
Attachment #506043 - Attachment is obsolete: true
Attachment #506044 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8422003 - Flags: feedback?(mcmanus)
Attachment #8422003 - Flags: feedback?(jduell.mcbugs)
Attachment #8422003 - Flags: feedback?(bzbarsky)
I'm not likely to have time to look at this for at least a week.  :(
(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?
Apart from the people you already asked for needinfo, no.
:) Okie doke then. We'll just wait for when you free.
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.
Attachment #8422003 - Attachment is obsolete: true
Attachment #8422003 - Flags: feedback?(mcmanus)
Attachment #8422003 - Flags: feedback?(jduell.mcbugs)
Attachment #8422003 - Flags: feedback?(bzbarsky)
Attachment #8425145 - Flags: feedback?(jduell.mcbugs)
Attachment #8425145 - Flags: feedback?(honzab.moz)
Attachment #8425145 - Flags: feedback?(bzbarsky)
Adding Tanvi as this is somewhat related to the security hooks work.
I think nsINetworkZonePolicy iface is missing from the patch ;)
Ooops. Yup, NZPService.h/cpp and nsINetworkZonePolicy.idl were omitted in the last patch. Should be added now.
Attachment #8425145 - Attachment is obsolete: true
Attachment #8425145 - Flags: feedback?(jduell.mcbugs)
Attachment #8425145 - Flags: feedback?(honzab.moz)
Attachment #8425145 - Flags: feedback?(bzbarsky)
Attachment #8426540 - Flags: feedback?(jduell.mcbugs)
Attachment #8426540 - Flags: feedback?(honzab.moz)
Attachment #8426540 - Flags: feedback?(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 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 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
Attachment #8426540 - Flags: feedback?(honzab.moz) → feedback+
Added comments; split up patches.

Haven't attended to Honza's initial feedback yet.
Attachment #8426540 - Attachment is obsolete: true
Attachment #8426540 - Flags: feedback?(jduell.mcbugs)
Attachment #8426540 - Flags: feedback?(bzbarsky)
Attachment #8427922 - Flags: feedback?(jduell.mcbugs)
Attachment #8427922 - Flags: feedback?(bzbarsky)
No changes in this from WIP 1.1; just splitting up the patches.
Attachment #8427926 - Flags: feedback?(mcmanus)
Attachment #8427926 - Flags: feedback?(jduell.mcbugs)
Attachment #8427926 - Flags: feedback?(honzab.moz)
Attachment #8427923 - Attachment description: WIP1.2 Support nsINetworkZonePolicy in nsILoadGroup → WIP 1.2 Support nsINetworkZonePolicy in nsILoadGroup
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 on attachment 8427926 [details] [diff] [review]
WIP 1.2 Support nsINetworkZonePolicy in Necko's HTTP code

Feedback provided in comment 153
Attachment #8427926 - Flags: feedback?(honzab.moz) → feedback+
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 :)
Attachment #8427926 - Flags: feedback?(mcmanus) → feedback+
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".
Attachment #8427922 - Flags: feedback?(bzbarsky) → feedback+
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.
Attachment #8427923 - Flags: feedback?(bzbarsky) → feedback+
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).
Attachment #8427922 - Attachment is obsolete: true
Attachment #8427923 - Attachment is obsolete: true
Attachment #8427922 - Flags: feedback?(jduell.mcbugs)
Attachment #8444210 - Flags: review?(bzbarsky)
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.
Attachment #8427926 - Attachment is obsolete: true
Attachment #8427926 - Flags: feedback?(jduell.mcbugs)
Attachment #8444211 - Flags: review?(mcmanus)
Attachment #8444211 - Flags: review?(honzab.moz)
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.
Attachment #8444212 - Flags: review?(mcmanus)
Attachment #8444212 - Flags: review?(honzab.moz)
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.
Attachment #8444211 - Flags: review?(mcmanus)
Attachment #8444212 - Flags: review?(mcmanus)
Attachment #8444211 - Flags: review?(honzab.moz) → review?(mcmanus)
Attachment #8444212 - Flags: review?(honzab.moz) → review?(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
Attachment #8444211 - Flags: review?(mcmanus) → review-
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.
Attachment #8444212 - Flags: review?(mcmanus) → review+
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!).
Attachment #8444210 - Flags: review?(bzbarsky) → review?(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
Attachment #8444210 - Flags: review?(mcmanus) → review+
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.
Totally happy to have reviews stolen.  ;)
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.
Posted patch NZPServiceSplinter Review
Changes based on comment #169. Carrying r+.
Attachment #8444210 - Attachment is obsolete: true
Attachment #8448319 - Flags: review+
Attachment #8444211 - Attachment is obsolete: true
Attachment #8448313 - Attachment is obsolete: false
Attachment #8444212 - Attachment is obsolete: true
Attachment #8448319 - Attachment filename: NZPService → v1.3 Add nsINetworkZonePolicy to protect resources loaded from private IPs
Attachment #8448316 - Flags: review?(mcmanus) → review+
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
Flags: needinfo?(sworkman)
Clearing needinfo; been looking into it today; no update as yet.
Flags: needinfo?(sworkman)
https://hg.mozilla.org/mozilla-central/rev/b786f0b2d1bd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

Comment 181

5 years ago
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.

Updated

5 years ago
Depends on: 1041420

Updated

5 years ago
Depends on: 1041511

Comment 182

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

Updated

5 years ago
Depends on: 1042157
Depends on: 1042497
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)
Status: RESOLVED → REOPENED
Flags: needinfo?(sworkman)
Resolution: FIXED → ---
Posted patch TPPSplinter Review
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
Attachment #8462294 - Flags: approval-mozilla-aurora?
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.
status-firefox33: --- → affected
status-firefox34: --- → fixed

Updated

5 years ago
status-firefox34: fixed → affected
https://hg.mozilla.org/mozilla-central/rev/96339f5d6392
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla33 → mozilla34
i should have marked this leave-open
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8462294 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Depends on: 1044938

Updated

5 years ago
No longer depends on: 1044938
status-firefox33: affected → wontfix

Updated

5 years ago
status-firefox33: wontfix → affected
Flags: needinfo?(sworkman)
(changing the component so that this might show in the right bugzilla queries)
Component: Security → Networking
Whiteboard: [sg:moderate][sg:want vector][parity-opera] → [sg:moderate][sg:want vector][parity-opera][necko-backlog]

Comment 191

2 years ago
Chrome has been working on this
CORS restrictions on internet-to-intranet connections.
https://www.chromestatus.com/feature/5733828735795200
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/ .
Assignee: sjhworkman → nobody
That this was marked as parity-opera before Opera switched to Blink in 2013 indicates that was for the Presto engine, not current Opera.
Whiteboard: [sg:moderate][sg:want vector][parity-opera][necko-backlog] → [sg:moderate][sg:want vector][parity-presto opera][necko-backlog]

Comment 196

a year ago
(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
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-opera-mini
Whiteboard: [sg:moderate][sg:want vector][parity-presto opera][necko-backlog] → [sg:moderate][sg:want vector][necko-backlog]
See Also: → bug 1481298

Comment 198

7 months ago
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) ?
You need to log in before you can comment on or make changes to this bug.