308366 - mail containing link to "local" numerical IP address should not be marked as scam

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[u81239]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8b4) Gecko/20050908 Firefox/1.4

Hey,

The attached plain text email is marked as a scam by Thunderbird. I wonder why
that is.

Perhaps it is because Thunderbird itself ‘linkifies’ http://www.backbase.com to
http://www.backbase.com/, which it subsequently detects as a scam attempt?

Two solutions then:
1. it should add a trailing slash when linkifying the URL, and
2. the scam detection should not be triggered on plain text emails (unless there
are specific detection mechanisms for that).


~Grauw

Reproducible: Always

Steps to Reproduce:

Comment 1

[u81239]

Attachment: The email

Comment 2

[u81239]

Oh, and 3. the scam detection should not consider the difference between
http://www.backbase.com and http://www.backbase.com/ as a scam attempt.


~Grauw

Comment 3

[Mike Cowperthwaite]

I'm guessing the scam detection is flagging on this link:
  http://192.168.50.109/#forum/messageThread.html?forum=1&thread=84[4] 
         ^^^^^^^^^^^^^^

Comment 4

[u81239]

Yes, it seems so (because the bugmail also showed a scam bar).

Or maybe it’s because of the # fragment identifier. We use links like that for
Backbase Ajax applications, such as http://www.backbase.com/ .

Anyways, it shouldn’t, the link is legitimate.


~Grauw

Comment 5

[u81239]

No, it seems to be the IP. I’m getting the scam warning again when I received
this mail:

"Your new WordPress blog has been successfully set up at:

http://192.168.50.109/weblog/wp-admin"

It’s getting pretty annoying... I really think the scam warning is giving way
too many false positives like this.


~Grauw

Comment 6

[Jeff Beal]

Hyperlinks containing IP addresses instead of domain names are considered to be
a common 'phishing' scheme, and therefore trigger the scam detector.  I'm OK
with this, but I think there should be a whitelist of some sort where I can
identify an IP address as a "safe" IP address.  When I click 'Not a scam', the
IP addresses in the message should automatically be added to the whitelist.

Comment 7

[u81239]

That sounds reasonable. In addition to that, local network addresses (10.*.*.*,
192.168.*.*, and the other one) should be on that list by default.

~Grauw

Comment 8

[zug_treno]

Related to bug 305089, bug 320739 and bug 318916?

Comment 9

[Magnus Melin [:mkmelin]]

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

Comment 10

[Magnus Melin [:mkmelin]]

-> new, adjusting summary

Comment 11

[jan klopper]

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

Comment 12

[Scott MacGregor]

Should be pretty easy for someone to modify phisingDetector.js to address comment 7

Comment 13

[u81239]

Attachment: Patch: do not mark as phishing if local IP address

Your wish is my command!

This patch adds a check on whether the IP address is a local IP address (one of: 10.0.0.0/8, 169.254.0.0/16, 172.16.0.0/20 or 192.168.0.0/16), and only marks the address as a phishing address when it is not one of those.


~Grauw

Comment 14

[u81239]

Attachment: Patch: do not mark as phishing if local IP address

(nit: tab->spaces)

Comment 15

[Scott MacGregor]

Comment on attachment 228174 [details] [diff] [review]
Patch: do not mark as phishing if local IP address

nice. I wish this happened with all of my bugs!

This patch looks good, one minor change:

     if (hostNameIsIPAddress(hrefURL.host, unobscuredHostName))
-      phishingType = kPhishingWithIPAddress;
+      if (hostNameIsHarmfulIPAddress(unobscuredHostName))
+        phishingType = kPhishingWithIPAddress;

can be just:
if (hostNameIsIPAddress(hrefURL.host, unobscuredHostName) && hostNameIsHarmfulIPAddress(unobscuredHostName))

Do you need me to checkin this patch or do you have CVS access? Thanks again!

Comment 16

[u81239]

And maybe change
   if (ipComponents[0] == 172 && (ipComponents[1] >= 16 && ipComponents[1] < 32))
to
   if (ipComponents[0] == 172 && ipComponents[1] >= 16 && ipComponents[1] < 32)
? It doesn’t really matter, I suppose.

But no, I don’t have CVS access, so if you could please check it in :).


~Grauw

Comment 17

[Scott MacGregor]

Attachment: tweaked patch

lhostl, what do you think of this? It should do the same thing, but the code is a little bit tighter.

Comment 18

[u81239]

I知 fine with it :).

(I do think though that the initial one, though not as tight, was a little bit better readable :).)

Comment 19

[u81239]

(In reply to comment #15)
> This patch looks good, one minor change:
> 
>      if (hostNameIsIPAddress(hrefURL.host, unobscuredHostName))
> -      phishingType = kPhishingWithIPAddress;
> +      if (hostNameIsHarmfulIPAddress(unobscuredHostName))
> +        phishingType = kPhishingWithIPAddress;
> 
> can be just:
> if (hostNameIsIPAddress(hrefURL.host, unobscuredHostName) &&
> hostNameIsHarmfulIPAddress(unobscuredHostName))

Actually, that’s even a necessary change, because there’s an ‘else if’ after it, and it would ‘else’ on the inner if and not the outer one.


~Grauw

Comment 20

[Rob Marshall [tH]]

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

Comment 21

[Mark Jansen]

(In reply to comment #7)
> That sounds reasonable. In addition to that, local network addresses (10.*.*.*,
> 192.168.*.*, and the other one) should be on that list by default.
> 
> ~Grauw

Any IP address in a plain text message should me marked as ok, only for HTML messages the default allowed ranges should be as mentioned.

In plain text messages, you can simply SEE the URL is numerical. Only in HTML messages the URL can be scammed. A scam prevention feature should only be triggered in case of scams, not in cases one can simply see what the actual URL is.

Comment 22

[Mark Jansen]

In case you change the summary of this bug to make explicitly clear you mean only local IP's should be whitelisted, then the bug 320739 is NOT a duplicate of this bug. In my bug report #320739 I really meant to comment as I did hear: any plain text message should not be marked as scams, only HTML message should be.

https://bugzilla.mozilla.org/show_bug.cgi?id=320739

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <phishingDetector.js> [Checkin: Comment 24 & 28 & 30]

TB part:
*3 space nits cleanup.

SM part:
Port/Copy this bug fix to SeaMonkey.

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 229133 [details] [diff] [review]
(Bv1) <phishingDetector.js>
[Checkin: Comment 24 & 28 & 30]


Checkin: {
2006-07-20 09:55	bugzilla%standard8.demon.co.uk 	mozilla/mail/base/content/phishingDetector.js 	1.18
2006-07-20 09:55	bugzilla%standard8.demon.co.uk 	mozilla/mailnews/base/resources/content/phishingDetector.js 	1.10
}

'approval1.8.1=?': (SeaMonkey only)
Simple enhancement, ported from Thunderbird, no risk.

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 229133 [details] [diff] [review]
(Bv1) <phishingDetector.js>
[Checkin: Comment 24 & 28 & 30]

Mike changed my resquest from |approval1.8.1?      | to |approval-thunderbird2?|:
I just want to make it explicit that the (real) code change is in the SeaMonkey part only.

Comment 26

[Scott MacGregor]

Comment on attachment 229133 [details] [diff] [review]
(Bv1) <phishingDetector.js>
[Checkin: Comment 24 & 28 & 30]

I can't actually approve the patch for 1.8.1 for you serge, I don't have access to that flag anymore. But I can fake the system out by approving it for tb2 since this is a mailnews only change that doesn't effect firefox.

Comment 27

[Serge Gautherie (:sgautherie)]

Oh, I didn't know: thanks to both of you, then :-)

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Checked in the mailnews part of the patch on the branch, with a=Neil.
mozilla/mailnews/base/resources/content/phishingDetector.js 	1.1.2.7

Comment 29

[Magnus Melin [:mkmelin]]

Adjusting summary for what this actually did.

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 229133 [details] [diff] [review]
(Bv1) <phishingDetector.js>
[Checkin: Comment 24 & 28 & 30]


Checkin: {
2006-07-25 17:22	gavin%gavinsharp.com 	mozilla/mail/base/content/phishingDetector.js 	1.12.2.6 	MOZILLA_1_8_BRANCH  	3/3  	Fix whitespace nits to sync branch/trunk.
}