326628 - Dialogs for pkcs11.addmodule and pkcs11.deletemodule are too jargony

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Jesse Ruderman]

To reproduce:

1. javascript:pkcs11.deletemodule("foopy");

Result:

A dialog that has no protection against bug 162020-style attacks and asks a question that doesn't mean anything to me:

  Confirm
    Are you sure you want to delete this security module? 
    Module name: foopy

Expected: 

A dialog that makes sense and protects against 162020-style attacks if necessary, or no dialog.

Comment 1

[Jesse Ruderman]

pkcs11.deletemodule() probably has the same issues.

Comment 2

[Kai Engert [:KaiE:]]

Jesse, what wording would make sense to you?

I'm fine with the idea to add a delay to that dialog, if it's generated from JavaScript.


I don't understand why you added comment 1, because it repeats the function stated in comment 0, did you intend to mention an additional function?


I am fine to add a small delay before the button can be pressed, if you think it makes sense.

Comment 3

[Jesse Ruderman]

In comment 1, I meant to say "pkcs11.addmodule probably has the same issues".

I have no idea what the dialogs do, so I can't suggest a better wording.

Comment 4

[Brendan Eich [:brendan]]

Whoever wrote this interface missed the memo about JS (and Java, and Smalltalk) using camelCaps (so deleteModule, not deletemodule).  Grump.

/be

Comment 5

[Kai Engert [:KaiE:]]

Jesse, FYI: pkcs#11 modules are external libraries that install additional security functionality, e.g. to allow Mozilla to access smard cards or USB crypto tokens.

Comment 6

[Jesse Ruderman]

What could a malicious page do if it could get you to add a module through addmodule?  Could it take over your computer?

What could a malicious page do if it could get you to remove an existing module, especially in a default mozilla.org build of Firefox?

Comment 7

[Kai Engert [:KaiE:]]

Bob, please correct me if I'm wrong.


> What could a malicious page do if it could get you to add a module through
> addmodule?  Could it take over your computer?

Besides adding a module, an attacker would have do to one more thing: Install a malicious executable shared library / dll on the user's machine, which contains the entry points expected by NSS.

If such a library gets installed and added to the user's profile using addmodule(), NSS will call functions on the library. That code will run with the user's permissions on the operating system level.


> What could a malicious page do if it could get you to remove an existing
> module, especially in a default mozilla.org build of Firefox?

According to security device manager in preferences, a default profile has 3 modules in NSS enabled, a crypto service (probably for providing functionality), the software security device (which contains the user's private keys, probably cert trust settings, etc. and is protected by the master password), and the builtins, which contains the list of root CA certificates and their default trust.

I don't know whether deletemodule() can be used to disable these modules. Bob can probably answer this question immediately.

If a malicious site were able to delete/disable these modules, this would qualify as a denial of service, as the user would no longer be able to perform security functionality like https/SSL using the profile.

Comment 8

[Jesse Ruderman]

I split "protect against bug162020-style attacks" into bug 328771, since that's more urgent and might be easier to fix.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

An Attacker who can get his own PKCS11 module installed into a mozilla-family
product has the ability to run any code with the user's privileges.  But 
with specific relevance to crypto security, it could:

a) install its own bogus root CA certs, thereby facilitating huge MITM attacks.

b) be an MITM right inside the browser, copying the encrypted or decrypted 
plaintext into storage, and later send that captured text to its home server.
This would be spyware that saw all the user's supposedly encrypted and secure
SSL (https, imaps, smtps, etc.) data.  

There is a company that produced a module to do exactly those things for MSIE.
So, the threat of this is not merely hypothetical.  

Comment 10

[Daniel Veditz [:dveditz]]

I don't see any text changes in the cards for 1.8.0.x releases. bug 328771 will have to be protection enough. If you want to fix this by blocking these highly specialized functions that are useless to most people then we could do that on the branch -- but I doubt that's the approach the crypto team wants to take.

Comment 11

[Kai Engert [:KaiE:]]

> If you want to fix this by blocking these highly
> specialized functions that are useless to most people then we could do that on
> the branch -- but I doubt that's the approach the crypto team wants to take.

I agree that we should try hard to keep the addmodule / deletemodule functionality!

Comment 12

[Kai Engert [:KaiE:]]

> I don't see any text changes in the cards for 1.8.0.x releases.

Being a non english speaker, I guess "to not see in the cards" has the meaning of "to not consider it doable".

I think it's a good idea to come up with better wording for add/delete dialogs.

Instead of
  "Are you sure you want to delete this security module?"
what about
   "A security module is software that provides additional 
    functionality on your computer, often used to access special
    security hardware like smart cards.
    Are you sure you want to disable this security software?"

Instead of
  "Are you sure you want to install this security module?"
I propose using the same new wording as above,
but changing "disable" to "install".

Should we ask Mike Beltzner to help out with better wordings?

Comment 13

[Wan-Teh Chang]

I suggest changing "security module" to "crypto module"
or "cryptographic module".  The word "security" can mean
anything from anti-virus to crypto.

Comment 14

[Jesse Ruderman]

I think the addmodule dialog should just be the extension-installation dialog.  The fact that it *claims* to be a cryptographic module is irrelevant to whether you trust the site enough to let it install software with your privileges.

The extension installation dialog contains these sentences, which are all relevant:

"A web site is requesting permission to install the following item..."

"Malicious software can damage your computer or violate your privacy."

"You should only install software from sources that you trust."

Comment 15

[Kai Engert [:KaiE:]]

> irrelevant to whether you trust the site

We are not talking about sites. You can use this to plug in local files only.

Comment 16

[Kai Engert [:KaiE:]]

I take that back, I momentarily ignored that this can be called using JavaScript.

Comment 17

[Kai Engert [:KaiE:]]

(In reply to comment #14)
> I think the addmodule dialog should just be the extension-installation dialog. 

Jesse, I can't find a way to easily reuse this dialog from within PSM.

While I found an interface that embeddors can use to replace our internal dialog with a different, it seems there is no interface to use XPInstall's  implementation from another XPCOM module.

Should somebody work on XPInstall to make it available using a new interface?

Comment 18

[Darin Fisher]

Given that bug 328771 has been fixed in FF 1.5.0.2, is there anything left here that's worth taking for FF 1.5.0.3?

Comment 19

[Kai Engert [:KaiE:]]

I don't know how I could easily implement Jesse's proposal to use the general software install dialog, as I can't find the necessary interfaces, and I think adding interfaces is not appropriate for 1.5.0.x. I guess nobody has suggestions to my comment 17?

If you confirm my understanding is right, we could go back to come up with better wordings, see comment 12 and comment 13.

Opinions?

Comment 20

[Darin Fisher]

We definitely want to minimize changes on the stable branch.  Interface changes are definitely not allowed, but interface additions are possible -- yet they should be avoided in general.  We also don't want to change the text that is localized because that involves too much coordination amongst the localizers.

Taking those into consideration, how important is it that we try to fix this for a 1.8.0.x release?  Given that the OK button has a timer on it, isn't most of the problem described in the original bug report avoided?  Perhaps we should simply focus on a better solution for FF2.

Comment 21

[Daniel Veditz [:dveditz]]

Would require l10n changes, not on 1.8.0 branch

Comment 22

[Mike Schroepfer]

Late in the game for 1.9 - minusing

Comment 23

[Lucas Adamski [:ladamski]]

From the comments its still not clear to me whether addmodule can result in remote installation of malicious code, or only load local modules. If the former then this is a critical bug (remote site can just force the user into an addmodule dialog loop).

Comment 24

[Benjamin Smedberg]

After discussion with dveditz, we don't understand why we should expose this functionality to content. We can keep the API accessible to extension authors, without needing any prompts or confirmation.

Comment 25

[Daniel Veditz [:dveditz]]

Combined with another bug that allows an attacker to force a download to a known location--and we've had those bugs in the past--this becomes a remote risk. Even without a "bug" the default download location is easily guessed for most windows users who run the "Home" versions of the OS.

I can't think of any legitimate reason for a _web page_ to need this functionality.

Comment 26

[Benjamin Smedberg]

Attachment: pkcs providers must not be installed by content, rev. 1

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 345353 [details] [diff] [review]
pkcs providers must not be installed by content, rev. 1

I agree, unless there's a *really* good reason Joe Webdeveloper needs the ability to install/remove modules here, this needs to go.

Comment 28

[Kai Engert [:KaiE:]]

I have asked several people by email whether this functionality is anything currently required/expected by products, and whether there is a need to veto this patch, or find a different approach. I don't expect to be one, but wanted to be certain. I've asked them to comment in the bug. I'll review the patch after I have feedback.

Comment 29

[Robert Relyea]

This feature was carried forward from the old Communicator 4.0 days. The JavaScript function was to aid PKCS #11 developers in building automatic Jar installation tools. Some also wrote 'web scripts' which would allow installation of PKCS #11 modules because this API more control over the configuration than the existing PCKS #11 installer UI built into PSM.

Moving this function from the crypto object to chrome should not cause problems in creating replacements for these tools. We just need to document how one would use an xpi file to install a PKCS #11 module, and we probably should write a plugin that imitates what those scripts used to do.

The crypto object wiki documentation should also be updated.

In short, since the patch retains the functionality, only moving it to Chrome, and since the number of these jar installers and scripts (I don't even know if the latter works anymore in the Firefox world) is almost certainly small and can be replaced, the benefits of this patch greatly outweigh the costs.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

To add to Bob's comment:  I believe the feature was intended to work in 
javascripts that were part of jar (now XPI) installers, not in ordinary 
web pages.  If the feature could be retained in XPI installers, but not 
in web pages, I think that would minimize the breakage.  I think we really
do not want to suddenly break all (or even most) of the XPI installers that
install PKCS#11 crypto modules into Firefox.  While relatively unknown in
the USA, these are widely used in some European countries.

Comment 31

[Benjamin Smedberg]

Just to follow up to comments via email: the window object was never available to XPI/JAR installers, and so this will not be a regression for them. And in any case, we no longer run scripts for extension installs.

Comment 32

[Benjamin Smedberg]

kaie: review-ping?

Comment 33

[Jesse Ruderman]

Bug 479652 has a beautiful exploit, so I think this should be made public and re-evaluated for blocking1.9.1.

Comment 34

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #33)
> Bug 479652 has a beautiful exploit, so I think this should be made public and
> re-evaluated for blocking1.9.1.

Renom blocking1.9.1 based on comment #33.

Comment 35

[Kai Engert [:KaiE:]]

Comment on attachment 345353 [details] [diff] [review]
pkcs providers must not be installed by content, rev. 1

I understand the functionality is risky and both Bob and Nelson are OK with the plan to remove the functionality for web sites. I agree that a prompt is no longer necessary to warn the user.

Patch looks good to me.

Should we remove the obsolete strings from the bundle?

We need to update the documentation as requested by Bob, announcing that this functionality will go away.

r=kaie

Comment 36

[Benjamin Smedberg]

Attachment: Branch patch, with possible issues

This is the branch patch. It appears to be fine, except I get the following warning when I run a build with this patch:

WARNING: nsDOMClassInfo::Finalize Don't call me!: file ../../../../src/dom/src/base/nsDOMClassInfo.cpp, line 4033

Is there some other aspect of dom classinfo that I need to keep in sync that I don't know about?

Comment 37

[Johnny Stenback (:jst)]

Uh, I can't see how this patch would have anything to do with finalize hooks in any way. Are you sure the error you're seeing is only apparent with this patch? I know there are some differences on trunk/branch regarding finalization of DOM stuff, so I would guess that what you're seeing is unrelated to this patch. If this is specific to this patch, can you find out what class we're finalizing when you see that error (by inspecting mData or what not)?

Comment 38

[Benjamin Smedberg]

Yeah, apparently that warning is entirely unrelated.

Pushed this to 1.9.1 without removing nsIDOMpkcs11.idl because that removal caused bug 487960 and bug 487980.

http://hg.mozilla.org/mozilla-central/rev/eea9639048b8 (trunk commit)
http://hg.mozilla.org/mozilla-central/rev/1aaacee9b2d0 (remove l10n strings which are no longer needed)

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/596f145b7002 (branch commit)

Comment 39

[Benjamin Smedberg]

Backed out from -central and -1.9.1 due to regression bug 494899

Comment 40

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

Do we want to do something wallpaperish here, like disable the dialog or only disable it in content-land? Or should we do that in bug 479652 and make that blocking?

Comment 41

[Johnathan Nightingale [:johnath]]

(In reply to comment #39)
> Backed out from -central and -1.9.1 due to regression bug 494899

I definitely understand backing it out, since breaking our dialogs isn't a good thing, and this isn't a good time to argue about what specific strings we should take/leave.  But this is still an sg:moderate blocker bug with an exploit.

Could we do something more reduced, like just removing the ability for content JS to invoke these, while preserving business as usual for chrome?  Benjamin, do you already have an alternative plan in mind?

Comment 42

[Benjamin Smedberg]

I'm sure there is a way to block content but not chrome from using this API, but I don't know what it is, and it would probably be simpler to remove the API again while solving the UI issues raised in bug 494899.

In any case I don't think this needs to be a release blocker at this point.

Comment 43

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

This is a release blocker due to the exploit (public) in bug 479652 - I'm actually interested in ensuring that we have solutions to that problem.

Comment 44

[Benjamin Smedberg]

Attachment: Rollup patch for pkcs providers and the device manager, rev. 2

This patch is the old patch with additional fixes requires by the device manager. This patch distinguishes between "duplicate module name" and "error installing device" so that the UI more closely matches the previous UI.

Comment 45

[Johnathan Nightingale [:johnath]]

Blew away some metadata when updating the whiteboard, fixing.

Comment 46

[Benjamin Smedberg]

http://build.mozilla.org/tryserver-builds/[e-mail]-try-2b6d6e045b1

Comment 47

[Kai Engert [:KaiE:]]

This patch doesn't provide the old diversity of error messages when removing a module. I think it's acceptable to drop the message about invalid module name, because that only is when the param is empty...

But I would have preferred to keep the old success message, that will either say "deleted internal" or "deleted external" module.

Why don't we simply keeping the old of code inside nsPkcs11::DeleteModule that shows up the success messages? We can revisit at a later time if we should drop them.

In other words, where the old code had:

   if (srv == SECSuccess) {
...
-    if (modType == SECMOD_EXTERNAL) {
-      nssComponent->GetPIPNSSBundleString("DelModuleExtSuccess", errorMessage);
-      *aReturn = JS_OK_DEL_EXTERNAL_MOD;
-    } else {
-      nssComponent->GetPIPNSSBundleString("DelModuleIntSuccess", errorMessage);
-      *aReturn = JS_OK_DEL_INTERNAL_MOD;
-    }

can we do this?

   if (srv == SECSuccess) {
...
     if (modType == SECMOD_EXTERNAL) {
       nssComponent->GetPIPNSSBundleString("DelModuleExtSuccess", errorMessage);
       alertUser(errorMessage.get());
     } else {
       nssComponent->GetPIPNSSBundleString("DelModuleIntSuccess", errorMessage);
       alertUser(errorMessage.get());
     }


Do you want to remove file nsIDOMPkcs11.idl ?
It seems in your new patch you keep it.

I leave it you to you address my proposal to keep the "2 different success messages on delete", but I'd really prefer it.

r=kaie

Comment 48

[Mike Shaver (:shaver emeritus)]

We have to keep the IDL file; see bug 487960 and bug 487980.

Why is it interesting to say that you deleted an internal vs. external module?  There's no decision to make at that point, AIUI, so I'm not sure what help it is to the user.

Comment 49

[Kai Engert [:KaiE:]]

(In reply to comment #48)
> Why is it interesting to say that you deleted an internal vs. external module? 
> There's no decision to make at that point, AIUI, so I'm not sure what help it
> is to the user.

I can't think of any good reason either, just trying to stick to old behavior as closely as possible.

Comment 50

[Johnny Stenback (:jst)]

Attachment: 1.9.1 version of the above.

Comment 51

[Johnny Stenback (:jst)]

Fix landed on trunk and 1.9.1.

http://hg.mozilla.org/mozilla-central/rev/4414591c887f
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/63d510563b10

Comment 52

[Sander Lepik]

Where is the documentation for a workaround?!? This came as a VERY BAD news for us. In Europe Estonia is not the only country using ID-cards. And removing this functionality without any info about workaround is a very-very bad practice!

https://ideelabor.ee/opensource/wiki/IdKaardiTarkvara/VeebisAutentimineMozillaga - this page is usless for Firefox 3.5. How can we do the same thing with xpi?

Dumbuser can't find even device manager.. How should they find correct module to load?!

I'm really waiting for GOOD documentation as this is a release blocker for Estonia and other countries using smart cards.

Comment 53

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

Sander, could you please file a new bug for that, and request blocking1.9.1?

Reopening a fixed bug is not the proper way to register the complaint (they're easy to miss, particularly since this one is already marked fixed1.9.1).

Comment 54

[Sander Lepik]

new bug: #495756

Comment 55

[Benjamin Smedberg]

Attachment: 1.9.0 version

1.9.0 branch patch

Comment 56

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

Comment on attachment 393779 [details] [diff] [review]
1.9.0 version

Approved for 1.9.0.14. a=ss

Comment 57

[Benjamin Smedberg]

Checking in dom/public/nsDOMClassInfoID.h;
/cvsroot/mozilla/dom/public/nsDOMClassInfoID.h,v  <--  nsDOMClassInfoID.h
new revision: 1.36; previous revision: 1.35
done
Checking in dom/src/base/nsDOMClassInfo.cpp;
/cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v  <--  nsDOMClassInfo.cpp
new revision: 1.553; previous revision: 1.552
done
Checking in dom/src/base/nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new revision: 1.1021; previous revision: 1.1020
done
Checking in dom/src/base/nsGlobalWindow.h;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.h,v  <--  nsGlobalWindow.h
new revision: 1.340; previous revision: 1.339
done
Checking in security/manager/pki/resources/content/device_manager.js;
/cvsroot/mozilla/security/manager/pki/resources/content/device_manager.js,v  <--  device_manager.js
new revision: 1.22; previous revision: 1.21
done
Checking in security/manager/ssl/public/Makefile.in;
/cvsroot/mozilla/security/manager/ssl/public/Makefile.in,v  <--  Makefile.in
new revision: 1.42; previous revision: 1.41
done
RCS file: /cvsroot/mozilla/security/manager/ssl/public/nsIPKCS11.idl,v
done
Checking in security/manager/ssl/public/nsIPKCS11.idl;
/cvsroot/mozilla/security/manager/ssl/public/nsIPKCS11.idl,v  <--  nsIPKCS11.idl
initial revision: 1.1
done
Checking in security/manager/ssl/src/nsCrypto.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsCrypto.cpp,v  <--  nsCrypto.cpp
new revision: 1.79; previous revision: 1.78
done
Checking in security/manager/ssl/src/nsCrypto.h;
/cvsroot/mozilla/security/manager/ssl/src/nsCrypto.h,v  <--  nsCrypto.h
new revision: 1.12; previous revision: 1.11
done

Fixed in CVS for 1.9.0.14

Comment 58

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.14 if the proper result is no dialog when doing javascript:pkcs11.deletemodule("foopy");

Can someone confirm that is the desired behavior?

Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.14pre) Gecko/2009081305 GranParadiso/3.0.14pre (.NET CLR 3.5.30729).