209902 - Images from same 2nd-level domain and another 3rd-level domain shouldn't be blocked

Status: VERIFIED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

[Manko]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030612
Build Identifier: Mozilla 1.4 RC2

Images from same 2nd-level domain and another 3rd-level domain are blocked.
Older Mozilla builds including 1.4 RC1 show it.

I consider old behavior to be more useful.

See URL for example: page site is www.yandex.ru, images source site is
im-tub.yandex.ru.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Boris Zbarsky [:bzbarsky]]

Did you set things to not load images from other servers?  Because www.yandex.ru
and im-tub.yandex.ru are different servers....

Comment 2

[Manko]

Of course, yes. I always use option "Accept images from originating server only".

But previous behavior (showing images from another 3rd-level domain if 2nd-level
domain is the same) I guess to be more smart and usable. Moz 1.2.1, 1.3.1 and
even 1.4rc1 do so.

If I didn't want see images from another 3rd-level domain, I simply choosed
"Block Images from this Server in image context menu.

Btw, even in Bugzilla if you use use option "Accept images from originating
server only", you now can't see Mozilla logo at top of page.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm?  We should never have loaded those other images.. certainly nothing between
rc1 and rc2 should have changed here...

Comment 4

[Manko]

Formally you're absolutely right.

But previous behavior was more handly. Often images on big sites are placed on
hosts beginning with "img." or "image.". Formerly (even in 1.4rc1) I saw it, now
- no.

Especially unpleasant this bug is because that bug 47475 still isn't fixed.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: fix tail matching function

There has always been code that made "only images from originating server" mean
"only images from the same 2nd level domain, even if the 3rd and higher domains
are different".
But that code was broken, but has been for a while. So 1.4rc1 should be broken
too.
Anyway, a patch.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

-> me

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126004 [details] [diff] [review]
fix tail matching function

>+    const nsACString &firstTail = Substring(firstHost, firstHost.Length() - tail.Length(), tail.Length());
...
>+    if ((firstHost.Length() > tail.Length() && 

Um.  That check really needs to happen _before_ that Substring() call, no? The
Substring() impl will happily read past start of string otherwise...

The rest looks fine, but please fix that.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: updated, check length

Added the length check. Can't remove it from the other if. We also want aaa.com
to match when visiting www.aaa.com, so firstHost having the same length as tail
is a special case.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 126049 [details] [diff] [review]
updated, check length

carrying forward r=dwitte

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126049 [details] [diff] [review]
updated, check length

There we go.  ;)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126049 [details] [diff] [review]
updated, check length

Checked in.  It may be a good idea to take this for 1.4 as well... small simple
change that fixes a pretty major problem with this option.

Comment 12

[Jim Brown]

*** Bug 204887 has been marked as a duplicate of this bug. ***

Comment 13

[Manko]

Nice!

One question: if I see site via IP instead of DNS name (e.g.
http://207.200.81.215/ instead if http://www.mozilla.org/), does this patch
allow see images from (e.g.) 192.168.81.215? This should be denied.

Comment 14

[[e-mail]]

No, it doesn't, unfortunately. It should.

it sucks that this code is forked between cookies & images - they should really
be using a common function.

Comment 15

[[e-mail]]

of course, by "no, it doesn't", I meant "no, it doesn't work properly". as in,
it's broken like you say. ;)

Comment 16

[Manko]

Can we add fork with checking last DNS suffix ("org" or "215" in this case)? If
it's a valid number, we have an IP and hosts must be compared strictly; if it
isn't a number, we have a DNS name and hosts must be compared by 2nd-level domain.

Yet another suggestion: DNS names should be lowercased before comparing.
mozilla.org and Mozilla.Org are the same host.

Comment 17

[[e-mail]]

>Can we add fork with checking last DNS suffix ("org" or "215" in this case)? If
>it's a valid number, we have an IP and hosts must be compared strictly; if it
>isn't a number, we have a DNS name and hosts must be compared by 2nd-level
>domain.

Cookies already handles this correctly; it's just a matter of making images use
this function (or fixing it separately)

>Yet another suggestion: DNS names should be lowercased before comparing.
>mozilla.org and Mozilla.Org are the same host.

nsIURI's are canonicalized during parsing, so this isn't an issue.

Comment 18

[Jim Brown]

Site 'www.apple.com' fails to load images with 'accept from originating server
only' selected.
They use image loading through akamai.net.  E.g.,
src="http://a772.g.akamai.net/7/772/51/6c91a531b2fde3/www.apple.com/t/2002/us/en/i/1bg.gif"

Comment 19

[[e-mail]]

what's wrong with that?

Comment 20

[Manko]

Jim. I guess this to be correct behavior. apple.com and akamai.net are
absolutely different domains and browser would need redundant intelligence to
load these images if "Accept images from originating server only" is checked.

Solution may be like in Popup Management: you point servers to always load
images from regardless of current site URL. Of course, you should open another
bug for this issue.

Comment 21

[Manko]

Hope to see it in 1.4 release

Comment 22

[Michiel van Leeuwen (email: mvl+moz@)]

*** Bug 211356 has been marked as a duplicate of this bug. ***

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 126049 [details] [diff] [review]
updated, check length

moving approval request forward.

Comment 24

[Asa Dotzler [:asa]]

mozilla1.4 shipped. unsetting blocking1.4 request.

Comment 25

[Mike Kaply [:mkaply]]

Comment on attachment 126049 [details] [diff] [review]
updated, check length

a=mkaply

Comment 26

[Mike Kaply [:mkaply]]

Please add fixed1.4.1 keyword when this is checked in.

Comment 27

[Manko]

OK, good work, but there are 2 issues here.

1. Important. Strict checking for numeric IPs, see comment 16.

2. Less important, but seems significant. Some 2nd-level subdomains in national
domains are public (e.g. ".co.uk", ".org.ru", ".net.au" etc.). DNS ending of
such domains should be compared by 3rd-level domain name.

Comment 28

[[e-mail]]

checked in to MOZILLA_1_4_BRANCH.

Comment 29

[Michiel van Leeuwen (email: mvl+moz@)]

manko: this bug is about a very specific regression. The comment on the IP
numbers should be in another bug. Don't know if it is filed already.
The comment on .co.uk is also a different problem, but don't hold your breath on
it ever getting fixed. How do you know the if wx.yz is a .com or a .co.uk like
domain?

Comment 30

[Manko]

Yes, we can't know all public domains in all countries. But, I think, simple
euristic - treating as public "co", "com", "org" and "net" subdomains in any
2-letter 1st-level domain - would be enough.

Comment 31

[Michiel van Leeuwen (email: mvl+moz@)]

My main point was: this bug is fixed :) Please search for another bug, or open a
new one.

Comment 32

[Manko]

Verified in 1.5b

Comment 33

[Andrew Schultz]

*** Bug 219316 has been marked as a duplicate of this bug. ***