253942 - enablePrivilege parameter can change the meaning of the dialog

Status: VERIFIED FIXED

(Core :: Security, defect, P3)

Description

[Jesse Ruderman]

Scripts calling enablePrivilege can change the meaning of setences in the dialog
by passing a parameter with spaces and English words.

As usual for enablePrivilege dialog holes, exploitation requires signing your
script.  ActiveX spyware reminds us that this is not an effective deterrent.

Comment 1

[Jesse Ruderman]

Attachment: demo

Comment 2

[Jesse Ruderman]

Attachment: screenshot

Comment 3

[Jesse Ruderman]

Suggested fix:

A) Maintain a map from jargony privilege names to sentences describing them. 
For example, "A site has requested UniversalXPConnect privileges" becomes "A
site has requested: * Permission to install software on your computer".  Do not
allow sites to request privileges that are not in the map.

OR

B) Don't distinguish requests for different privileges in the UI.

Comment 4

[chris hofmann]

seems like we should try for atleast a bit of clean up in 1.0 

Comment 5

[chris hofmann]

any progress,  this might be a good one to get for the preview release

Comment 6

[Daniel Veditz [:dveditz]]

I don't know that blake is working on this so I've been working on a patch

Comment 7

[Daniel Veditz [:dveditz]]

Attachment: de-jargonize the enable privilege dialog (trunk)

This patch takes Jesse's approach "A" and de-jargonizes the capabilities
request. It also reformats the request dialog to put each capability on its own
line, and if the capability requested is not a known standard one (other apps
could define their own capabilities) then the raw capability is printed.

Since I reworded the dialog I went ahead and incorporated a fix for bug 253944,
changing the button text to "Allow" and "Deny". I reused the existing Yes No
strings from caps.properties so that the dialog will degrade somewhat
gracefully in locales that don't incorporate these changes right away.

I'm open to suggestion on the wording, especially the capability descriptions.

This is a trunk patch. The aviary patch is identical except caps.properties was
moved under toolkit/locales and trivial line number shifting.

Comment 8

[Steffen Wilberg]

The demo in comment 1 looks funny with this patch applied. Every word of "that
you dance around..." is on line of its own.

Comment 9

[Daniel Veditz [:dveditz]]

Attachment: screenshot with patch applied

The demo looks funny because it's a hack attempt. Much more obvious something
strange is going on than in the original screenshot. Normally a single
preference would be requested, or a small group (two or three). Legit cases
won't look so bad. Hopefully in cases that /do/ look this bad people will have
the sense to deny it (how many people would figure out from the original screen
shot that clicking "Yes" would grant the attacker full 0wnership of the
victim's machine?).

Comment 10

[Jesse Ruderman]

> +capdesc.CapabilityPreferencesAccess = Modify core security settings
> +capdesc.UniversalPreferencesWrite = Modify program settings

These should just be the same as UniversalXPConnect, since you can use them to
install software IIRC.

> +capdesc.UniversalPreferencesRead = Read program settings

How juicy is information in Firefox or Seamonkey prefs?

> and if the capability requested is not a known standard one (other apps
> could define their own capabilities) then the raw capability is printed.

That doesn't sound secure.  Can we make apps supply descriptions, and
automatically deny a request for a security setting we don't know about?

Comment 11

[Mike Kaply [:mkaply]]

The problem with putting this on 1.7.3 is that it is a translation hit - in this
case, it doesn't break anything (because there isn't new entries in the
properties files) ... what do other people think?

Comment 12

[Daniel Veditz [:dveditz]]

I could go either way on fixing this in 1.7

I don't want to give prefs identical strings, it would be confusing when they
are asked for at the same time (for example, to modify capability prefs you need
both CapabilityPreferencesAccess and UniversalPreferencesWrite).

I could beef up the Capability string. I don't see how UniversalPreferencesWrite
could let an attacker install software directly, but it can mess things up.
(could add a site to the xpi whitelist, but they'd still get the xpi
confirmation dialog. Could redirect the update url, though, which people would
probably trust. Could lower anti-spoofing settings, allow popups, etc.)

> How juicy is information in Firefox or Seamonkey prefs?

Pretty juicy from a privacy standpoint. Home page, specific directories on disk,
mail account info, titles of most recently edited docs. Extensions can be worse:
aim password (lightly scrambled), irc startup URLS (which might have channel
keys or passwords), adbock rexexps, etc.

> That doesn't sound secure.  Can we make apps supply descriptions, and
> automatically deny a request for a security setting we don't know about?

Creating such an API would take time to get right, and then no one is likely to
use it. I'd rather just get this in because I think it solves the problem.
Easier would be just to deny any unknown privilege, but the current code only
knows whether a property string is defined for that privilege or not. Signed
scripts (not that there are (m)any) would be broken in localizations until they
caught up.

Comment 13

[Jesse Ruderman]

> I don't see how UniversalPreferencesWrite could let an attacker install 
> software directly

I see 3 ways to 0wn the user directly:
browser.throbber.url
update.app.url
update.extensions.autoUpdate
security.dialog_enable_delay

and 3 ways to spoof so effectively that the user is effectively 0wned:
browser.chromeURL
dom.disable_window_open_feature.titlebar
network.proxy.autoconfig_url
network.proxy.http

> I don't want to give prefs identical strings, it would be confusing when they
> are asked for at the same time (for example, to modify capability prefs you 
> need both CapabilityPreferencesAccess and UniversalPreferencesWrite).

When a script requests CapabilityPreferencesAccess and
UniversalPreferencesWrite, only show "Run or install software on your machine" once.

> Creating such an API would take time to get right, and then no one is likely 
> to use it. I'd rather just get this in because I think it solves the problem.

It doesn't solve the problem.  Scripts could request a priv containing a
character that looks like a space, use a language like Japanese where spaces can
be omitted, or use a language like Japanese where vertical text is allowed. 
Scripts could request "UniversalPreferencesRead but not UniversalXPConnect" and
it wouldn't be obvious that clicking "Allow" grants the ability to install software.

> Signed scripts (not that there are (m)any) would be broken in localizations 
> until they caught up.

mkaply suggested: on the 1.7 branch, hard-code English strings and use those if
the properties string is missing.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 156234 [details] [diff] [review]
de-jargonize the enable privilege dialog (trunk)

unsetting 1.8a3 approval request. we've shipped already.

Comment 15

[chris hofmann]

ben and caillon, can you review?

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

From what I understood of my conversation with dveditz, he was making a new patch...

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 156234 [details] [diff] [review]
de-jargonize the enable privilege dialog (trunk)

please don't request approval until you have a fully reviewed patch. thanks.

Comment 18

[Daniel Veditz [:dveditz]]

Attachment: testcase with non-ascii char in privilege name

It turns out a script cannot request a privilege that looks like Japanese
(comment 13). The API uses "string" (which is supposed to be 7-bit ascii) but
then throws it through the utf8 converter which fails and results in an empty
string in the dialog. You can ask for anything this way and the user wouldn't
know what.

Comment 19

[Daniel Veditz [:dveditz]]

Attachment: Original demo minus apostrophe in privilege name

To address part of Jesse's concerns in comment 13 I've restricted the set of
valid characters in capability names to alphanums plus the delimiters '_', '-',
and '.'

Our built-in prefs use StudlyCaps and custom prefs most likely follow that
example, but it seemed safer to allow the other word break styles (all can be
found in Mozilla code) rather than break some custom app built on 1.7

Comment 20

[Daniel Veditz [:dveditz]]

Attachment: Updated patch to reject illegal chars in capability names

I didn't reword any of the capability descriptions, but I did beef up the
warnings on the surrouding dialog (I hope). If people ignore the warnings that
are already there then repeating "can be used to install software" over and
over won't help.

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 157313 [details] [diff] [review]
Updated patch to reject illegal chars in capability names

>Index: caps/include/nsScriptSecurityManager.h
>===================================================================
>RCS file: /cvsroot/mozilla/caps/include/nsScriptSecurityManager.h,v
>retrieving revision 1.81
>diff -u -p -8 -r1.81 nsScriptSecurityManager.h
>--- caps/include/nsScriptSecurityManager.h	10 Jul 2004 19:38:28 -0000	1.81
>+++ caps/include/nsScriptSecurityManager.h	29 Aug 2004 11:00:38 -0000
>@@ -403,16 +403,19 @@ private:
>     GetPrincipalAndFrame(JSContext *cx,
>                          nsIPrincipal** result,
>                          JSStackFrame** frameResult);
> 
>     static PRBool
>     CheckConfirmDialog(JSContext* cx, nsIPrincipal* aPrincipal,
>                        const char* aCapability, PRBool *checkValue);
> 
>+    static nsresult
>+    FormatCapabilityString(nsAString& aCapability);

This could be a void method since you have it always returning NS_OK, and
nothing even bothers to check the return value.


>     nsresult
>     SavePrincipal(nsIPrincipal* aToSave);
> 
>     nsresult
>     CheckXPCPermissions(nsISupports* aObj,
>                         const char* aObjectSecurityLevel);
> 
>     nsresult
>Index: caps/src/caps.properties
>===================================================================
>RCS file: /cvsroot/mozilla/caps/src/caps.properties,v
>retrieving revision 1.4
>diff -u -p -8 -r1.4 caps.properties
>--- caps/src/caps.properties	17 Apr 2004 21:50:35 -0000	1.4
>+++ caps/src/caps.properties	29 Aug 2004 11:00:38 -0000
>@@ -30,20 +30,30 @@
> # under the terms of either the GPL or the LGPL, and not to allow others to
> # use your version of this file under the terms of the MPL, indicate your
> # decision by deleting the provisions above and replace them with the notice
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
>-Yes = Yes
>-No = No
>+Yes = Allow
>+No = Deny
> Titleline = Internet Security
> CheckMessage = Remember this decision
>-EnableCapabilityQuery = A script from "%S" has requested %S privileges. You should grant these privileges only if you are comfortable downloading and executing a program from this source. Do you wish to allow these privileges?
>+EnableCapabilityQuery = A script from "%S" is requesting enhanced abilities that are UNSAFE and could be used to compromise your machine or data:\n\n%S\n\nAllow these abilities only if you would install software from this source and are sure it would be free of viruses or other malicious programs.

Hm, how about:

  A script from "%S" is requesting the following enhanced abilities which are
potentially unsafe:\n\n%S\n\nYou should allow these only if you trust the
source to be free of viruses or malicious programs.

Rationale:
- The sites I trust are not "UNSAFE".
- Some users would not install software from that source because they don't
have the priveleges to do so on a machine, so I removed that bit.  Plus, it
really doesn't need to be there IMO.


>+nsresult
>+nsScriptSecurityManager::FormatCapabilityString(nsAString& aCapability)
>+{
>+    nsAutoString newcaps;
>+    NS_NAMED_LITERAL_STRING(capdesc, "capdesc.");
>+    PRInt32 pos;
>+    PRInt32 index = kNotFound;
>+    nsresult rv;
>+
>+    NS_ASSERTION( kNotFound == -1, "Basic constant changed, algorithm broken!");

Do we really need this assertion?  Presumably, in the already unlikely event 
we have a dropped in XPCOM string impl, which changes kNotFound, they probably
also have fixed the Find() methods to return the right thing.  But maybe it
would just help to #undef and re-#define it to the right thing

Nit: If this gets left in, remove the space after the open parenthesis.

>+
>+    do {
>+        pos = index+1;
>+        index = aCapability.FindChar(' ', pos);
>+        nsDependentSubstring rawcap( aCapability, pos,
>+                                    (index == kNotFound) ? index : index-pos );

Nit: remove the space after the opening and before the closing parentheses.

>+
>+        nsXPIDLString capstr;
>+        rv = sStrBundle->GetStringFromName(
>+                            nsPromiseFlatString(capdesc+rawcap).get(),
>+                            getter_Copies(capstr));
>+        if (NS_SUCCEEDED(rv))
>+            newcaps += capstr;
>+        else
>+            newcaps += rawcap;

I like this approach.  I think the one thing I'd suggest is prepending
"Unknown: " or something to anything we don't know about.  That would impose an
extra l10n requirement, but I think that would make it clearer that we don't
have a bunch of random things as possible capabilities.


>+
>+        newcaps += NS_LITERAL_STRING("\n");
>+    } while (index != kNotFound);
>+
>+    aCapability = newcaps;
>+    return NS_OK;
>+}
>+
> PRBool
> nsScriptSecurityManager::CheckConfirmDialog(JSContext* cx, nsIPrincipal* aPrincipal,
>                                             const char* aCapability, PRBool *checkValue)
> {
>     nsresult rv;
>     *checkValue = PR_FALSE;
> 
>     //-- Get a prompter for the current window.


...

>+    //-- Check capability string for valid characters
>+    //
>+    //   Logically we might have wanted this in nsPrincipal, but performance
>+    //   worries dictate it can't go in IsCapabilityEnabled() and we may have
>+    //   to show the capability on a dialog before we call the principal's
>+    //   EnableCapability().
>+    //
>+    //   We don't need to validate the capability string on the other APIs
>+    //   available to web content. Without the ability to enable junk then
>+    //   isPrivilegeEnabled, disablePrivilege, and revertPrivilege all do
>+    //   the right thing (effectively nothing) when passed unallowed chars.
>+    for (const char *ch = capability; *ch; ++ch)
>+    {
>+        if ( !NS_IS_ALPHA(*ch) && *ch != ' ' && !NS_IS_DIGIT(*ch)

Nit: its the parentheses-space nit again.  ;-)

Comment 22

[Daniel Veditz [:dveditz]]

Attachment: Once more, with feeling!

Fixes nits, displays extended caps as "Unknown: <foo>".  This isn't too
friendly to any extensions that create these, but I suspect no one uses this
and we can fix the word later if anyone comes up with anything (or add the
capability definition to caps.properties).

In warning dialog I left the wording of the first sentence. The capabilities
ARE unsafe whether you trust the site or not. That's where the trust comes in,
you are trusting the site to handle them responsibly. We don't need to minimize
the danger, if anything people don't pay enough attention when presented with
dialogs of this type.

I removed the reference to installing in the second sentence to tighten things
up.

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 157437 [details] [diff] [review]
Once more, with feeling!

>+            JS_SetPendingException(cx, STRING_TO_JSVAL(JS_NewStringCopyZ(cx, msg)));

Null-check the return value of JS_NewStringCopyZ, and sr=me.

/be

Comment 24

[Daniel Veditz [:dveditz]]

Attachment: Address brendan's nit

JS_SetPendingException() idiom was used throughout the file, we should fix all
or none.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 157478 [details] [diff] [review]
Address brendan's nit

carrying over brendan's sr=

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 157478 [details] [diff] [review]
Address brendan's nit

requesting approvals

Comment 27

[Mike Kaply [:mkaply]]

We can't take this change in 1.7 if it affects a translated property file.

We need to come up with some way to fix this for 1.7 that doesn't affect
translation, or if it affects translation, it has to work if those new strings
in caps.properties don't exist.

Comment 28

[chris hofmann]

Comment on attachment 157478 [details] [diff] [review]
Address brendan's nit

a=chofmann for the branches

Comment 29

[Mike Kaply [:mkaply]]

Comment on attachment 157478 [details] [diff] [review]
Address brendan's nit

No, this patch cannot be used for the 1.7.x branch.

We must have a patch for the branch that doesn't involve changing .properties
files, even if it requires hardcoding english in this case.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 157478 [details] [diff] [review]
Address brendan's nit

This patch works absolutely FINE without a changed caps file. it's not quite as
good that way but it still helps the problem a bit by putting each privilege on
a separate line and screening out bogus chars.

Comment 31

[Mike Kaply [:mkaply]]

Comment on attachment 157478 [details] [diff] [review]
Address brendan's nit

Thanks for the info Dan!

Sorry for the hassle.

a=mkaply for 1.7 checkin without the properties file change.

Nice work.

Comment 32

[Daniel Veditz [:dveditz]]

Fixed on trunk plus aviary, 1.7, and 1.7.2+ branches

Comment 33

[Asa Dotzler [:asa]]

Verified with Mozilla 1.7.3 on windows XP and Firefox 0.10 on winXP and demo 1.

Comment 34

[Tracy Walker [:tracy]]

verified with windows 1.7.5 12/15