334893 - Naked hostname isn't very effective for identifying dialog origin

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Jesse Ruderman]

The fix for bug 298934 added the hostname to dialogs, so that any dialog from bugzilla.mozilla.org has "bugzilla.mozilla.org" at the top in bold.  I claim that this is ineffective.

Since that form of origin identification is unfamiliar to most users (compared to, say, the address bar) and the hostname is by itself (not part of a sentence), it's easy to overlook and can even be incorporated into a message.  For example, consider an alert that looks like this:

  ------------------------------------------------------------------------
  |  bugzilla.mozilla.org                                                |
  |                                                                      |
  |  is the new address for Wells Fargo.  Please update your bookmarks.  |
  ------------------------------------------------------------------------

See bug 334891 for a spoofing exploit that incorporates this trick.

Comment 1

[Daniel Veditz [:dveditz]]

it was bug 301069 that added it to the mac. On Linux and Win the host is embedded in the title bar of the popup which makes the distinction between browser- and site-generated content more clear. Mac sheets don't have a titlebar, probably should have added an "Alert from site: " prefix or something more than just bold to set that line off from the popup content.

Comment 2

[Jesse Ruderman]

It might make sense to change the titlebar on Windows, too.

Comment 3

[Daniel Veditz [:dveditz]]

Fixing this would require a localization change, we'll have to catch it in FF2

What do you propose? "Message from <site>" maybe?

Comment 4

[Jesse Ruderman]

Sounds reasonable.  I wonder if mconnor or beltzner has ideas/opinions.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: alert from mail.google.com on mac

Attaching a screencap for reference

"Alert" and "Message" seem a little unfriendly to me. Do we have separate codepaths for alert and confirm? If so, maybe we could use:

alert = The page at %s says ...
confirm = The page at %s is asking ...

Windows:
     .-------------------------------------------------------.
     | The page at http://mail.google.com says ...          X|
     |-------------------------------------------------------|
     |                                                       |
     | ####    Blah blah blah blah blah blah                 |
     | ####                                                  |
     |                                                       |
     |                                           ( OK )      |
     '-------------------------------------------------------'

Mac:
     |         The page at http://mail.google.com says ____  |
     |                                                       |
     | ####    Blah blah blah blah blah blah                 |
     | ####                                                  |
     |                                                       |
     |                                             ( OK )    |
     '-------------------------------------------------------'

Comment 6

[Darin Fisher]

Josh: can you work up a patch for this in the FF2 beta2 timeframe?  Thanks!

Comment 7

[Josh Aas]

working on a patch right now

Comment 8

[Josh Aas]

Attachment: fix v1.0

Comment 9

[Josh Aas]

Attachment: fix v1.0 screenshot

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 227596 [details] [diff] [review]
fix v1.0

Did you diff the wrong (joke) version or something?

How about "[JavaScript Application from %S]" instead. Would then want to change the " - " to simply " ", or then you could use two %S in the string and format it all in one call.

But no, you can't make the localizers have to figure out what a Beltzner is.

Comment 11

[Josh Aas]

(In reply to comment #10)
> (From update of attachment 227596 [details] [diff] [review] [edit])
> Did you diff the wrong (joke) version or something?

Something like that :)

> How about "[JavaScript Application from %S]" instead. Would then want to change
> the " - " to simply " ", or then you could use two %S in the string and format
> it all in one call.

I don't know what the best way to word it is myself, that decision is better made by UI and security people. I just coded up what Beltzner suggested we do in comment #5. I prefer Beltzner's way of doing it. On checkin the string would read:

"The page at http://www.somesite.com says..."

which seems more clear to me than:

"JavaScript Application from http://www.somesite.com"

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Dan, I think Josh was making a clever joke. I'm assuming that the dialog would look like it does in my mockup in comment 5. One suggested modification, though would be to use a ":" instead of a "..." which I think will parse better.

Comment 13

[Josh Aas]

Attachment: fix v1.1

This includes some string formatting adjustments - ":" instead of "..." and if there is extra information, it'll be inserted like this:

"The page at http://www.slashdot.org says (extra info here):"

Otherwise normally it'll say:

"The page at http://www.slashdot.org says:"

Comment 14

[Mark Mentovai]

Comment on attachment 229850 [details] [diff] [review]
fix v1.1

>Index: dom/src/base/nsGlobalWindow.cpp
>+              const PRUnichar *formatStrings[] = {  ucsPrePath.get(), extraString.get() };

Extra space after the opening brace.

>Index: xpfe/global/resources/locale/en-US/commonDialogs.properties
>-ScriptDlgTitle=[JavaScript Application] %S
>+ScriptDlgTitle=The page at %S says%S:
>+ScriptDlgDefaultTitle=[JavaScript Application] %S

These keys aren't really descriptive.  How about leaving ScriptDlgTitle as it is, and making the "Beltzner says" message keyed as ScriptDlgHeading?

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 229850 [details] [diff] [review]
fix v1.1

The code's fine, but I have a nit and l10n concerns about the actual string

>+ScriptDlgTitle=The page at %S says%S:

nit: wouldn't "A" page be better? Most sites have more than one.

concern 1: when you use multiple replacements please use the reordering syntax, %1$S %2$S so localizers can adjust the string as necessary.

bigger concern 2: There's no way for a localizer to deal with "says%S:"--it expands to nothing or " (<title>):" with no way to fix if that's not appropriate syntax. My original string technically had this problem, but we cheated since it was a raw partial URL. When you phrase it as a sentence we can't get away with cheating. I think you need two completely separate strings, picking the appropriate one depending on whether the prompt title (extraString) is present or not.

ScriptDlgHeading=A page at %S says:
ScriptDlgHeadingWithTitle=A page at %1$S says (%2$S):

I'm also not sure any reasonable prompt title makes much sense in parentheses, but since I'm not sure how many people use prompt passwords I'm less concerned.

   A page at http://www.yahoo.com says (Enter your ID):

?

Comment 16

[Jesse Ruderman]

How about making the page-supplied dialog title appear as a normal line of prompt text?  So...

   A page at http://www.yahoo.com says:

   Enter your ID

   ...

Comment 17

[Daniel Veditz [:dveditz]]

I have no idea what forms of prompt are commonly used. People might have the title say the name of the site (which will look silly next to our repetition), I know I've seen it essentially duplicate the text line that's already in the dialog (as in the example I gave, imagine now that "Enter your ID" shows up in the dialog twice, instead of once each in the dialog and the title), and a lot of times the title is just skipped. It isn't even an option for alert() and confirm().

Any radical change is complicated by the fact that people want their code to work in other browsers too. Opera appears to drop the 3rd (title) argument entirely, maybe we could get away with just doing that. Can't test IE at the moment.

Comment 18

[Daniel Veditz [:dveditz]]

IE, like Opera, also only supports two arguments to prompt(). Looks like the prompt title argument is useless, and by dropping it we simplify the localization here tremendously. In other words, forget aInTitle/extraString and just do a single replacement for the site host.

Comment 19

[Josh Aas]

Attachment: fix v1.2

Changed key names and dropped support for 3rd argument to prompt().

Comment 20

[Mark Mentovai]

Comment on attachment 230753 [details] [diff] [review]
fix v1.2

 void
 nsGlobalWindow::MakeScriptDialogTitle(const nsAString &aInTitle,
                                       nsAString &aOutTitle)
 {
+  // We don't use "aInTitle" because we ignore the 3rd (title) argument to
+  // prompt(). IE and Opera ignore it too. See Mozilla bug 334893.

"Warning: variable aInTitle is unused."  (I'm guessing.)

Why don't you fix MakeScriptDialogTitle's prototype and fix up the callers?  It's not an interface and it's only ever used internally by nsGlobalWindow.

Comment 21

[Josh Aas]

Attachment: fix v1.3

Comment 22

[Mark Mentovai]

Comment on attachment 230936 [details] [diff] [review]
fix v1.3

+ScriptDlgTitle=[JavaScript Application]
+ScriptDlgHeading=The page at %S says:

Maybe now these keys would make more sense:

ScriptDlgTitleNoHost
ScriptDlgTitleWithHost

For the trunk, we should consider taking the bogus argument out of the nsIDOMWindowInternal::Prompt method.

Comment 23

[Axel Hecht [:Pike]]

Comment on attachment 230936 [details] [diff] [review]
fix v1.3

>RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/global/commonDialogs.properties,v
<...>

>-ScriptDlgTitle=[JavaScript Application] %S
>+ScriptDlgTitle=[JavaScript Application]
>+ScriptDlgHeading=The page at %S says:


This is a semantic change and should change the key for the localized string. Regarding #22, layout-related parts like "heading" and "title" should in general be the last part of a key.
While you're at it, adding a localization note to describe the nature of %S would be good.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 230936 [details] [diff] [review]
fix v1.3

>-ScriptDlgTitle=[JavaScript Application] %S
>+ScriptDlgTitle=[JavaScript Application]
>+ScriptDlgHeading=The page at %S says:

Looks good overall, but technical procedural nit: as mentioned by Mark and Axel when we change an existing string (ScriptDlgTitle) we must change the key name to flag it for re-translation. Otherwise the localizers won't know it needs to be changed and then the raw %S will show up in localized builds.

A different sort of translation tool would track this kind of change in the original and it wouldn't be a problem, but we have to work with the tools we have. Maybe "ScriptDlgGenericHeading"?

Comment 25

[Josh Aas]

Attachment: fix v1.4

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 231110 [details] [diff] [review]
fix v1.4

sr=dveditz

Thanks! sorry to be a PITA

Comment 27

[Josh Aas]

checked in on trunk

Comment 28

[Mike Schroepfer]

Comment on attachment 231110 [details] [diff] [review]
fix v1.4

approved by schrep for drivers

Comment 29

[Josh Aas]

Attachment: fix v1.4 branch

Turns out the branch patch is pretty different.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 231424 [details] [diff] [review]
fix v1.4 branch

sr=dveditz

Comment 31

[Josh Aas]

landed on MOZILLA_1_8_BRANCH