448626 - fix a (probably harmless) mistake in nsUrlClassifierDBServiceWorker::IsCanonicalizedIP()

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Bartłomiej Brzozowiec (BartZilla)]

User-Agent:       Konqueror
Build Identifier: 

There is some mistake in nsUrlClassifierDBService.cpp - in a method nsUrlClassifierDBServiceWorker::IsCanonicalizedIP(). See http://mxr.mozilla.org/firefox/source/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp#1931 or http://bb.homelinux.org/firefox/sources/3.0.1/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp.html#1931 .

This line:
return (i1 <= 0xFF && i1 <= 0xFF && i1 <= 0xFF && i1 <= 0xFF);
should be replaced with:
return (i1 <= 0xFF && i2 <= 0xFF && i3 <= 0xFF && i4 <= 0xFF);
I guess. It is probably pretty harmless anyway...


Reproducible: Always

Comment 1

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: fix IsCanonicalizedIP()

Patch.
BTW, is there some way to add a patch during filling a new bug? Because I haven't found any...

Comment 2

[Matthias Versen [:Matti]]

You can only attach it later..

Comment 3

[Reed Loden [:reed]]

Pushed as 17096:ddab14b88e4a.

Comment 4

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

Dave, what does this patch do, functionally, and why is this change needed/wanted? Can this be tested?

Comment 5

[Dave Camp (:dcamp)]

The IsCanonicalizedIP() function is used by the classifier to decide whether to treat a host as an IP address (which is handled in a single chunk, ie "154.128.100.3") or as a hostname (which is handled as a series of hostname components, ie "www.foo.com" and "foo.com" etc).

The function is supposed to check for four numeric components that are < 256.  This patch fixes a bug where it checked for four numeric components, only the first of which needs to be < 256.

As the title says, this is probably a harmless bug - it means that 150.200.888.900 will be tested as an IP address (one string) rather than a hostname.  But given that there won't be blacklist entries at a domain called 200.888.900, it's not much of a practical problem, but it's a very-low-risk fix.

I'll look at writing a test.

Comment 6

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

Comment on attachment 331829 [details] [diff] [review]
fix IsCanonicalizedIP()

Sounds good to me. Thanks.

Approved for 1.9.0.2. Please land in CVS. a=ss

Be sure to land any test you create in CVS as well.

Comment 7

[Dave Camp (:dcamp)]

Actually it turns out to be really tough to test, because there's an earlier pass (during hostname canonicalization) that makes sure that the numbers will always be < 255.  So there's actually no way to trigger this bug.

Comment 8

[Dave Camp (:dcamp)]

Checking in src/nsUrlClassifierDBService.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp,v  <--  nsUrlClassifierDBService.cpp
new revision: 1.81; previous revision: 1.80
done