369341 - gatherTextUnder is a bad treewalker, making the phishing filter easy to evade

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[Phil Ringnalda (:philor)]

Links in HTML email are checked for a mismatch between the href URL and the link text, since <a href="http://phisher.com">http://ebay.com</a> is an obvious clue, but the link text comes from the much-copied-around gatherTextUnder function in {product}.../utilityOverlay.js, which is easily confused - see bug 274486. <a href="http://phisher.com"><span>h</span><span><span>t</span></span>tp://ebay.com</a> will sail right past http://mxr.mozilla.org/seamonkey/source/mail/base/content/phishingDetector.js#212 because the passed-in aLinkNodeText will be "h t"

The maybe-tolerable fix would be to make gatherTextUnder's "// Last resort..." method test for .parentNode.nextSibling, and continue walking up the tree as long as there's still depth && !.nextSibling, but since the desired effect is a TreeWalker, using a genuine TreeWalker seems like a better way to go.

Comment 1

[Daniel Veditz [:dveditz]]

There are other simple ways to evade the phishing heuristic, such as have the text link be simply "eBay" -- victims are still going to click.

It would definitely be nice to fix the heuristic to catch this, but I don't know that this needs to be hidden as a security bug.

Comment 2

[Phil Ringnalda (:philor)]

The other ways, though, require obviously HTML email that should put a slightly aware person on guard: target URL as link text is popular because you can make it your only HTML, and hope someone mistakes it for a linkified URL in plain text.

I don't mind fixing this in a public bug, fixing all the copies in bug 274486, but I'd rather not be the one to explain in detail to phishers just what sort of email they should send to Tb users.

Comment 3

[Daniel Veditz [:dveditz]]

I don't mind leaving this hidden to obscure the fixing process.

Comment 4

[Scott MacGregor]

me neither. Thanks for helping out with this Phil. Let me know if you need any help.

Comment 5

[Phil Ringnalda (:philor)]

Attachment: mail/ v.1

Well, I certainly need some sort of help, though I'm not sure what. I've never figured a good workable plan for these "same tiny change in four files in three products" bugs, and while this seems to work fine in trunk Tb (and the equivalent change in trunk Fx), I've got no idea how to evaluate how good range's bits were for either 1.8.1.x or, moreso, 1.8.0.x, where I don't have a single working build environment to even do basic testing.

Comment 6

[Phil Ringnalda (:philor)]

Attachment: SeaMonkey v.1

Wherein I'm even more at sea: works fine for Bookmark This Link in xpfe SeaMonkey, and I don't use mail in SeaMonkey, or know how to build suite/, or have any idea what branch(es) SeaMonkey's shipping, or know who'll wind up willing to review this :)

Comment 7

[[e-mail]]

What about linked broken images with alt text?

Comment 8

[Phil Ringnalda (:philor)]

Comment on attachment 254507 [details] [diff] [review]
SeaMonkey v.1

Mmm, <img alt="h" border="0" style="margin:0;padding:0;">ttp://www.ebay.com works wonderfully. Currently, too, since gatherTextUnder bails with just the h.

Comment 9

[[e-mail]]

At least on the trunk, a document encoder seems to be the best approach.

Comment 10

[Phil Ringnalda (:philor)]

Attachment: testcase

I *think* this covers the variations that matter.

Comment 11

[Phil Ringnalda (:philor)]

Attachment: SeaMonkey v.2

I look forward to figuring out how on earth to use a document encoder, in bug 274486. Meanwhile, baling wire and duct tape that can land on the branch, too. I'm not sure whether the xpfe/ copy is deadcode, or just used by something I don't know of, but I patched it as well, even though non-suiterunner seems to be using the copy in suite/common/.

The change to concat alt text and actual text, instead of only using alt text, makes me a little uneasy, but not uneasy enough to want to have two more copies so that bookmarks/save as can have subtly different behavior than mail. And having both in mixed image/text situations would at least include what I would expect to get, the text, rather than the current oddness of bookmarking a linked image with the text "Hot! Hot! Hot!" underneath, and getting the bookmark title as just "Click to enlarge."

Comment 12

[[e-mail]]

Comment on attachment 255546 [details] [diff] [review]
SeaMonkey v.2

(In reply to comment #11)
>I look forward to figuring out how on earth to use a document encoder
1. Create it
2. Init it
3. Point it at the container
4. Encode away!

>I'm not sure whether the xpfe/ copy is deadcode, or just used by something I
>don't know of, but I patched it as well, even though non-suiterunner seems to
>be using the copy in suite/common/.
For SeaMonkey patch suite/ on the trunk and xpfe/ on the branch.

Comment 13

[Phil Ringnalda (:philor)]

Attachment: Thunderbird v.2

Opened files don't seemed to get the anti-phishing treatment, so I don't have nice testcases, but this seems to work with my collection of things I sent myself (and certainly should work just like it does in a browser).

Comment 14

[Scott MacGregor]

Phil, can we land the tb patch on the branch?

Comment 15

[Phil Ringnalda (:philor)]

An excellent idea, possibly only surpassed by my having done it last weekend, when I thought I did.

mail/base/content/utilityOverlay.js 1.3, 1.1.2.3

Comment 16

[Phil Ringnalda (:philor)]

xpfe/communicator/resources/content/utilityOverlay.js 1.95
suite/common/utilityOverlay.js 1.98

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 255546 [details] [diff] [review]
SeaMonkey v.2

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 255637 [details] [diff] [review]
Thunderbird v.2

approved for 1.8.0.12, a=dveditz for release-drivers. Appears to be already checked into Tbird2, right?

Comment 19

[Phil Ringnalda (:philor)]

Right: both branches for Sm, just 1.8.0.x for Tb, was exactly what I needed, thanks.

Well, that and a solid eight hours when I can watch 1.8.0.x tinderboxes, but Saturday is plenty soon enough.

Comment 20

[Phil Ringnalda (:philor)]

Comment on attachment 255637 [details] [diff] [review]
Thunderbird v.2

Gah, too many branches and forks in the tree, too few brains between my ears. Tb didn't fork utilityOverlay.js in 1.8.0.x, so the "SeaMonkey" patch is Sm&Tb both, there.

Comment 21

[Phil Ringnalda (:philor)]

xpfe/communicator/resources/content/utilityOverlay.js: 1.87.4.7
xpfe/communicator/resources/content/utilityOverlay.js: 1.87.4.3.2.1