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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: bsmedberg)

Tracking

(Blocks: 1 bug, 6 keywords)

Trunk
mozilla1.9alpha1
csectype-spoof, fixed1.9.0.14, fixed1.9.1, relnote, sec-moderate, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
wanted1.9.1 +
blocking1.9 -
blocking1.9.0.14 +
blocking1.8.0.2 -
blocking1.8.0.4 -
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Clicking "OK" in unclear dialog can lead to software installation])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Blocks: 326633
(Reporter)

Comment 1

12 years ago
pkcs11.deletemodule() probably has the same issues.

Comment 2

12 years ago
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.
(Reporter)

Comment 3

12 years ago
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.
Whoever wrote this interface missed the memo about JS (and Java, and Smalltalk) using camelCaps (so deleteModule, not deletemodule).  Grump.

/be
(Reporter)

Updated

12 years ago
Keywords: testcase

Comment 5

12 years ago
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.
(Reporter)

Comment 6

12 years ago
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

12 years ago
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.
(Reporter)

Updated

12 years ago
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.2?
OS: MacOS X → All
Hardware: Macintosh → All
Summary: Dialog for pkcs11.deletemodule is too jargony → Dialogs for pkcs11.addmodule and pkcs11.deletemodule are too jargony
Whiteboard: [sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation
(Reporter)

Updated

12 years ago
Depends on: 328771
(Reporter)

Comment 8

12 years ago
I split "protect against bug162020-style attacks" into bug 328771, since that's more urgent and might be easier to fix.
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.  
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.
Flags: blocking1.8.0.2? → blocking1.8.0.2-

Comment 11

12 years ago
> 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

12 years ago
> 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

12 years ago
I suggest changing "security module" to "crypto module"
or "cryptographic module".  The word "security" can mean
anything from anti-virus to crypto.
(Reporter)

Comment 14

12 years ago
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

12 years ago
> 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

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

Comment 17

12 years ago
(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

11 years ago
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

11 years ago
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

11 years ago
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.
Would require l10n changes, not on 1.8.0 branch
Flags: blocking1.8.0.3? → blocking1.8.0.3-

Updated

11 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
(Reporter)

Updated

10 years ago
Flags: blocking1.9?

Comment 22

10 years ago
Late in the game for 1.9 - minusing
Flags: blocking1.9? → blocking1.9-
(Reporter)

Updated

9 years ago
Flags: blocking1.9.1?
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).
(Assignee)

Comment 24

9 years ago
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.
Assignee: kaie → benjamin
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.
Assignee: benjamin → kaie
Assignee: kaie → benjamin
QA Contact: psm
(Assignee)

Comment 26

9 years ago
Created attachment 345353 [details] [diff] [review]
pkcs providers must not be installed by content, rev. 1
Attachment #345353 - Flags: superreview?(jst)
Attachment #345353 - Flags: review?(kaie)
Attachment #345353 - Flags: review?(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.
Attachment #345353 - Flags: superreview?(jst)
Attachment #345353 - Flags: superreview+
Attachment #345353 - Flags: review?(jst)
Attachment #345353 - Flags: review+

Comment 28

9 years ago
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

9 years ago
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.
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.
(Assignee)

Comment 31

9 years ago
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.
(Assignee)

Comment 32

9 years ago
kaie: review-ping?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
(Reporter)

Comment 33

8 years ago
Bug 479652 has a beautiful exploit, so I think this should be made public and re-evaluated for blocking1.9.1.
(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.
Flags: blocking1.9.1- → blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: P2 → P1
(Assignee)

Updated

8 years ago
Whiteboard: [sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation → [sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation. [need review kaie]

Comment 35

8 years ago
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
Attachment #345353 - Flags: review?(kaie) → review+
(Assignee)

Comment 36

8 years ago
Created attachment 372114 [details] [diff] [review]
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?
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)?

Updated

8 years ago
Depends on: 487960

Updated

8 years ago
Depends on: 487980
Whiteboard: [sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation. [need review kaie] → [sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation. [branch patch has issues?]
(Assignee)

Comment 38

8 years ago
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)
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite-
Keywords: fixed1.9.1
Resolution: --- → FIXED
(Reporter)

Updated

8 years ago
Depends on: 494899
(Assignee)

Comment 39

8 years ago
Backed out from -central and -1.9.1 due to regression bug 494899
Status: RESOLVED → REOPENED
Keywords: fixed1.9.1
Resolution: FIXED → ---
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?
(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?
(Assignee)

Comment 42

8 years ago
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.
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.
(Assignee)

Updated

8 years ago
Group: core-security
Blocks: 479652
(Assignee)

Comment 44

8 years ago
Created attachment 379949 [details] [diff] [review]
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.
Attachment #345353 - Attachment is obsolete: true
Attachment #372114 - Attachment is obsolete: true
Attachment #379949 - Flags: review?(kaie)
No longer blocks: 479652
Group: core-security
Whiteboard: [sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation. [branch patch has issues?] → [needs review kaie][sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation.
Blew away some metadata when updating the whiteboard, fixing.
Blocks: 479652
Group: core-security
(Assignee)

Comment 46

8 years ago
http://build.mozilla.org/tryserver-builds/bsmedberg@mozilla.com-try-2b6d6e045b1

Comment 47

8 years ago
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

Updated

8 years ago
Whiteboard: [needs review kaie][sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation. → [sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation.

Updated

8 years ago
Attachment #379949 - Flags: review?(kaie) → review+
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

8 years ago
(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.
Whiteboard: [sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation. → [has reviewed patch][sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation.
Created attachment 380309 [details] [diff] [review]
1.9.1 version of the above.
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
Keywords: fixed1.9.1

Updated

8 years ago
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED

Comment 52

8 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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).
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED

Comment 54

8 years ago
new bug: #495756
(Reporter)

Updated

8 years ago
Depends on: 495756

Updated

8 years ago
Keywords: relnote
(Assignee)

Comment 55

8 years ago
Created attachment 393779 [details] [diff] [review]
1.9.0 version

1.9.0 branch patch
Attachment #393779 - Flags: approval1.9.0.15?
Attachment #393779 - Flags: approval1.9.0.14?
Flags: blocking1.9.0.14+
Comment on attachment 393779 [details] [diff] [review]
1.9.0 version

Approved for 1.9.0.14. a=ss
Attachment #393779 - Flags: approval1.9.0.15?
Attachment #393779 - Flags: approval1.9.0.14?
Attachment #393779 - Flags: approval1.9.0.14+
(Assignee)

Comment 57

8 years ago
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
Keywords: fixed1.9.0.14
Blocks: 509413
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).
(Assignee)

Updated

8 years ago
Duplicate of this bug: 513243
(Reporter)

Updated

4 years ago
Keywords: csec-spoof, sec-moderate
Whiteboard: [has reviewed patch][sg:moderate?] Clicking "OK" in unclear dialog can lead to software installation. → [Clicking "OK" in unclear dialog can lead to software installation]
You need to log in before you can comment on or make changes to this bug.