154031 - Improper case-sensitive domain comparison in same-domain-only code

Status: RESOLVED WORKSFORME

(Core :: Graphics: Image Blocking, defect, P4)

Description

[Tim]

Very simply put, a strcmp was used to compare two host names instead of
strcasecmp.  Patch is as follows (line 189, for reference):
The extra comment is irrelavent to this issue, but is related (and already
discussed vaguely in another bug)
Oh, and to make this a more proper bug report, this is in the trunk CVS from
earlier today.

RCS file: /cvsroot/mozilla/extensions/cookie/nsImages.cpp,v
retrieving revision 1.18
diff -u -r1.18 nsImages.cpp
--- nsImages.cpp        31 May 2002 18:40:32 -0000      1.18
+++ nsImages.cpp        24 Jun 2002 23:09:25 -0000
@@ -180,13 +180,13 @@
       if (*tailFirstHostname == '.') {
         dotcount++;
       }
-      if (dotcount == 2) {
+      if (dotcount == 2) { /* XXX: so does that make a.co.uk == b.co.uk? */
         tailFirstHostname++;
         break;
       }
       tailFirstHostname--;
     }
-    if (PL_strcmp(tailFirstHostname, tailHostname)) {
+    if (PL_strcasecmp(tailFirstHostname, tailHostname)) {
       *permission = PR_FALSE;
       return NS_OK;
     }

Comment 1

[Tim]

Attachment: Same as comment

Comment 2

[Stephen P. Morse]

> /* XXX: so does that make a.co.uk == b.co.uk? */

Yes, it treats them as being in the same domain.  That's an old problem -- see 
bug 8743 in detail.  Please remove that comment from your patch since it is not 
related.  With that change, r=morse

But please tell me why this patch is needed.  The user doesn't type in the 
hostname -- it comes from the site.  So are you saying that at one time the 
site reported its name as xxx.com when you decided to block the image, and at 
some future visit when the blocking test is being made, the site reported its 
name as XXX.com?

Comment 3

[Tim]

Attachment: More isolated patch

Comment 4

[Tim]

My reason is that regardless of who or what produces them, domain names are
always case-insensitive.  I was thinking of a purely imaginary
anti-image-blocking server that randomly rotated the domain case on image
elements it served.  When I made the patch, I knew the problem would be rare and
unlikely, but I chose to fix what I saw as (insignificantly) incorrect behavior
instead of leaving it as-is.

Also worth noting is that bug 33576 also appears to cover (now a lot more than)
the problem with using 2 levels of the fully qualified host name for comparison

Comment 5

[Stephen P. Morse]

Comment on attachment 89582 [details] [diff] [review]
More isolated patch

r=morse

Comment 6

[Ben Bucksch (:BenB)]

Steve Morse, please confirm bugs when you see or review the patches.

Comment 7

[Stephen P. Morse]

I gave it an r= because the patch was completely harmless.  But I can't confirm 
that this is really a problem or that the patch is really needed.

Comment 8

[Mitchell Stoltz (not reading bugmail)]

Mass reassigning of Image manager bugs to [e-mail], and futuring.
Most of these bugs are enhancement requests or are otherwise low priority at
this time.

Comment 9

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

I tried a page with a <img src="http://TEST.LOCALDOMAIN/warning.gif"> on it,
while blocking test.localdomain. The image was blocked anyway.
Is there still a need for this patch? (note that the code has been drasticly
changed, so the patch won't apply anymore, but the general structure still applies)

Comment 10

[[e-mail]]

This should be the job of the URI parser now - the permissions backend uses
URI's in its API now (mostly), rather than strings. because of this, we can
guarantee domain names have been parsed by the URI's (scheme-dependent)
constructor. For the http case, this performs a lowercase canonicalization:

http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsStandardURL.cpp#535

For other schemes, this may not be the case, but I think that's up to the URI
parser to decide, not the permissions backend.

-> worksforme since the permissions rewrite "fixed" this.