74340 - PSM2 should support the crypto object

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Bob Lord]

This version number will be a single string (e.g. "2.0") rather than the pair
that it used to be for showing the version of both the client and the server
(e.g. "1.2,1.4")

Comment 1

[Bob Lord]

Target -> 2.0

Comment 2

[John G. Myers]

From bug 75130:

1. Visit the above URL.
What is expected: The top line should read "PSM 2.0 is loaded".
What happens: The wording does not appear.
"document.write(crypto.version);" fails.

Comment 3

[John Unruh]

*** Bug 75233 has been marked as a duplicate of this bug. ***

Comment 4

[John Unruh]

This bug seems to be blocking bug 78401, preventing getting certs from CMS 4.2 - 
"No submit button appears on CMS 4.2"

Comment 5

[John Unruh]

Adding mozilla0.9.1 keyword.

Comment 6

[Javier Delgadillo]

Taking bug

Comment 7

[Javier Delgadillo]

Changing summar to better reflect the problem.

Comment 8

[Javier Delgadillo]

Attachment: Patch to support crypto object in PSM 2

Comment 9

[Ian McGreer]

review comments (by mcgreer)

Terry says we should remove all xmlns:html attributes from the XUL.

nsCrypto.cpp:

* in cryptojs_GetFunctionObjectPrincipal(), if jsClass is null, should the loop
terminate?

* should PSM_VERSION_STRING be #define'd somewhere else, where everybody can
access it?  Seems like it should show up in the UI, so that users can tell us
what version they are running.

* in nsCrypto::ImportUserCertificates, someone will probably complain that you
have a loser goto label that returns NS_OK


these are all take-it-or-leave-it, so r=mcgreer

Comment 10

[Javier Delgadillo]

Attachment: new patch w/ mcgreer's points taken.

Comment 11

[Javier Delgadillo]

requesting sr from blizzard.

Comment 12

[Christopher Blizzard (:blizzard)]

+  <button id="help-button" class="dialog" label="&help.label;"
+    style="width: 10ex" onclick="alert('Will bring up help one day');" /> 

Can you remove that button in the meantime and file a bug to re-add it later? 
There are few things more annoying than a button that pops up a reminder to
programmers. :)

+struct RunnableEvent : PLEvent {
+  RunnableEvent(nsIRunnable* runnable);
+  ~RunnableEvent();
+
+   nsIRunnable* mRunnable;
+};

I wonder if this will work on OS/2?  C++ callbacks are different there and even
though you are using the right callback types for the event ( PR_CALLBACK later
in the file ) it might cause problems.  I've never seen anyone else do a C++
class as the child of a PLEvent.  The docs say to use:

typedef struct foo { 
  PLEvent e;
  RunnableEvent *runnable;
} bar;

I suspect that the C++ type will work, though.  Hard to say.


+#if 1
+/*
+ * We're cheating for now so that escrowing keys on smart cards 
+ * will work.  The NSS team gave us their blessing to do this
+ * until they export a public function with equivalent functionality.
+ */

bug # for the new functionality?

+       rsaParams = (PK11RSAGenParams*)nsMemory::Alloc(sizeof(PK11RSAGenParams));

NS_STATIC_CAST, please.

+    src.data = (unsigned char*)authenticator;

same.

  (void*)subjectName);

I'm surprised that there isn't something like nsPrepareBitStringForEncoding
already in the NSS code somewhere.

+  PRUint32 argc;

I wonder if using the variable name 'argc' will cause problems since it's a
global.  I've seen this when I use the variable name 'socket.'  Can you fix that?

Also, you don't have any comments around any of these really obscure function
names so reading this code is difficult.  In the future please put some english
comments around the functions if possible.

Since this affects public JS apis does it need to go through jst or through a
security review from mstoltz?

Fix those little nits above and you have an sr=blizzard

Comment 13

[Javier Delgadillo]

Can you remove that button in the meantime and file a bug to re-add it later? 
There are few things more annoying than a button that pops up a reminder to
programmers. :)

>New bug is http://bugzilla.mozilla.org/show_bug.cgi?id=81048


+struct RunnableEvent : PLEvent {
+  RunnableEvent(nsIRunnable* runnable);
+  ~RunnableEvent();
+
+   nsIRunnable* mRunnable;
+};


>This is based on the code at
>http://lxr.mozilla.org/seamonkey/source/extensions/psm-glue/src/nsCrypto.cpp#91

>This will work on OS/2

+#if 1
+/*
+ * We're cheating for now so that escrowing keys on smart cards 
+ * will work.  The NSS team gave us their blessing to do this
+ * until they export a public function with equivalent functionality.
+ */

bug # for the new functionality?
>http://bugzilla.mozilla.org/show_bug.cgi?id=81037

+       rsaParams =
(PK11RSAGenParams*)nsMemory::Alloc(sizeof(PK11RSAGenParams));

NS_STATIC_CAST, please.

>Many NS_STATIC_CAST's added.

I'm surprised that there isn't something like nsPrepareBitStringForEncoding
already in the NSS code somewhere.

>There is, but it's not exported
>http://lxr.mozilla.org/mozilla/ident?i=PrepareBitStringForEncoding

+  PRUint32 argc;
>argc->myArgc
>argv->myArgv


Also, you don't have any comments around any of these really obscure function
names so reading this code is difficult.  In the future please put some english
comments around the functions if possible.

>Adding copious comments

Since this affects public JS apis does it need to go through jst or through a
security review from mstoltz?

>jst already reviewed.  He sent his comments via e-mail.   I sent off a message
>to mstoltz.

Comment 14

[Mitchell Stoltz (not reading bugmail)]

I don't think there's any security issue here on the JS level. If jst is OK with
this than so am I.

Comment 15

[Javier Delgadillo]

Fix checked in.

Comment 16

[John Unruh]

Verified.