Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 649154 (domcrypt)

Implement DOMCrypt (Internal) API

RESOLVED WONTFIX

Status

()

Core
DOM
RESOLVED WONTFIX
6 years ago
4 years ago

People

(Reporter: ddahl, Assigned: ddahl)

Tracking

(Blocks: 4 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sec-assigned:curtisk:744938], URL)

Attachments

(6 attachments, 44 obsolete attachments)

131.55 KB, patch
Details | Diff | Splinter Review
3.23 KB, text/plain
Details
2.83 KB, text/plain
Details
11.03 KB, text/plain
Details
12.37 KB, text/plain
Details
63.26 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Port DOMCrypt Firefox extension into mozilla-central as a DOM property. see https://github.com/daviddahl/domcrypt/ and http://domcrypt.org This will initially be preffed off as we get feedback from web developers, endusers and work out the possible use of crypto standards or look into standardization.

Firefox Project page is here: https://wiki.mozilla.org/DOMCryptAPI

DOMCrypt currently has a simple, 'webby' API:

crypt.generateKeyPair()
crypt.encrypt()
crypt.decrypt()
crypt.prompDecrypt()
crypt.makeHash()
crypt.getPubKey()
crypt.sign()
crypt.verify()

All of this API wraps a js-ctypes wrapping of NSS. (A former incarnation of WeaveCrypto.js)

There is also an "AddressbookManager" API that allows endusers to exchange public keys (or 'addressbook entries') via a simple meta-tag / notification prompt

crypt.getAddressbook()

Internally, DOMCrypt sniffs out addressbook-entry meta tags and prompts the user to save the contents of this tag to the addressbook.

==================================
Improvements that need to be made:

1. Move all crypto API work to a ChromeWorker with a callback that is executed in a document sandbox

2. Namespace this API so additional APIs can be added and future standardization issues can be dealt with. I think the current API should reside under crypt.pk.*

3. decide on a better name for the property. crypt is too close to crypto, which is already a property of the window.

4. a comprehensive browser test suite
(Assignee)

Updated

6 years ago
Assignee: nobody → ddahl

Comment 1

6 years ago
You're aware of window.crypto, right?  I think adding a separate window.crypt will be pretty confusing to web authors...
(Assignee)

Comment 2

6 years ago
(In reply to comment #1)
> You're aware of window.crypto, right?  I think adding a separate window.crypt
> will be pretty confusing to web authors...

Yes. in my extension I called it crypt, it was just a placeholder name. A new name is a top priority.
Has this been discussed with any other browser vendors?
(Assignee)

Comment 4

6 years ago
(In reply to comment #3)
> Has this been discussed with any other browser vendors?

Not yet, but it will be.
(Assignee)

Comment 5

6 years ago
(In reply to comment #0)
> Firefox Project page is here: https://wiki.mozilla.org/DOMCryptAPI

Moved this page to https://wiki.mozilla.org/Privacy/Features/DOMCryptAPI

and on the roadmap here: https://wiki.mozilla.org/Privacy/Roadmap_2011#Enhance_User_Controlled_Disclosure
(Assignee)

Comment 6

6 years ago
Created attachment 525624 [details] [diff] [review]
v 0.1 WIP saving work

Getting things working so that the property is available to content and the jsm is available to addons and we are using a worker for heavy lifting
(Assignee)

Comment 7

6 years ago
Created attachment 525922 [details] [diff] [review]
v 0.2 WIP - NSS is running in a ChromeWorker now

generateKeypair is working off main thread
Attachment #525624 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
I have noticed a single ASSERT when calling the generateKeyPair method in the worker:

###!!! ASSERTION: Wrong thread!: 'NS_IsMainThread()', file /home/ddahl/code/moz/mozilla-central/dom/src/threads/nsDOMWorker.cpp, line 929

http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMWorker.cpp#929

However, it all works, there is no crash, and other NSS methods wrapped with ctypes do not assert.

Comment 9

6 years ago
That assert doesn't look happy (in the "probably exploitable if you try" sense).
(Assignee)

Comment 10

6 years ago
Some more context:

I create the worker in the jsm on import:

var workerFactory = Cc["@mozilla.org/threads/workerfactory;1"].
                      createInstance(Ci.nsIWorkerFactory);

var worker = workerFactory.newChromeWorker("domcrypt_worker.js");

Now that I put in some logging messages, I see that this ASSERT happens before I even call any of the worker's methods:

###!!! ASSERTION: Wrong thread!: 'NS_IsMainThread()', file /home/ddahl/code/moz/mozilla-central/dom/src/threads/nsDOMWorker.cpp, line 929
*** DOMCryptMethods: save generateKeypairCallback
*** DOMCryptMethods: Calling the Worker...
WeaveCrypto: Worker started

It seems like this happens even before I init NSS - I will attach a backtrace
(Assignee)

Comment 11

6 years ago
Created attachment 526080 [details]
Worker Assert 'NS_IsMainThread()'

Backtrace
It's not clear to me whether or not this is a problem yet. Basically it's fallout from our patch to use the tracing API to 'un-gray' XPConnect objects.
(Assignee)

Comment 13

6 years ago
(In reply to comment #12)
> It's not clear to me whether or not this is a problem yet. Basically it's
> fallout from our patch to use the tracing API to 'un-gray' XPConnect objects.

ok, let me know if you need a test case or anything else to help figure it out.
(Assignee)

Comment 14

6 years ago
Created attachment 526183 [details] [diff] [review]
v 0.3 WIP Filling in the rest of the API
Attachment #525922 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
Created attachment 526459 [details] [diff] [review]
v 0.4 All Crypto API methods now work

Next: test suite
Attachment #526183 - Attachment is obsolete: true
(Assignee)

Comment 16

6 years ago
still working on the test suite - experiencing a crash in a browser chrome test:

http://pastebin.mozilla.org/1207414

Somewhat unclear why I am getting this:

WARNING: NS_ENSURE_SUCCESS(result, result) failed with result 0x80004005: file /home/ddahl/code/moz/mozilla-central/editor/libeditor/base/nsEditor.cpp, line 3927
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file /home/ddahl/code/moz/mozilla-central/editor/libeditor/text/nsTextEditRules.cpp, line 471
Assertion failure: wrong tag, at /home/ddahl/code/moz/mozilla-central/js/src/jsgc.cpp:694

// debugging just ensued...

Looks like this may have had to do with my calling Cu.forceGC at an inopportune moment. It is not crashing any longer. The strange thing is that the content script I have written as a manual test did not exhibit this problem. 

The dialog is still not accepted however.
(Assignee)

Comment 17

6 years ago
Created attachment 526933 [details] [diff] [review]
v 0.5 Test suite coming along - most methods are tested
Attachment #526459 - Attachment is obsolete: true
(Assignee)

Comment 18

6 years ago
Created attachment 527188 [details] [diff] [review]
v 0.6 API is fully tested now - need to test 3 additional methods
Attachment #526933 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
Created attachment 527713 [details]
Occasional crash when showing a modal dialog

backtrace: Every 20 times (or so) I run a manual test that quickly runs through the entire API, I see a crash having to do with the modal dialog shown before the keypair is generated
(Assignee)

Comment 20

6 years ago
Created attachment 527720 [details] [diff] [review]
v 0.7 Added prompt tests

Almost ready for review, needs another test. current TODO list:

1. start using passphrase() getter * done *
2. test additional prompts * done *
3. handle error conditions in the worker more gracefully * done *
4. move to NetUtils and Fileutils for all filesystem access
5. decrypt should check the passphrase cache first - test needs to be written for this where we set the TTL to 1 second
6. change API property name to mozCipher from cipher?
7. expose validate passphrase method
Attachment #527188 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 527944 [details] [diff] [review]
v 0.8 refactored callback storage

Currently hunting down a leak I have introduced
Attachment #527720 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
Created attachment 528262 [details] [diff] [review]
v 0.8.1 Crushed the leaks, yay
Attachment #527944 - Attachment is obsolete: true
(Assignee)

Comment 23

6 years ago
Created attachment 528782 [details] [diff] [review]
v 0.8.2 async file issues

In my attempt to make all file reading and writing async, I just cannot figure out the best way to initialize the window property - I also think there is something wrong with my file writing function in DOMCryptMethods.jsm

You can start a manual test of the API by loading this file: obj-dir/_tests/testing/mochitest/browser/dom/tests/browser/test-domcrypt.html
Attachment #527713 - Attachment is obsolete: true
Attachment #528262 - Attachment is obsolete: true
Attachment #528782 - Flags: feedback?(sdwilsh)
Comment on attachment 528782 [details] [diff] [review]
v 0.8.2 async file issues

Review of attachment 528782 [details] [diff] [review]:

I didn't look at the tests, but here's your high-level feedback.  I think you are headed in the right direction, but there are some API issues that need to get worked out before this is feedback+able.

::: browser/app/profile/firefox.js
@@ +1009,5 @@
 // Whether the Panorama should animate going in/out of tabs
 pref("browser.panorama.animate_zoom", true);
+
+// DOMCrypt prefs
+pref("dom.domcrypt.enabled", true);

remember that you'll want this to be false in your final patch

@@ +1010,5 @@
 pref("browser.panorama.animate_zoom", true);
+
+// DOMCrypt prefs
+pref("dom.domcrypt.enabled", true);
+pref("dom.domcrypt.passphraseTTL", 3600000);

this needs a comment about what it's for and the units it is in

::: dom/base/DOMCrypt.js
@@ +12,5 @@
+ * License.
+ *
+ * The Original Code is DOMCrypt API code.
+ *
+ * The Initial Developer of the Original Code is Mozilla Foundation

global-nit: per https://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c, "the Mozilla Foundation." should be on a newline.

@@ +113,5 @@
+   * This method sets up the crypto API and returns the object that is
+   * accessible from the DOM
+   *
+   * @param nsIDOMWindow aWindow
+   * @returns object

global-nit: need more documentation on all the @param comments.  I also don't think repeating the method name in the comment right above the method is terribly useful.

@@ +126,5 @@
+                              { sandboxPrototype: this.window, wantXrays: false });
+
+    Services.obs.addObserver(this, "inner-window-destroyed", false);
+
+    // TODO: change the property name to mozCipher?

We absolutely need to prefix it.  We may want to look into extending the current DOM property that bz mentioned in comment 1.

::: dom/base/DOMCryptMethods.jsm
@@ +46,5 @@
+XPCOMUtils.defineLazyServiceGetter(this, "promptSvc",
+                                   "@mozilla.org/embedcomp/prompt-service;1",
+                                   "nsIPromptService");
+
+XPCOMUtils.defineLazyServiceGetter(this, "SDR",

I would prefer a more descriptive variable name here.

@@ +69,5 @@
+ * @returns string
+ */
+function getStr(aName)
+{
+  return stringBundle.GetStringFromName(aName);

You probably don't want to have to grab these each time you want to use them because that may cause disk I/O (if I recall correctly).  You can memoize them as you need them by copying code like this:
https://hg.mozilla.org/mozilla-central/annotate/ec809c159ad2/toolkit/mozapps/downloads/DownloadUtils.jsm#l103

@@ +103,5 @@
+// We can call ChromeWorkers from this JSM via nsIWorkerFactory
+var workerFactory = Cc["@mozilla.org/threads/workerfactory;1"].
+                      createInstance(Ci.nsIWorkerFactory);
+
+var worker = workerFactory.newChromeWorker("domcrypt_worker.js");

I highly suspect you'll want to do this work lazily as there is no reason to create this on the first import of the JSM.  You can make DOMCryptMethods a lazy getter and do this work at the first access.  Or even better, do it the first time you need to use the worker (so make worker a lazy getter).

@@ +107,5 @@
+var worker = workerFactory.newChromeWorker("domcrypt_worker.js");
+
+worker.onmessage = function DCM_worker_onmessage(aEvent) {
+  switch (aEvent.data.action) {
+  case "keypairGenerated":

All of your actions here should be constants instead of magic strings.

@@ +108,5 @@
+
+worker.onmessage = function DCM_worker_onmessage(aEvent) {
+  switch (aEvent.data.action) {
+  case "keypairGenerated":
+    DOMCryptMethods.handleGenerateKeypair(aEvent.data.keypairData);

All of your handle* methods are on DOMCryptMethods, but they really should only be used internally, correct?  In general, any helper methods should not be on the object you export.  This holds true with properties you use to store things (such as the callbacks).  NetUtil.jsm keeps these things in the global scope which prevents them from being exported.

@@ +165,5 @@
+ *
+ * DOMCryptMethods' onmessage chooses which callback to execute in the original
+ * content window's sandbox.
+ */
+var DOMCryptMethods = {

A bunch of methods on this object need comments explaining what they do and what their arguments are.

@@ +190,5 @@
+  },
+
+  callbacks: null,
+
+  registerCallback: function DCM_registerCallback(aLabel, aCallback, aSandbox)

I think it's overly clunky to make consumers of this have to worry about this themselves.  Each method that takes a callback should be able to manage this itself (and this means each of those methods should also grow an aCallback parameter).

@@ +202,5 @@
+  },
+
+  /**
+   * wrap the content-provided script in order to make it easier
+   * to import and run in the sandbox

This does not seem to match what this method is doing at all.

@@ +208,5 @@
+   * @param string aPubKey
+   * @returns function
+   */
+  makeGenerateKeypairCallback:
+  function DA_makeGenerateKeypairCallback(aPubKey)

The prefix on this method does not match the rest.

@@ +215,5 @@
+    let callback = function generateKeypair_callback()
+                   {
+                     self.callbacks.generateKeypair.callback(aPubKey);
+                   };
+    return callback;

FWIW, instead of using self, you can just say |return callback.bind(this);|

@@ +471,5 @@
+    let prompt =
+      promptSvc.promptPassword(this.callbacks.generateKeypair.sandbox.window,
+                               getStr("enterPassphraseTitle"),
+                               getStr("enterPassphraseText"),
+                               passphrase, null, { value: false });

I suspect we don't want to default to using the promptservice for this (although it'd be a good fallback).  Talk to dolske about what to use here.

@@ +503,5 @@
+   */
+  generateKeypair: function DCM_generateKeypair(aPassphrase)
+  {
+    worker.postMessage({ action: "generateKeypair", passphrase: aPassphrase });
+    this.passphraseCache.encryptedPassphrase = SDR.encryptString(aPassphrase);

Do we really want to encrypt something on the main thread?

@@ +527,5 @@
+   * @param string aPlainText
+   * @param string aPublicKey
+   * @returns void
+   */
+  encrypt: function DCM_encrypt(aPlainText, aPublicKey)

You should verify that you are getting what you expect in your arguments and if not throw an error (with the call stack pointing to the caller).  See https://hg.mozilla.org/mozilla-central/annotate/1156b1885571/netwerk/base/src/NetUtil.jsm#l289 for an example on how to do that.  You should also test all the error checking you introduce to make sure you throw errors as expected.

@@ +803,5 @@
+    // get profile directory
+    let file = Cc["@mozilla.org/file/directory_service;1"].
+                 getService(Ci.nsIProperties).
+                 get("ProfD", Ci.nsIFile);
+    file.append(".domcrypt.json");

Use FileUtils.getFile (https://hg.mozilla.org/mozilla-central/annotate/b15803d8aee8/toolkit/mozapps/shared/FileUtils.jsm#l61)

@@ +849,5 @@
+      let converter = Cc["@mozilla.org/intl/converter-output-stream;1"].
+                        createInstance(Ci.nsIConverterOutputStream);
+      converter.init(foStream, "UTF-8", 0, 0);
+      converter.writeString(data);
+      converter.close();

https://developer.mozilla.org/en/Code_snippets/File_I%2f%2fO#Write_a_string but you may not want to use the safe file output stream depending on what it actually being stored here.

@@ +854,5 @@
+    }
+    catch (ex) {
+      log(ex);
+      log(ex.stack);
+    }

What exactly will error here?

@@ +904,5 @@
+                    createInstance(Ci.nsIFileInputStream);
+    let cstream = Cc["@mozilla.org/intl/converter-input-stream;1"].
+                    createInstance(Ci.nsIConverterInputStream);
+    fstream.init(aFile, -1, 0, 0);
+    cstream.init(fstream, "UTF-8", 0, 0);

https://developer.mozilla.org/en/Code_snippets/File_I%2f%2fO#Read_into_a_stream_or_a_string but see the section *right* below it too.

@@ +951,5 @@
+ */
+function initializeDOMCrypt()
+{
+  let fileCreated = false;
+  let file = DOMCryptMethods.configurationFile(fileCreated);

This isn't going to work like you think it is.  In fact, fileCreated is always going to be false.  If you want to pass by reference, you have to pass in an object ({value: false}) and then check the value property after the call.

@@ +984,5 @@
+{
+  let fileCreated = false;
+  let file = DOMCryptMethods.configurationFile(fileCreated);
+
+  let ostream = FileUtils.openSafeFileOutputStream(file);

Depending on what you are saving here and how much you care about it not being corrupted, you may not want to use a safe file output stream.

::: dom/base/domcrypt_worker.js
@@ +46,5 @@
+ * WeaveCrypto's API as it was released in Firefox 4 was reduced in scope due
+ * to Sync's move to J-Pake, hence the need for this more complete version.
+ *
+ * This version has the additional APIs 'sign' and 'verify' and has been
+ * edited for use in a ChromeWorker.

I think you should consider renaming this object now.
Attachment #528782 - Flags: feedback?(sdwilsh) → feedback-
(Assignee)

Comment 25

6 years ago
(In reply to comment #24)
> Comment on attachment 528782 [details] [diff] [review] [review]

> @@ +126,5 @@
> +                              { sandboxPrototype: this.window, wantXrays:
> false });
> +
> +    Services.obs.addObserver(this, "inner-window-destroyed", false);
> +
> +    // TODO: change the property name to mozCipher?
> 
> We absolutely need to prefix it.  We may want to look into extending the
> current DOM property that bz mentioned in comment 1.
> 
Was bz suggesting a consolidation, or a new name? I would prefer just calling it "window.mozCipher" for now until more feedback comes in from the community and other browser makers.

> @@ +190,5 @@
> +  },
> +
> +  callbacks: null,
> +
> +  registerCallback: function DCM_registerCallback(aLabel, aCallback,
> aSandbox)
> 
> I think it's overly clunky to make consumers of this have to worry about
> this themselves.  Each method that takes a callback should be able to manage
> this itself (and this means each of those methods should also grow an
> aCallback parameter).
> 
Indeed.

This is a half-measure right now. I am trying to take this pattern and automate it. I also had some weird crashes happen a few times when setting the callback inside the main api method. Not sure why. It was intermittent and may have been unrelated. In any case, this does need to be made simpler.

> @@ +471,5 @@
> +    let prompt =
> +     
> promptSvc.promptPassword(this.callbacks.generateKeypair.sandbox.window,
> +                               getStr("enterPassphraseTitle"),
> +                               getStr("enterPassphraseText"),
> +                               passphrase, null, { value: false });
> 
> I suspect we don't want to default to using the promptservice for this
> (although it'd be a good fallback).  Talk to dolske about what to use here.
> 
The only thing I am concerned about is not allowing any passphrases to be typed into content, that would be bad.
CCing dolske on this.

Comment 26

6 years ago
> Was bz suggesting a consolidation, or a new name? 

Either is fine offhand, but having both crypto and (unprefixed) crypt would be bad.

Heck, crypto and mozCrypto together would be fine by me.
(Assignee)

Comment 27

6 years ago
Created attachment 530802 [details] [diff] [review]
v 0.9 Most of sdwilsh's feedback is resolved

A few more documentation/comment tweaks and a new test file for testing bad input and I will ask for review
Attachment #528782 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Summary: Implement DOMCrypt 'window.crypt' as a mozilla DOM property → Implement 'window.mozCipher' as a mozilla DOM property
(Assignee)

Comment 28

6 years ago
Created attachment 531768 [details] [diff] [review]
v 0.9.1 most comments addressed

Most of your pre-feedback comments are addressed. I still need to write a couple of tests that prove bad arguments will just cause the API to throw and not introduce garbage into the worker's methods. 

I also have not gotten the write config to disk via async methods working. I think the problem is in the lazy nature of an nsIDOMGlobalPropertyInitializer. Perhaps all of the configuration needs to be taken care of in the DOMCryptAPI.init() method. I'm not sure if that will work either. 

Feel free to toggle this back to feedback instead of review.
Attachment #530802 - Attachment is obsolete: true
Attachment #531768 - Flags: review?(sdwilsh)
(Assignee)

Updated

6 years ago
Blocks: 657432
Is there a spec for this, preferably one that's on a standards track?
(Assignee)

Comment 30

6 years ago
(In reply to comment #29)
> Is there a spec for this, preferably one that's on a standards track?

Not formally. I will write one up ASAP. Should I just publish it on the moz wiki - for now? As far as getting this on a standards track, I really need to sit down and learn the best way to do this. This part of software engineering is all new to me.
I think this is a fantastic area to explore, and would be fine as an addon (which it was, originally?), but I think this need additional project work before we talk about landing it Firefox.

A few specific concerns:

* No spec. Need one for other browsers to consider for implementation, and early adopters would need one to refer to.

* Needs a security design review. Since it's pretty fundamentally a security feature, this should happen before landing (and not as a post-landing checkup, as most features do).

* If you haven't already, ought to solicit input from interested parties (community), related stakeholders (NSS/PSM, possibly Sync), and probably relevant standards group (not sure who these would be, but a proposal on WHAGWG / some W3C group would probably be a start).

* Needs a legal review, wrt to potential issues with shipping a crypto API. Shouldn't be a big deal, AFAIK, but needs to happen.

I think people will (fairly) assume that if Mozilla starts shipping DOM APIs, even moz-prefixed, there is a plan to standardize them. And I suspect you'll find there a lot of people with an opinion on that. ;) I'm not trying to throw stop-energy at this, just that if we're going to do this it should be done with due diligence.
Comment on attachment 531768 [details] [diff] [review]
v 0.9.1 most comments addressed

I'm actually going to hold off on review until comment 31 is addressed.
Attachment #531768 - Flags: review?(sdwilsh)
(Assignee)

Comment 33

6 years ago
(In reply to comment #31)
> A few specific concerns:
> 
> * No spec. Need one for other browsers to consider for implementation, and
> early adopters would need one to refer to.
> 
definitely is something I am beginning to address now. I will publish a spec on our wiki this week. 

> * Needs a security design review. Since it's pretty fundamentally a security
> feature, this should happen before landing (and not as a post-landing
> checkup, as most features do).
Yep. I have already spoken with the security team about this and the fact that this should not just be a normal security code review. 

> 
> * If you haven't already, ought to solicit input from interested parties
> (community), related stakeholders (NSS/PSM, possibly Sync), and probably
> relevant standards group (not sure who these would be, but a proposal on
> WHAGWG / some W3C group would probably be a start).

I have already presented this idea to both members of the Sync team, content team, A member of the Carnegie Mellon web security research group (http://websec.sv.cmu.edu/),  and the main NSS maintainers. I then sat down with Kai Engert and talked at length about the intersection of this code and the various psm APIs. 

People were pretty stoked about this concept, but like your concerns, we have only ever spent time on technical issues. You are correct - a formalized spec will help me get to the next step of presenting this to other browser vendors.

> 
> * Needs a legal review, wrt to potential issues with shipping a crypto API.
> Shouldn't be a big deal, AFAIK, but needs to happen.
>
No doubt.
 
> I think people will (fairly) assume that if Mozilla starts shipping DOM
> APIs, even moz-prefixed, there is a plan to standardize them. And I suspect
> you'll find there a lot of people with an opinion on that. ;) 
Indeed. Opinions vary greatly.

> I'm not trying
> to throw stop-energy at this, just that if we're going to do this it should
> be done with due diligence.
Not at all. I very much appreciate your guidance here, this is going to be the trickiest part of this project. My emphasis on working code is taking a play from the way the audio APIs evolved into such amazing tools. I think we have to really wow people with some working demos before we take that deep dive and wrangle over algorithms and whatnot. 

One thing of note: It looks somewhat likely that the Identity VerifiedEmailProtocol ( https://wiki.mozilla.org/Identity/Verified_Email_Protocol/Latest ) may use the Chrome API in this patch (or variant) for Key Generation and signing of data.  In which case, we can segment this further into chrome-only bits and eventually the content API.
(Assignee)

Updated

6 years ago
(Assignee)

Comment 34

6 years ago
(In reply to comment #33)
> > * No spec. Need one for other browsers to consider for implementation, and
> > early adopters would need one to refer to.
> > 
> definitely is something I am beginning to address now. I will publish a spec
> on our wiki this week. 

I have created a draft spec: https://wiki.mozilla.org/Privacy/Features/DOMCryptAPISpec/Latest
Can you please email it to whatwg@lists.whatwg.org?
(Assignee)

Comment 36

6 years ago
(In reply to comment #35)
> Can you please email it to whatwg@lists.whatwg.org?

Ah, yes, I will do that as soon as I have ported the newer, async API back over to the DOMCrypt extension. In a day or 2 at the latest.
(Assignee)

Comment 37

6 years ago
(In reply to comment #35)
> Can you please email it to whatwg@lists.whatwg.org?
Done.

Comment 38

6 years ago
Just want to throw a minor suggestion (which i've already mentioned to David) into the mix... as far as the vendor-prefixing part of this idea is concerned:

Bug #652140 documents an enhancement suggestion for creating a `window.mozilla` (or `Moz` or `Gecko` or whatever) property, for two specific purposes. Related to *this* thread, the main purpose would be that this object could be a better namespace to hang experimental (not-yet-standardized) APIs off of, so it would be like `mozilla.crypto` or whatever, until such a time as it graduates to non-prefixed and a more standard location, such as `window` or whatever.

I like the idea of this crypto lib making it in, and I think it's a great use-case candidate for the vendor-prefixing/namespacing idea I put forth in that other bug, so perhaps we should consider doing them both at the same time?

I dislike the idea of doing vendor-prefixing (of any JavaScript API) by putting "Moz" into the name of the API, only to later remove it by a full rename, as opposed to using the much more common pattern of object namespacing like `Moz.crypto`. As I mentioned in that other thread, when the feature does graduate to non-prefixed status, the former `Moz.crypto` location could remain as an alias to the new location of the API, as an aid in the migration of code from vendor-prefixed to non-vendor-prefixed.
(Assignee)

Comment 39

6 years ago
(In reply to comment #38)
> I dislike the idea of doing vendor-prefixing (of any JavaScript API) by
> putting "Moz" into the name of the API

Until and if there is a standard, I think this is the way it is done. There is also talk in this bug and on the WHAT-WG list of adding this api to window.crypto or consolidation of some sort - which would cause less confusion.

Comment 40

6 years ago
(in reply to comment #39)

>> I dislike the idea of doing vendor-prefixing (of any JavaScript API) by
>> putting "Moz" into the name of the API
>
> Until and if there is a standard, I think this is the way it is done

Firstly, I absolutely agree with the need to do some sort of vendor-prefixing. It's the *manner* in which we go about that which my suggestion is going towards. 

Secondly, "it is done" suggests (unless I read too much into it) that this is just how it's always been done, which is fine, I'm not taking issue with the past. I'm simply suggesting what I think would be a better pattern going forward to accomplish the same goal: vendor-prefixing before standardization.

In the JavaScript world, it's much more common of a pattern to use object property namespacing for this sort of thing, rather than baking in some set of characters into every function/object name. This pattern is also a little bit more friendly for future-refactoring, again from a JavaScript content developer's point of view.
(Assignee)

Comment 41

6 years ago
Update: The WHAT-WG discussion is being archived here: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-May/031741.html
(Assignee)

Comment 42

6 years ago
Created attachment 537418 [details]
WIP WebIDL of current proposed spec

Just getting my head around WebIDL, I am sure a lot is missing yet like exceptions and wrong, as in how I handle callbacks
Created attachment 537428 [details]
WebIDL

I was thinking something more like this
(Assignee)

Comment 44

6 years ago
Comment on attachment 537428 [details]
WebIDL

Ah, cool. Looks like the 'module' bits are unneeded. Do we need to specify how each method should throw?
(Assignee)

Comment 45

6 years ago
Updated the title of this bug as the spec is changing, also, the WebKit bug for this API is here: https://bugs.webkit.org/show_bug.cgi?id=62010
Summary: Implement 'window.mozCipher' as a mozilla DOM property → Implement 'window.mozCrypto' as a mozilla DOM property
(Assignee)

Updated

6 years ago
Blocks: 669927

Updated

6 years ago
Duplicate of this bug: 542879

Updated

6 years ago
Blocks: 673923
(Assignee)

Comment 47

6 years ago
Created attachment 549120 [details]
v 0.1 Chatter Bookmarklet - WIP

Saving work - this is a bookmarklet demo that will allow OTR style chatting on any chat web site
(Assignee)

Updated

6 years ago
Summary: Implement 'window.mozCrypto' as a mozilla DOM property → Implement DOMCrypt API
(Assignee)

Updated

6 years ago
Duplicate of this bug: 686249
Keywords: dev-doc-needed
Alias: domcrypt
Duplicate of this bug: 250792
(Assignee)

Comment 50

6 years ago
Created attachment 578449 [details]
v 1 Psuedocode

Brian:

This is some pseudo-code I put together while researching all of the APIs I need to use to begin implementing this API. We should meet up (soon) when you have some time to go over it.
Attachment #578449 - Flags: feedback?(bsmith)
(Assignee)

Comment 51

6 years ago
Created attachment 582429 [details] [diff] [review]
domcrypt-platform-generate-keypair v 1

Saving work, not built or tested
Attachment #578449 - Attachment is obsolete: true
Attachment #578449 - Flags: feedback?(bsmith)
(Assignee)

Comment 52

6 years ago
Created attachment 587729 [details] [diff] [review]
domcrypt-platform-generate-keypair v 1.1
Attachment #582429 - Attachment is obsolete: true
(Assignee)

Comment 53

6 years ago
Created attachment 588202 [details] [diff] [review]
v. 2 - it builds!

We now have functions to generate keys - creating an opaque 'nsIDOMCryptAPIKeyPair'. The keys have a 'nickname/keyID' associated with them. There is also a new function that gets the nsIDOMCryptAPIKeyPair via it's nickname/keyID.
Attachment #587729 - Attachment is obsolete: true
Attachment #588202 - Flags: feedback?(bsmith)

Updated

6 years ago
Attachment #537428 - Attachment is patch: false
(Assignee)

Comment 54

6 years ago
Created attachment 588437 [details] [diff] [review]
v. 2.1 XPCOM Errors

Trying to use this new interface in a xpcshell test, and I get: test_domcrypt_pk_generate_keypair.js:17 | TypeError: Cc['@mozilla.org/domcrypt-internal-api;1'] is undefined

I assume I have not added all of the magical XPCOM Gunk. I added NS_IMPL_CLASSINFO(nsDOMCryptInternalAPI, NULL, nsIClassInfo::THREADSAFE, NS_DOMCRYPTINTERNALAPI_CID) but get this error:

/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2708:41: error: expected identifier before ‘__null’
/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2708:41: error: expected ‘,’ or ‘...’ before ‘__null’
/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2714:1: error: expected constructor, destructor, or type conversion before ‘nsrefcnt’
Attachment #588202 - Attachment is obsolete: true
Attachment #588202 - Flags: feedback?(bsmith)
Attachment #588437 - Flags: feedback?(doug.turner)
(Assignee)

Comment 55

6 years ago
Created attachment 590107 [details] [diff] [review]
v 2.2 persistent key generation fails

Everything builds and links now, the persistent key gen fails in nsDOMCryptInternalAPI::GenerateKeyPair
Attachment #588437 - Attachment is obsolete: true
Attachment #588437 - Flags: feedback?(doug.turner)
Attachment #590107 - Flags: feedback?(bsmith)
Comment on attachment 590107 [details] [diff] [review]
v 2.2 persistent key generation fails

>--- /dev/null
>+++ b/security/manager/ssl/public/nsIDOMCryptAPIKeyPair.idl
>+interface nsIDOMCryptAPIKeyPair : nsISupports
>+{
>+  readonly attribute wchar keyID;

Is this supposed to be just one character?

>--- a/security/manager/ssl/src/nsNSSComponent.cpp
>+++ b/security/manager/ssl/src/nsNSSComponent.cpp
>+nsDOMCryptAPIKeyPair::GetKeyID(PRUnichar* aOutString)
>+{
>+  aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));

Does this even work? It seems to me that you're setting the pointer, and not actually filling in *aOutString...
(Assignee)

Comment 57

6 years ago
(In reply to Ms2ger from comment #56)
> Comment on attachment 590107 [details] [diff] [review]
> v 2.2 persistent key generation fails
> 
> >--- /dev/null
> >+++ b/security/manager/ssl/public/nsIDOMCryptAPIKeyPair.idl
> >+interface nsIDOMCryptAPIKeyPair : nsISupports
> >+{
> >+  readonly attribute wchar keyID;
> 
> Is this supposed to be just one character?
No, it will be longer.  The resulting data is unsigned char *data - so the IDL type should probably be "string"? Some confusion on my part.

> 
> >--- a/security/manager/ssl/src/nsNSSComponent.cpp
> >+++ b/security/manager/ssl/src/nsNSSComponent.cpp
> >+nsDOMCryptAPIKeyPair::GetKeyID(PRUnichar* aOutString)
> >+{
> >+  aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));
> 
> Does this even work? It seems to me that you're setting the pointer, and not
> actually filling in *aOutString...
This is untested - I was just getting that part to build for now.
(Assignee)

Comment 58

6 years ago
I am wondering if I am using the wrong slot...

  PK11SlotInfo *slot = PK11_GetInternalSlot();

  ...

  aPrivKey = PK11_GenerateKeyPair(slot, 
                                  CKM_RSA_PKCS_KEY_PAIR_GEN, 
                                  &rsaParams, 
                                  &aPubKey, 
                                  PR_TRUE /*isPerm*/, 
                                  PR_TRUE /*isSensitive*/, 
                                  NULL /*&pwdata*/);

... with "isPerm" set to true?
(In reply to David Dahl :ddahl from comment #57)
> (In reply to Ms2ger from comment #56)
> > Comment on attachment 590107 [details] [diff] [review]
> > v 2.2 persistent key generation fails
> > 
> > >--- /dev/null
> > >+++ b/security/manager/ssl/public/nsIDOMCryptAPIKeyPair.idl
> > >+interface nsIDOMCryptAPIKeyPair : nsISupports
> > >+{
> > >+  readonly attribute wchar keyID;
> > 
> > Is this supposed to be just one character?
> No, it will be longer.  The resulting data is unsigned char *data - so the
> IDL type should probably be "string"? Some confusion on my part.

You could do that, but DOMString makes for much nicer code.
(Assignee)

Comment 60

6 years ago
(In reply to Ms2ger from comment #59)

> You could do that, but DOMString makes for much nicer code.
Ok, will do. I was unsure what type the generated ID would be.
So then this:
NS_IMETHODIMP
nsDOMCryptInternalAPI::GetKeyPairByKeyID(char* aKeyID, 
                                         nsIDOMCryptAPIKeyPair **_retval NS_OUTPARAM)

should be:

NS_IMETHODIMP
nsDOMCryptInternalAPI::GetKeyPairByKeyID(nsAString& aKeyID, 
                                         nsIDOMCryptAPIKeyPair **_retval NS_OUTPARAM)
Inparams would be const nsAString&, out are nsAString&
(Assignee)

Comment 62

6 years ago
Created attachment 590365 [details]
backtrace
Attachment #590365 - Flags: feedback?(bsmith)
(Assignee)

Comment 63

6 years ago
Created attachment 590993 [details] [diff] [review]
v 3 Adding rough draft of PKEncrypt and PKDecrypt, still has keypair issues
(Assignee)

Updated

6 years ago
Summary: Implement DOMCrypt API → Implement DOMCrypt (Internal) API
(Assignee)

Updated

6 years ago
Blocks: 721193
(Assignee)

Updated

6 years ago
Blocks: 721199
(Assignee)

Updated

6 years ago
Blocks: 721200
(Assignee)

Comment 64

6 years ago
Created attachment 592050 [details] [diff] [review]
v 4 Linking problem

Linking error: 
/security/manager/ssl/src/nsNSSComponent.o: In function `nsDOMCryptInternalAPI::GetPublicKeyByKeyID(char*, SECKEYPublicKeyStr*)':
/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2898: undefined reference to `PK11_FindPrivateKeyFromNickname'
/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2905: undefined reference to `PK11_ExtractPublicKey'
collect2: ld returned 1 exit status
make[1]: *** [libxul.so] Error 1

Not sure how to include the right headers here, or something...
Attachment #590993 - Attachment is obsolete: true
Attachment #592050 - Flags: feedback?(bsmith)
(Assignee)

Updated

6 years ago
Attachment #590107 - Flags: feedback?(bsmith)
(Assignee)

Updated

6 years ago
Attachment #590365 - Flags: feedback?(bsmith)
(Assignee)

Comment 65

6 years ago
Created attachment 592057 [details] [diff] [review]
v 4.1 Cannot use private NSS functions, etc...

Running into a dead end with some private NSS functions used to get a private key from a keyID.
Attachment #592050 - Attachment is obsolete: true
Attachment #592050 - Flags: feedback?(bsmith)
Attachment #592057 - Flags: feedback?(bsmith)
(Assignee)

Comment 66

6 years ago
Seeing a cascade of missing dependencies and errors trying to use PK11_ExtractPublicKey. This seems like something I am not allowed to use outside of NSS itself. I attempted to re-create it as a private function to no avail: another cascade of errors like:

/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2763:3: error: invalid use of incomplete type ‘PK11SlotInfo {aka struct PK11SlotInfoStr}’
../../../../dist/include/secmodt.h:59:16: error: forward declaration of ‘PK11SlotInfo {aka struct PK11SlotInfoStr}’
/home/ddahl/code

I think I have already re-created a few wheels here. is there another way to get a keypair from the keydb via a nickname or keyID?

If not, I assume we need to create public versions of these 2 functions:

PK11_ExtractPublicKey
and PK11_FindPrivateKeyFromNickname

I also tried to substitute PK11_FindPrivateKeyFromNickname with PK11_FindKeyByKeyID
(Assignee)

Comment 67

6 years ago
Created attachment 592912 [details] [diff] [review]
v 4.2 Naive Patch with copied functions, it'll never work
Attachment #592912 - Flags: feedback?(bsmith)

Comment 68

6 years ago
I gave feedback to David on IRC.
(Assignee)

Comment 69

6 years ago
Created attachment 593424 [details]
irc discussion with kaie
(Assignee)

Comment 70

6 years ago
Created attachment 593668 [details] [diff] [review]
v. 4.3 breaking out PSM from XPCOM/DOM untested

Saving my work
Attachment #526080 - Attachment is obsolete: true
Attachment #590107 - Attachment is obsolete: true
Attachment #590365 - Attachment is obsolete: true
Attachment #592057 - Attachment is obsolete: true
Attachment #592912 - Attachment is obsolete: true
Attachment #592057 - Flags: feedback?(bsmith)
Attachment #592912 - Flags: feedback?(bsmith)
(Assignee)

Comment 71

6 years ago
Created attachment 597097 [details] [diff] [review]
4.4 Attempting to isolate all XPCOM bits to dom/

In my attempt to kill off all XPCOM bits of the PSM part of this patch, I cannot get it to build. 
1. The presence of a dtor in the .h or the impl gives me:

/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp:99:61: error: definition of implicitly-declared ‘nsDOMCryptInternalAPIKeyPair::~nsDOMCryptInternalAPIKeyPair()’
/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp:158:47: error: definition of implicitly-declared ‘nsDOMCryptInternalAPI::~nsDOMCryptInternalAPI()’


So I just removed the dtor for now.

2. I always see: /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp:266:3: error: ‘class nsDOMCryptInternalAPIKeyPair’ has no member named ‘AddRef’

It seems like I cannot not have an XPCOM interface and just use a plain old C++ class here.
Attachment #593668 - Attachment is obsolete: true
Attachment #597097 - Flags: feedback?(bsmith)
(Assignee)

Comment 72

6 years ago
(In reply to David Dahl :ddahl from comment #71)
> Created attachment 597097 [details] [diff] [review]
> 4.4 Attempting to isolate all XPCOM bits to dom/
> 
> In my attempt to kill off all XPCOM bits of the PSM part of this patch, I
> cannot get it to build.

[clarification] The attempt is to keep all xpcom interfaces in dom/ and have none in PSM to simplify what is landed in PSM
(Assignee)

Comment 73

6 years ago
Ah, ok, making headway with NS_IMPL_ISUPPORTS0
(Assignee)

Comment 74

6 years ago
Created attachment 597303 [details] [diff] [review]
4.5 Builds now, added PKDecrypt

I still cannot figure out how to enable the destructor and nsNSSShutdown bits
Attachment #597097 - Attachment is obsolete: true
Attachment #597097 - Flags: feedback?(bsmith)
Attachment #597303 - Flags: feedback?(bsmith)
(In reply to David Dahl :ddahl from comment #72)
> (In reply to David Dahl :ddahl from comment #71)
> > Created attachment 597097 [details] [diff] [review]
> > 4.4 Attempting to isolate all XPCOM bits to dom/
> > 
> > In my attempt to kill off all XPCOM bits of the PSM part of this patch, I
> > cannot get it to build.
> 
> [clarification] The attempt is to keep all xpcom interfaces in dom/ and have
> none in PSM to simplify what is landed in PSM

Note: that means you can't directly access the code in PSM from script, including xpcshell tests.
(Assignee)

Comment 76

6 years ago
(In reply to Ms2ger from comment #75)
> (In reply to David Dahl :ddahl from comment #72)
> > (In reply to David Dahl :ddahl from comment #71)
> > > Created attachment 597097 [details] [diff] [review]
> > > 4.4 Attempting to isolate all XPCOM bits to dom/
> > > 
> > > In my attempt to kill off all XPCOM bits of the PSM part of this patch, I
> > > cannot get it to build.
> > 
> > [clarification] The attempt is to keep all xpcom interfaces in dom/ and have
> > none in PSM to simplify what is landed in PSM
> 
> Note: that means you can't directly access the code in PSM from script,
> including xpcshell tests.

I was thinking I would be able to add an light wrapper to this internalAPI in dom/base or elsewhere that allows script access. The downside of not having direct script access might mean that the testing is not as complete or tightly coupled as it could be. That seems to be a problem now that I think about it.
(Assignee)

Comment 77

6 years ago
(In reply to David Dahl :ddahl from comment #76)
> > Note: that means you can't directly access the code in PSM from script,
> > including xpcshell tests.
> 
> I was thinking I would be able to add an light wrapper to this internalAPI
> in dom/base or elsewhere that allows script access. The downside of not
> having direct script access might mean that the testing is not as complete
> or tightly coupled as it could be. That seems to be a problem now that I
> think about it.

Of course, how feasible is creating an IDL where the arguments are low-level raw keys like SECKEYPublicKey, etc? I am unsure about how this could be accomplished.
(Assignee)

Comment 78

6 years ago
Build errors starting to cascade:

nsDOMCryptAPI.cpp
In file included from ../../dist/include/nsDOMCryptInternalAPI.h:57:0,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp:45:
../../dist/system_wrappers/secmodt.h:3:26: fatal error: secmodt.h: No such file or directory
compilation terminated.

It seems like any headers used from inside NSS or PSM are not found inside of nsDOMCryptAPI in dom/base. I added nsDOMCryptInternalAPI.h to the EXPORTS in the security/manager/ssl/src/Makefile.in

I must be doing something wrong here, I cannot imagine I will have to export each header I use since the Makefile in ssl/src had no EXPORTS at all. (Attaching latest patch in a moment.)
(Assignee)

Comment 79

6 years ago
Created attachment 597595 [details] [diff] [review]
v 4.6 Build errors, header export problem
Attachment #597595 - Flags: feedback?
(Assignee)

Updated

6 years ago
Attachment #597595 - Flags: feedback? → feedback?(Ms2ger)
Comment on attachment 597595 [details] [diff] [review]
v 4.6 Build errors, header export problem

Please start using the new MPL boilerplate: http://www.mozilla.org/MPL/headers/

>--- /dev/null
>+++ b/dom/base/nsDOMCryptAPI.cpp
>+#include "nsCOMPtr.h"
>+#include "jsapi.h"
>+#include "nsStringGlue.h"
>+#include "plbase64.h"
>+
>+#include "nsDOMCryptInternalAPI.h"
>+#include "nsDOMCryptAPI.h"

This include should be first.

>+
>+// Stolen from /toolkit/components/places/Helpers.cpp
>+// TODO: move this to Utils.h as per bz on irc

How about xpcom/io/Base64.h?


>+//---------------------------------------------
>+// Implementing nsDOMCryptAPIKeyPair
>+//---------------------------------------------
>+
>+NS_IMPL_ISUPPORTS1(nsDOMCryptAPIPublicKey, nsIDOMCryptAPIPublicKey)
>+
>+static NS_DEFINE_CID(kDOMCryptAPIPublicKeyCID, 
>+                     NS_DOMCRYPTAPIPUBLICKEY_CID);
>+
>+nsDOMCryptAPIPublicKey::nsDOMCryptAPIPublicKey()
>+{
>+  
>+}
>+
>+nsDOMCryptAPIPublicKey::nsDOMCryptAPIPublicKey()
>+{
>+
>+}

Remove double implementation

>+
>+nsDOMCryptAPIPublicKey::~nsDOMCryptAPIPublicKey(nsACString* aPublicKey,
>+                                                nsACString* aKeyID)
>+{
>+  mBase64EncodedDERPublicKey = aPublicKey;
>+  mKeyID = aKeyID;
>+}

A destructor that takes arguments?

>+nsDOMCryptAPIPublicKey::GetPublicKey(nsACString& aOutString)
>+{
>+  // XXX: format of this key: DEREncoded->base64encoded
>+  // aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID))
>+  aOutString = &mBase64EncodedDERPublicKey;

You need *mBase64EncodedDERPublicKey, no?

>+nsDOMCryptAPIPublicKey::GetKeyID(nsAString& aOutString)
>+{
>+  aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));

ToNewUnicode() shouldn't be necessary, I think

>+//---------------------------------------------
>+// Implementing nsDOMCryptAPIPKCipherData: 
>+// The result of an Encrypt operation
>+//---------------------------------------------
>+
>+NS_IMPL_ISUPPORTS1(nsDOMCryptAPIPKCipherData, nsIDOMCryptAPIPKCipherData)
>+
>+static NS_DEFINE_CID(kDOMCryptAPIPKCipherDataCID, 
>+                     NS_DOMCRYPTAPIPKCIPHERDATA_CID);
>+
>+nsDOMCryptAPIPKCipherData::nsDOMCryptAPIPKCipherData()
>+{
>+}
>+
>+nsDOMCryptAPIPKCipherData::nsDOMCryptAPIPKCipherData(nsDOMCryptAPIPublicKey* aPublicKey,
>+                                                     char* aEncryptedData)
>+{
>+  mPublicKey = aPublicKey;
>+  mEncryptedData = aEncryptedData;
>+
>+}
>+
>+nsDOMCryptAPIPKCipherData::~nsDOMCryptAPIPKCipherData()
>+{
>+}
>+
>+NS_IMETHODIMP
>+nsDOMCryptAPIPKCipherData::GetPublicKey(nsACString& aOutString)
>+{
>+  mPublicKey->GetPublicKey(aOutString);
>+  
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDOMCryptAPIPKData::GetKeyID(nsAString& aOutString)
>+{
>+  // aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));
>+  mPublicKey->GetKeyID(aOutString);
>+  return NS_OK;
>+}
>+
>+//---------------------------------------------
>+// Implementing nsDOMCryptAPI
>+//---------------------------------------------
>+
>+NS_IMPL_ISUPPORTS1(nsDOMCryptAPI, nsIDOMCryptAPI)
>+
>+static NS_DEFINE_CID(kDOMCryptAPICID, NS_DOMCRYPTAPI_CID);
>+
>+nsDOMCryptAPI::nsDOMCryptAPI()
>+{
>+  NS_ADDREF(mInternalAPI = new nsDOMCryptInternalAPI());
>+}
>+
>+nsDOMCryptAPI::~nsDOMCryptAPI()
>+{
>+}
>+
>+NS_IMETHODIMP 
>+nsDOMCryptAPI::PKGenerateKeyPair(PRUint32 aKeySizeInBits,
>+                                 PRUint32 aAlgorithm,
>+                                 nsIDOMCryptAPIPublicKey** _retval NS_OUTPARAM)
>+{
>+  // Wraps nsDOMCryptInternalAPI::PKGenerateKeyPair
>+  char* keyID;
>+  PRUint32 algorithm = 0;
>+  PRUint32 keySizeInBits = 2048;
>+  // TODO: create these variables inside the internal API... 
>+  // perhaps we should not use any NSS types in this API

We probably shouldn't.

>+  SECKEYPrivateKey privk* = NULL;
>+  SECKEYPublicKey pubk* = NULL;
>+  
>+  mInternalAPI->PKGenerateKeyPair(privk, pubk, keySizeInBits, algorithm, keyID);

This returns an nsresult; check it?

>+  nsACString base64DERPubKey;
>+  nsCString tmpStr;
>+
>+  nsresult rv = Base64urlEncode(data, len, tmpStr);
>+
>+  if (NS_FAILED(rv)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  base64DERPubKey = tmpStr;
>+  
>+  // TODO: get keyID as nsACString
>+  nsACString pubKeyID;
>+  pubKeyID = static_cast<nsACstring>(keyID);

You're doing a lot of weird things here...

How about

nsAutoCString base64DERPubKey;
nsresult rv = Base64urlEncode(data, len, base64DERPubKey);
NS_ENSURE_SUCCESS(rv, rv);

NS_ADDREF(*_retval = new nsDOMCryptAPIPublicKey(base64DERPubKey, nsDependentCString(keyID)));
return NS_OK;


>--- /dev/null
>+++ b/dom/base/nsDOMCryptAPI.h
>+#ifndef _nsDOMCryptAPI_h_
>+#define _nsDOMCryptAPI_h_

No leading or trailing underscores, please.

>+#define NS_DOMCRYPTAPIPUBLICKEY_CONTRACTID "@mozilla.org/domcrypt-api-publickey;1"
>+#define NS_DOMCRYPTAPIPUBLICKEY_COMPONENT_CLASSNAME "DOMCrypt API Public Key";
>+#define NS_DOMCRYPTAPIPUBLICKEY_CID {0x029a82c2, 0x9e7f, 0x49d9, {0x90, 0x7e, 0x37, 0xf4, 0x8d, 0xc2, 0x86, 0xeb}}
>+
>+#define NS_DOMCRYPTAPI_CONTRACTID "@mozilla.org/domcrypt-api;1"
>+#define NS_DOMCRYPTAPI_COMPONENT_CLASSNAME "DOMCrypt API";
>+#define NS_DOMCRYPTAPI_CID {0x42092ff4, 0x23c7, 0x4ed8, {0x9d, 0x57, 0xbf, 0xbc, 0x67, 0xc6, 0x12, 0x78}}
>+
>+#define NS_DOMCRYPTAPIPKCIPHERDATA_CONTRACTID "@mozilla.org/domcrypt-pk-cipher-data;1"
>+#define NS_DOMCRYPTAPIPKCIPHERDATA_COMPONENT_CLASSNAME "DOMCrypt API PK Cipher Data";
>+#define NS_DOMCRYPTAPIPKCIPHERDATA_CID {0xd0a66274, 0xc26a, 0x4b1c, {0x86, 0x85, 0x5a, 0x77, 0x55, 0x5c, 0x17, 0xaf}}
>+
>+#define NS_DOMCRYPTAPIPKCLEARDATA_CONTRACTID "@mozilla.org/domcrypt-pk-clear-data;1"
>+#define NS_DOMCRYPTAPIPKCLEARDATA_COMPONENT_CLASSNAME "DOMCrypt API PK Clear Data";
>+#define NS_DOMCRYPTAPIPKCLEARDATA_CID {0xd7adcacc, 0x3203, 0x4f12, {0x83, 0x7a, 0x16, 0x5b, 0xdf, 0xf3, 0x31, 0xf5}}

I don't really understand why you want all this.


>+class nsDOMCryptAPIPublicKey : public nsIDOMCryptAPIPublicKey
>+{
>+  nsDOMCryptAPIPublicKey(nsACString* aPublicKey, nsAString* aKeyID);

We don't usually use pointers to strings.


>+class nsDOMCryptAPIPKCipherData : public nsIDOMCryptAPIPKCipherData
>+{
>+  nsDOMCryptAPIPublicKey* mPublicKey;
>+  jsval& mEncryptedData; // an ArrayBuffer

privatize those. You probably want to keep mEncryptedData as JSObject*.

>+class nsDOMCryptAPIPKClearData : public nsIDOMCryptAPIPKClearData
>+{
>+  char* mClearData;

Same

>+class nsDOMCryptAPI : public nsIDOMCryptAPI
>+{
>+  nsDOMCryptInternalAPI mInternalAPI;

Pointer?

>--- /dev/null
>+++ b/security/manager/ssl/public/nsIDOMCryptAPIKeyPair.idl
>--- /dev/null
>+++ b/security/manager/ssl/public/nsIDOMCryptInternalAPI.idl

I think we'd best start without interfaces in psm. In any case, these shouldn't be nsIDOM* prefixed; that prefix is for DOM interfaces.

>--- a/security/manager/ssl/src/Makefile.in
>+++ b/security/manager/ssl/src/Makefile.in
>+	nsDOMCryptInternalAPI.cpp \

Two spaces

>+EXPORTS = \
>+	nsDOMCryptInternalAPI.h \
>+	$(NULL)

And here

>--- /dev/null
>+++ b/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp
>+nsDOMCryptInternalAPIKeyPair::GetKeyID(nsAString& aOutString)
>+{
>+  aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));

No ToNewUnicode

>+nsDOMCryptInternalAPIKeyPair::GetDEREncodedRSAPublicKey(unsigned char* aOutData)
>+{
>+  aOutData = mDERPubKey->data;

This doesn't do anything.

>+nsDOMCryptInternalAPI::GenerateKeyPair(SECKEYPublicKey* aPubKey, 
>+                                       SECKEYPrivateKey* aPrivKey,
>+                                       PRUint32 aKeySizeInBits,
>+                                       PRUint32 aAlgorithm)
>+{

If the goal is to return a new public/private key, those arguments need to be pointers-to-pointers, so you can set |*aPubKey = foo();|

>--- /dev/null
>+++ b/security/manager/ssl/src/nsDOMCryptInternalAPI.h
>+class nsDOMCryptInternalAPIKeyPair : public nsISupports //, public nsNSSShutDownObject
>+class nsDOMCryptInternalAPI : public nsISupports //, public nsNSSShutDownObject

No need to inherit from nsISupports for either of these

>--- a/security/manager/ssl/src/nsNSSComponent.cpp
>+++ b/security/manager/ssl/src/nsNSSComponent.cpp
>--- a/security/manager/ssl/src/nsNSSComponent.h
>+++ b/security/manager/ssl/src/nsNSSComponent.h
>--- a/security/manager/ssl/src/nsNSSModule.cpp
>+++ b/security/manager/ssl/src/nsNSSModule.cpp

Revert changes to these files, I think
(In reply to David Dahl :ddahl from comment #78)
> Build errors starting to cascade:
> 
> nsDOMCryptAPI.cpp
> In file included from ../../dist/include/nsDOMCryptInternalAPI.h:57:0,
>                  from
> /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp:45:
> ../../dist/system_wrappers/secmodt.h:3:26: fatal error: secmodt.h: No such
> file or directory
> compilation terminated.
> 
> It seems like any headers used from inside NSS or PSM are not found inside
> of nsDOMCryptAPI in dom/base. I added nsDOMCryptInternalAPI.h to the EXPORTS
> in the security/manager/ssl/src/Makefile.in
> 
> I must be doing something wrong here, I cannot imagine I will have to export
> each header I use since the Makefile in ssl/src had no EXPORTS at all.
> (Attaching latest patch in a moment.)

I doubt anything outside PSM uses NSS directly, we probably want to keep it that way.
(Assignee)

Comment 82

6 years ago
(In reply to Ms2ger from comment #80)
> Comment on attachment 597595 [details] [diff] [review]
> 
> >+
> >+// Stolen from /toolkit/components/places/Helpers.cpp
> >+// TODO: move this to Utils.h as per bz on irc
> 
> How about xpcom/io/Base64.h?
> 
Ah, cool. I was unaware. I will try to make some helpers that use this as I also need to decode base64url encoding

> >+
> >+nsDOMCryptAPIPublicKey::~nsDOMCryptAPIPublicKey(nsACString* aPublicKey,
> >+                                                nsACString* aKeyID)
> >+{
> >+  mBase64EncodedDERPublicKey = aPublicKey;
> >+  mKeyID = aKeyID;
> >+}
> 
> A destructor that takes arguments?

Oh jeez. that is misplaced.

> You're doing a lot of weird things here...
> 
> How about
> 
> nsAutoCString base64DERPubKey;
> nsresult rv = Base64urlEncode(data, len, base64DERPubKey);
> NS_ENSURE_SUCCESS(rv, rv);
> 
> NS_ADDREF(*_retval = new nsDOMCryptAPIPublicKey(base64DERPubKey,
> nsDependentCString(keyID)));
> return NS_OK;
> 

Thanks - these pointers are invaluable. I know a fair bit of my hacking here is me learning the Mozilla C++ way.  

> >+
> >+#define NS_DOMCRYPTAPIPKCLEARDATA_CONTRACTID "@mozilla.org/domcrypt-pk-clear-data;1"
> >+#define NS_DOMCRYPTAPIPKCLEARDATA_COMPONENT_CLASSNAME "DOMCrypt API PK Clear Data";
> >+#define NS_DOMCRYPTAPIPKCLEARDATA_CID {0xd7adcacc, 0x3203, 0x4f12, {0x83, 0x7a, 0x16, 0x5b, 0xdf, 0xf3, 0x31, 0xf5}}
> 
> I don't really understand why you want all this.
> 
I wanted to explicitly lay out the inputs and outputs from the encrypt and decrypt. The KeyID and publicKey have to travel around with the encrypted data.

Perhaps it is overkill, with some test writing, it should play out as too much or if it is OK.

> 
> >+class nsDOMCryptAPIPublicKey : public nsIDOMCryptAPIPublicKey
> >+{
> >+  nsDOMCryptAPIPublicKey(nsACString* aPublicKey, nsAString* aKeyID);
> 
> We don't usually use pointers to strings.
Ok. Gotcha. 

> >--- /dev/null
> >+++ b/security/manager/ssl/public/nsIDOMCryptAPIKeyPair.idl
> >--- /dev/null
> >+++ b/security/manager/ssl/public/nsIDOMCryptInternalAPI.idl
> 
> I think we'd best start without interfaces in psm. In any case, these
> shouldn't be nsIDOM* prefixed; that prefix is for DOM interfaces.
> 
I forgot to nuke those from the patch, thanks.

> >--- a/security/manager/ssl/src/Makefile.in
> >+++ b/security/manager/ssl/src/Makefile.in
> >+	nsDOMCryptInternalAPI.cpp \
> 
> Two spaces
> 
> >+EXPORTS = \
> >+	nsDOMCryptInternalAPI.h \
> >+	$(NULL)
> 
> And here
> 
> >--- /dev/null
> >+++ b/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp
> >+nsDOMCryptInternalAPIKeyPair::GetKeyID(nsAString& aOutString)
> >+{
> >+  aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));
> 
> No ToNewUnicode
> 
> >+nsDOMCryptInternalAPIKeyPair::GetDEREncodedRSAPublicKey(unsigned char* aOutData)
> >+{
> >+  aOutData = mDERPubKey->data;
> 
> This doesn't do anything.
> 
> >+nsDOMCryptInternalAPI::GenerateKeyPair(SECKEYPublicKey* aPubKey, 
> >+                                       SECKEYPrivateKey* aPrivKey,
> >+                                       PRUint32 aKeySizeInBits,
> >+                                       PRUint32 aAlgorithm)
> >+{
> 
> If the goal is to return a new public/private key, those arguments need to
> be pointers-to-pointers, so you can set |*aPubKey = foo();|
> 
> >--- /dev/null
> >+++ b/security/manager/ssl/src/nsDOMCryptInternalAPI.h
> >+class nsDOMCryptInternalAPIKeyPair : public nsISupports //, public nsNSSShutDownObject
> >+class nsDOMCryptInternalAPI : public nsISupports //, public nsNSSShutDownObject
> 
> No need to inherit from nsISupports for either of these
> 
> >--- a/security/manager/ssl/src/nsNSSComponent.cpp
> >+++ b/security/manager/ssl/src/nsNSSComponent.cpp
> >--- a/security/manager/ssl/src/nsNSSComponent.h
> >+++ b/security/manager/ssl/src/nsNSSComponent.h
> >--- a/security/manager/ssl/src/nsNSSModule.cpp
> >+++ b/security/manager/ssl/src/nsNSSModule.cpp
> 
> Revert changes to these files, I think

Ah yes. No code should change there anymore. 

A new patch is forthcoming. I am working on getting it to build. 

Thanks again. I hope it is not a burden for you to peek at these ROUGH patches:(
(Assignee)

Comment 83

6 years ago
Created attachment 598394 [details] [diff] [review]
v 4.7 Saving Latest Work, more build errors
Attachment #597303 - Attachment is obsolete: true
Attachment #597595 - Attachment is obsolete: true
Attachment #597303 - Flags: feedback?(bsmith)
Attachment #597595 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 84

6 years ago
Created attachment 600564 [details] [diff] [review]
v 4.8 include errors, trouble with exports and nss headers

Mike:

With this patch, all of the nss headers are missing. I tried to export a new header in security/manager/ssl/src/Makefile.in

nsDOMCryptAPI.cpp
In file included from ../../dist/include/nsDOMCryptInternalAPI.h:12:0,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp:6:
../../dist/system_wrappers/pk11pub.h:3:26: fatal error: pk11pub.h: No such file or directory
compilation terminated.

pk11pub.h is not in dist/include - this is after a full clobber
Attachment #600564 - Flags: feedback?(mh+mozilla)
(Assignee)

Comment 85

6 years ago
(In reply to David Dahl :ddahl from comment #84)
> pk11pub.h is not in dist/include - this is after a full clobber

Looks like a full rebuild without the patch queue has fixed this issue.

Now I get:
security/manager/ssl/src/nsDOMCryptInternalAPI.cpp:26:1: error: invalid static_cast from type ‘nsDOMCryptInternalAPIKeyPair*’ to type ‘nsISupports*’

Via this macro: NS_IMPL_ISUPPORTS0(nsDOMCryptInternalAPIKeyPair)
(Assignee)

Updated

6 years ago
Attachment #600564 - Flags: feedback?(mh+mozilla)
You can't use NSS directly from the DOM.  NSS is built too late for that.
Comment on attachment 600564 [details] [diff] [review]
v 4.8 include errors, trouble with exports and nss headers

Review of attachment 600564 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMCryptAPI.cpp
@@ +7,5 @@
> +
> +#include "nsCOMPtr.h"
> +#include "jsapi.h"
> +#include "nsStringGlue.h"
> +#include "plbase64.h"

No need to include that anymore

@@ +43,5 @@
> +{
> +  nsresult rv = mozilla::Base64Encode(aPlainData, aB64UrlEncoded);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCString tmpStr = aB64UrlEncoded;

Pass tmpStr to mozilla::Base64Encode instead of copying back and forth.

@@ +60,5 @@
> +  nsCString tmpStr = aB64UrlEncoded;
> +  tmpStr.ReplaceChar('_', '/');
> +  tmpStr.ReplaceChar('-', '+');
> +  
> +  nsACString replacedStr = tmpStr;

No need for this, you can do

return mozilla::Base64Decode(tmpStr, aDecodedData);

@@ +177,5 @@
> +  nsresult rv;
> +
> +  rv = mInternalAPI->PKGenerateKeyPair(keySizeInBits, 
> +                                       algorithm, 
> +                                       internalKeyPair);

nsDOMCryptInternalAPIKeyPair* internalKeyPair;
nsresult rv = mInternalAPI->PKGenerateKeyPair(keySizeInBits, algorithm, &internalKeyPair);

You'll also need to figure out how not to leak it.

@@ +188,5 @@
> +
> +  PRUint32 len;
> +  PRUint8* data;
> +  len = static_cast<PRUint32>(derPubKey->len);
> +  data = static_cast<PRUint8*>(derPubKey->data);

That won't work with unsigned char* derPubKey;

@@ +197,5 @@
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsACString base64DERPubKey = tmpStr; // convert nsCString to nsACString

Not necessary, an nsCString already is an nsACString.

@@ +199,5 @@
> +  }
> +
> +  nsACString base64DERPubKey = tmpStr; // convert nsCString to nsACString
> +
> +  nsACString pubKeyID;

Need a concrete type; nsCAutoString or nsCString.

::: security/manager/ssl/src/nsDOMCryptInternalAPI.cpp
@@ +82,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +nsDOMCryptInternalAPIKeyPair::GetDEREncodedRSAPublicKey(unsigned char* aOutData)

I think you just want nsACString& aOutData

@@ +91,5 @@
> +  //   return NS_OK;
> +
> +  mDERPubKey = PK11_DEREncodePublicKey(mPublicKey);
> +  // XXXMs2Ger: this does not do anything...
> +  aOutData = mDERPubKey->data;

And then have aOutData = nsDependentCString(mDERPubKey->data, mDERPubKey->len);

::: security/manager/ssl/src/nsDOMCryptInternalAPI.h
@@ +16,5 @@
> +#include "base64.h"
> +#include "secerr.h"
> +#include "pkcs11t.h" 
> +#include "keyhi.h"
> +#include "keythi.h"

You won't be able to include all this from an exported header you need in DOM. Try forward declaring what you need for now.

@@ +21,5 @@
> +
> +class nsDOMCryptInternalAPIKeyPair //: public nsNSSShutDownObject
> +{
> +public:
> +  NS_DECL_ISUPPORTS

Remove this

@@ +24,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  // NS_DECL_NSIDOMCRYPTAPIKEYPAIR
> +
> +  nsDOMCryptInternalAPIKeyPair();

And this

@@ +56,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  // NS_DECL_NSIDOMCRYPTINTERNALAPI
> +
> +  nsDOMCryptInternalAPI();

Same here
(Assignee)

Comment 88

6 years ago
(In reply to Ms2ger from comment #87)
> Comment on attachment 600564 [details] [diff] [review]

> You won't be able to include all this from an exported header you need in
> DOM. Try forward declaring what you need for now.

It looks like I need to forward declare SECKEYPublicKey, SECKEYPrivateKey and SECItem - after reading about it, I am unclear how to do it. Is it some kind of #define or typedef?

> 
> @@ +21,5 @@
> > +
> > +class nsDOMCryptInternalAPIKeyPair //: public nsNSSShutDownObject
> > +{
> > +public:
> > +  NS_DECL_ISUPPORTS
> 
> Remove this

When I remove these, I get: /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.h:23:3: error: ‘nsresult’ does not name a type
(In reply to David Dahl :ddahl from comment #88)
> (In reply to Ms2ger from comment #87)
> > Comment on attachment 600564 [details] [diff] [review]
> 
> > You won't be able to include all this from an exported header you need in
> > DOM. Try forward declaring what you need for now.
> 
> It looks like I need to forward declare SECKEYPublicKey, SECKEYPrivateKey
> and SECItem - after reading about it, I am unclear how to do it. Is it some
> kind of #define or typedef?

class SECKEYPublicKey;
class SECKEYPrivateKey;
class SECItem;

should be enough.
(Assignee)

Comment 90

6 years ago
(In reply to Mike Hommey [:glandium] from comment #89)
> class SECKEYPublicKey;
> class SECKEYPrivateKey;
> class SECItem;
> 
> should be enough.

I was wrong about how many nss types are referenced here - there are many more in the cpp file, and the forward declaration is not working.

see: http://pastebin.mozilla.org/1491898

Is this idea of trying to use this PSM class sans xpcom inside dom/ not feasible?
(In reply to David Dahl :ddahl from comment #90)
> (In reply to Mike Hommey [:glandium] from comment #89)
> > class SECKEYPublicKey;
> > class SECKEYPrivateKey;
> > class SECItem;
> > 
> > should be enough.
> 
> I was wrong about how many nss types are referenced here - there are many
> more in the cpp file, and the forward declaration is not working.
> 
> see: http://pastebin.mozilla.org/1491898
> 
> Is this idea of trying to use this PSM class sans xpcom inside dom/ not
> feasible?

The cpp file is in PSM, so you can just move the includes from the .h to that .cpp

(In reply to David Dahl :ddahl from comment #88)
> > @@ +21,5 @@
> > > +
> > > +class nsDOMCryptInternalAPIKeyPair //: public nsNSSShutDownObject
> > > +{
> > > +public:
> > > +  NS_DECL_ISUPPORTS
> > 
> > Remove this
> 
> When I remove these, I get:
> /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/
> nsDOMCryptInternalAPI.h:23:3: error: ‘nsresult’ does not name a type

Include nscore.h
(Assignee)

Comment 92

6 years ago
Created attachment 601771 [details] [diff] [review]
v 4.9 It builds and Links!

Stripped out unneeded interfaces - this patch actually builds and links. Shocking. Provides PKGenerateKeyPair, PKEncrypt, PKDecrypt - not tested. I think the mInternalAPI member variable in nsDOMCryptAPI ought to be lazily init'd or I should be using an nsRefPtr?? not sure. If you have time for feedback I would greatly appreciate it. Also, I think my string usage is still not right in some cases. I re-read the docs again, it helped.
Attachment #598394 - Attachment is obsolete: true
Attachment #600564 - Attachment is obsolete: true
Attachment #601771 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 93

6 years ago
Created attachment 601872 [details] [diff] [review]
v 5 It Builds, but the component is not available in Components.classes

The previous questions are still valid, I added a test and tried to load an instance and I get:

TEST-INFO | (xpcshell/head.js) | test 1 pending
DOMCryptAPI: TypeError: Cc['@mozilla.org/domcrypt-api;1'] is undefined
DOMCryptAPI: run_test()@/home/ddahl/code/moz/domcrypt/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/dom/tests/unit/test_domcryptapi.js:13
_execute_test()@/home/ddahl/code/moz/domcrypt/src/testing/xpcshell/head.js:326
@-e:1


gdb tells me:

(gdb) break nsDOMCryptAPI::nsDOMCryptAPI
Breakpoint 1 at 0x2ad7cdb09edd: file /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp, line 96.
(gdb) bt
#0  0x00002ad7d0af3bad in read () at ../sysdeps/unix/syscall-template.S:82
#1  0x00002ad7d0a94d38 in _IO_new_file_underflow (fp=0x2ad7d0db89c0) at fileops.c:606
#2  0x00002ad7d0a95d7e in _IO_default_uflow (fp=0x2ad7d0db89c0) at genops.c:440
#3  0x00002ad7d0a8a08a in _IO_getline_info (fp=0x2ad7d0db89c0, buf=0x7fff0518c370 "", n=255, delim=10, extract_delim=1, eof=0x0) at iogetline.c:74
#4  0x00002ad7d0a88f6b in _IO_fgets (buf=0x7fff0518c370 "", n=<optimized out>, fp=0x2ad7d0db89c0) at iofgets.c:58
#5  0x0000000000406121 in GetLine (cx=0x2ad7d9ff2000, bufp=0x7fff0518c530 "", file=0x2ad7d0db89c0, prompt=0x43000b "js> ")
    at /home/ddahl/code/moz/domcrypt/src/js/xpconnect/shell/xpcshell.cpp:277
#6  0x0000000000408255 in ProcessFile (cx=0x2ad7d9ff2000, obj=0x2ad7dd22f060, filename=0x0, file=0x2ad7d0db89c0, forceTTY=1)
    at /home/ddahl/code/moz/domcrypt/src/js/xpconnect/shell/xpcshell.cpp:1075
#7  0x0000000000408603 in Process (cx=0x2ad7d9ff2000, obj=0x2ad7dd22f060, filename=0x0, forceTTY=1) at /home/ddahl/code/moz/domcrypt/src/js/xpconnect/shell/xpcshell.cpp:1130
#8  0x0000000000408d11 in ProcessArgs (cx=0x2ad7d9ff2000, obj=0x2ad7dd22f060, argv=0x7fff0518da30, argc=20) at /home/ddahl/code/moz/domcrypt/src/js/xpconnect/shell/xpcshell.cpp:1295
#9  0x000000000040a948 in main (argc=20, argv=0x7fff0518da30, envp=0x7fff0518dad8) at /home/ddahl/code/moz/domcrypt/src/js/xpconnect/shell/xpcshell.cpp:2001
(gdb) n
read () at ../sysdeps/unix/syscall-template.S:83
83	in ../sysdeps/unix/syscall-template.S
(gdb) 
_IO_new_file_underflow (fp=0x2ad7d0db89c0) at fileops.c:608
608	fileops.c: No such file or directory.
	in fileops.c
(gdb) 
618	in fileops.c
(gdb) 
615	in fileops.c
(gdb) 
618	in fileops.c
(gdb) 
620	in fileops.c
(gdb) 
621	in fileops.c
(gdb) 
_IO_default_uflow (fp=0x2ad7d0db89c0) at genops.c:441
441	genops.c: No such file or directory.
Attachment #601771 - Attachment is obsolete: true
Attachment #601771 - Flags: feedback?(Ms2ger)
Attachment #601872 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 94

6 years ago
Comment on attachment 601771 [details] [diff] [review]
v 4.9 It builds and Links!

I also changed the CID to mozilla.org/dom/domcrypt-api;1 and I ran:

$ strings ./dist/bin/components/dom_base.xpt | grep -i crypt
nsIDOMCrypto
nsIDOMCryptAPIPublicKey
nsIDOMCryptAPI
PKEncrypt
PKDecrypt
crypto

So I assume there is one last undocumented thing to do (or two):)
Comment on attachment 601872 [details] [diff] [review]
v 5 It Builds, but the component is not available in Components.classes

Review of attachment 601872 [details] [diff] [review]:
-----------------------------------------------------------------

Haven't reviewed it entirely

::: dom/base/nsDOMCryptAPI.cpp
@@ +14,5 @@
> +
> +// Base64urlEncode / Decode helpers
> +static 
> +nsresult 
> +Base64urlEncoder(nsCString aPlainData, nsACString& aB64UrlEncoded)

const nsCString& aPlaindata.

@@ +21,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  // XXXddahl: base64 URL encoded
> +  // nsCString tmpStr = aB64UrlEncoded;
> +  // tmpStr.ReplaceChar('+', '-');
> +  // tmpStr.ReplaceChar('/', '_');

So, since you only call this with a nsCString for aB64UrlEncoded, I think you should just change to signature to take nsCString& instead of nsACString&, and do ReplaceChar directly on aB64UrlEncoded.

@@ +30,5 @@
> +}
> +
> +static 
> +nsresult
> +Base64urlDecoder(nsACString& aB64UrlEncoded, nsACString& aDecodedData)

const nsACString& aB64UrlEncoded

@@ +35,5 @@
> +{
> +  // XXXddahl: this should be base64 URL decoded
> +  // nsCString tmpStr = aB64UrlEncoded;
> +  // tmpStr.ReplaceChar('_', '/');
> +  // tmpStr.ReplaceChar('-', '+');

This should work, I think. Any particular reason to comment it out?

@@ +37,5 @@
> +  // nsCString tmpStr = aB64UrlEncoded;
> +  // tmpStr.ReplaceChar('_', '/');
> +  // tmpStr.ReplaceChar('-', '+');
> +  
> +  // nsACString replacedStr = tmpStr;

Remove this line

@@ +118,5 @@
> +  nsDOMCryptInternalAPIKeyPair* internalKeyPair;
> +
> +  rv = mInternalAPI->PKGenerateKeyPair(keySizeInBits, 
> +                                       algorithm, 
> +                                       internalKeyPair);

&internalKeyPair

@@ +124,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  unsigned char* derPubKey;
> +  internalKeyPair->GetDEREncodedRSAPublicKey(derPubKey);

&derPubKey

@@ +143,5 @@
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCString base64DERPubKey = tmpStr; // convert nsCString to nsACString

No need for this, you can just use the nsCString you already have.

@@ +160,5 @@
> +
> +NS_IMETHODIMP
> +nsDOMCryptAPI::PKEncrypt(nsIDOMCryptAPIPublicKey* aPublicKey,
> +                         const jsval& aClearData,
> +                         jsval* _retval NS_OUTPARAM)

You're not returning anything?

@@ +179,5 @@
> +    return NS_ERROR_FAILURE; // TODO: perhaps we need a better error here?
> +  }
> +  char* clearData = NULL;
> +
> +  clearData = static_cast<char*>(JS_GetTypedArrayData(view));

You've got two-step assignment like this in a number of places, they should just be like

char* clearData = static_cast<char*>(JS_GetTypedArrayData(view));

@@ +201,5 @@
> +  mInternalAPI->PKEncrypt(derPubKeyData, 
> +                          clearData, 
> +                          encryptedData);
> +
> +  NS_ENSURE_TRUE(encryptedData != NULL, NS_ERROR_FAILURE);

It will be null, because mInternalAPI->PKEncrypt can't touch your encryptedData

@@ +213,5 @@
> +                         jsval* aClearData NS_OUTPARAM)
> +{
> +  // Convert everything to char* to call the internal API
> +  char* keyID = NULL;
> +  keyID = const_cast<char*>(aKeyID.Data());

Are we sure aKeyID.Data() won't go away too soon? Maybe this needs to copy

@@ +226,5 @@
> +    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +  }
> +
> +  char* encryptedData = NULL;
> +  encryptedData = static_cast<char*>(JS_GetTypedArrayData(encryptedView));

.

@@ +232,5 @@
> +  NS_ENSURE_TRUE(encryptedData != NULL, NS_ERROR_FAILURE);
> +  
> +  // Get the clear buffer data
> +
> +  if (!aClearData->isObject()) {

aClearData is a return value; you need to create a typed array yourself

::: dom/base/nsDOMCryptAPI.h
@@ +49,5 @@
> +  nsDOMCryptAPI();
> +
> +private:
> +  nsDOMCryptInternalAPI* mInternalAPI;
> +  // nsRefPtr<nsDOMCryptInternalAPI> mInternalAPI; // Should use an nsRefPtr??

I don't think so, unless someone else can get its hand on this pointer. I think it should just stay alive as long as nsDOMCryptAPI does, so either |delete| in the dtor, or use nsAutoPtr.

@@ +50,5 @@
> +
> +private:
> +  nsDOMCryptInternalAPI* mInternalAPI;
> +  // nsRefPtr<nsDOMCryptInternalAPI> mInternalAPI; // Should use an nsRefPtr??
> +  // ~nsDOMCryptAPI(); // XXXddahl: trouble declaring dtor

You probably need to make it public

::: security/manager/ssl/src/nsDOMCryptInternalAPI.cpp
@@ +169,5 @@
> +
> +nsresult
> +nsDOMCryptInternalAPI::PKGenerateKeyPair(PRUint32 aKeySizeInBits,
> +                                         PRUint32 aAlgorithm,
> +                                         nsDOMCryptInternalAPIKeyPair *_retval)

This needs to be a pointer-to-a-pointer

@@ +208,5 @@
> +  _retval = new nsDOMCryptInternalAPIKeyPair(privKey, 
> +                                             pubKey, 
> +                                             aKeySizeInBits, 
> +                                             aAlgorithm, 
> +                                             nickname);

This currently only changes the local variable _retval, not the pointer your caller passed in.

@@ +223,5 @@
> +  SECStatus srv;
> +  SECKEYPublicKey* pubk = NULL;
> +  SECItem* derEncodedKey;
> +
> +  derEncodedKey->data = (unsigned char*)aDERPubKey;

Wait, don't you need to initialize your derEncodedKey pointer first?

@@ +231,5 @@
> +  NS_ENSURE_TRUE(pubk != NULL, NS_ERROR_FAILURE);
> +
> +  srv = PK11_PubEncryptPKCS1(pubk, 
> +                             (unsigned char*)aEncryptedData, 
> +                             (unsigned char*)aClearData, 

static_cast

@@ +255,5 @@
> +  PK11SlotInfo *slot = PK11_GetInternalSlot();
> +  SECItem* keyID;
> +  
> +  keyID->data = (unsigned char*)aKeyID;
> +  keyID->len = strlen(aKeyID);

I think you should just pass an XPCOM string here and use its Length() instead of recalculating.

::: security/manager/ssl/tests/unit/test_domcrypt_pk_generate_keypair.js
@@ +13,5 @@
> +  dump("DOMCryptPK test: " + aMsg + "\n");
> +}
> +
> +XPCOMUtils.defineLazyGetter(this, "DOMCryptInternalAPI", function (){
> +  return Cc["@mozilla.org/domcrypt-internal-api;1"].createInstance(Ci.nsIDOMCryptInternalAPI);

You noticed this was missing a dom/, right?
(Assignee)

Comment 96

6 years ago
(In reply to Ms2ger from comment #95)
> Comment on attachment 601872 [details] [diff] [review]
> ::: security/manager/ssl/tests/unit/test_domcrypt_pk_generate_keypair.js
> @@ +13,5 @@
> > +  dump("DOMCryptPK test: " + aMsg + "\n");
> > +}
> > +
> > +XPCOMUtils.defineLazyGetter(this, "DOMCryptInternalAPI", function (){
> > +  return Cc["@mozilla.org/domcrypt-internal-api;1"].createInstance(Ci.nsIDOMCryptInternalAPI);
> 
> You noticed this was missing a dom/, right?

I need to remove that test, it us from before I moved the public API out into dom/base and removed the public interface from the internal API.

The test I added should be in dom/tests/unit. I am working through these comments and will post another patch soon. Thanks!
(Assignee)

Comment 97

6 years ago
Created attachment 602211 [details] [diff] [review]
v 5.1 build problems

more include problems where nsIDOMCryptInternalAPI.h cannot include nss headers:

nsDOMCryptAPIModule.cpp
In file included from ../../dist/include/nsDOMCryptInternalAPI.h:14:0,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.h:8,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp:5:
../../dist/system_wrappers/secerr.h:3:25: fatal error: secerr.h: No such file or directory
compilation terminated.
In file included from ../../dist/include/nsDOMCryptInternalAPI.h:14:0,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.h:8,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPIModule.cpp:8:
../../dist/system_wrappers/secerr.h:3:25: fatal error: secerr.h: No such file or directory
compilation terminated.
Attachment #601872 - Attachment is obsolete: true
Attachment #601872 - Flags: feedback?(Ms2ger)
Attachment #602211 - Flags: feedback?(Ms2ger)
(Assignee)

Updated

6 years ago
Attachment #602211 - Flags: feedback?(bsmith)
Created attachment 602308 [details] [diff] [review]
v 5.2

This should build
Attachment #602211 - Attachment is obsolete: true
Attachment #602211 - Flags: feedback?(bsmith)
Attachment #602211 - Flags: feedback?(Ms2ger)
Attachment #602308 - Flags: feedback?(bsmith)
Keywords: sec-review-needed
need to schedule a full review on the calendar
Whiteboard: [secr:curtisk]
(Assignee)

Comment 100

5 years ago
Created attachment 614197 [details] [diff] [review]
v 5.3 Builds - cleaning up runtime issues

Saving my work, still have some runtime issues to fix
Attachment #602308 - Attachment is obsolete: true
Attachment #602308 - Flags: feedback?(bsmith)
(Assignee)

Updated

5 years ago
Attachment #614197 - Attachment is patch: true
(Assignee)

Comment 101

5 years ago
Getting a segfault in PK11_GetInternalSlot:

82	../sysdeps/unix/syscall-template.S: No such file or directory.
	in ../sysdeps/unix/syscall-template.S
(gdb) break nsDOMCryptInternalAPI::GenerateKeyPair
Breakpoint 1 at 0x2aece09a35e9: file /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp, line 141.
(gdb) c
Continuing.

Breakpoint 1, nsDOMCryptInternalAPI::GenerateKeyPair (this=0x2aeceb807918, aPubKey=0x0, aPrivKey=0x0, aKeySizeInBits=2048, aAlgorithm=1)
    at /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp:141
141	  PK11SlotInfo *slot = NULL;
(gdb) n
142	  slot = PK11_GetInternalSlot();
(gdb) 

Program received signal SIGSEGV, Segmentation fault.
0x00002aecdec0e48c in TouchBadMemory () at /home/ddahl/code/moz/domcrypt/src/memory/mozalloc/mozalloc_abort.cpp:68
68	    gDummyCounter += *p;   // TODO annotation saying we know
(Assignee)

Comment 102

5 years ago
More clues...

Breakpoint 1, PK11_GetInternalSlot () at pk11slot.c:1797
1797	    SECMODModule * mod = SECMOD_GetInternalModule();
(gdb) 
1798	    PORT_Assert(mod != NULL);
(gdb) 

Program received signal SIGSEGV, Segmentation fault.
0x00002b027485448c in TouchBadMemory () at /home/ddahl/code/moz/domcrypt/src/memory/mozalloc/mozalloc_abort.cpp:68
68	    gDummyCounter += *p;   // TODO annotation saying we know
(Assignee)

Comment 103

5 years ago
Comment on attachment 614197 [details] [diff] [review]
v 5.3 Builds - cleaning up runtime issues

Crashing inside PK11_GetInternalSlot - Bad Memory access??
Attachment #614197 - Flags: feedback?(bsmith)
(Assignee)

Comment 104

5 years ago
backtrace: http://pastebin.mozilla.org/1565956
Depends on: 744938
Whiteboard: [secr:curtisk] → [secr:curtisk:744938]
(Assignee)

Comment 105

5 years ago
In trying to re-enable nsNSSShutdownObject parent in nsDOMCryptInternalAPI, I cannot build:

c++ -o nsDOMCryptAPI.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /home/ddahl/code/moz/domcrypt/src/config/gcc_hidden.h -D_IMPL_NS_LAYOUT -DMOZ_JSDEBUGGER -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux3.0\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES  -DOS_LINUX=1 -DOS_POSIX=1  -I/home/ddahl/code/moz/domcrypt/src/dom/base -I/home/ddahl/code/moz/domcrypt/src/dom/battery -I/home/ddahl/code/moz/domcrypt/src/dom/power -I/home/ddahl/code/moz/domcrypt/src/dom/network/src -I/home/ddahl/code/moz/domcrypt/src/dom/settings -I/home/ddahl/code/moz/domcrypt/src/dom/sms/src -I/home/ddahl/code/moz/domcrypt/src/dom/contacts -I/home/ddahl/code/moz/domcrypt/src/dom/src/events -I/home/ddahl/code/moz/domcrypt/src/dom/src/storage -I/home/ddahl/code/moz/domcrypt/src/dom/src/offline -I/home/ddahl/code/moz/domcrypt/src/dom/src/geolocation -I/home/ddahl/code/moz/domcrypt/src/dom/src/notification -I/home/ddahl/code/moz/domcrypt/src/dom/workers -I/home/ddahl/code/moz/domcrypt/src/content/xbl/src -I/home/ddahl/code/moz/domcrypt/src/content/xul/document/src -I/home/ddahl/code/moz/domcrypt/src/content/events/src -I/home/ddahl/code/moz/domcrypt/src/content/base/src -I/home/ddahl/code/moz/domcrypt/src/content/html/content/src -I/home/ddahl/code/moz/domcrypt/src/content/html/document/src -I/home/ddahl/code/moz/domcrypt/src/content/svg/content/src -I/home/ddahl/code/moz/domcrypt/src/layout/generic -I/home/ddahl/code/moz/domcrypt/src/layout/style -I/home/ddahl/code/moz/domcrypt/src/layout/xul/base/src -I/home/ddahl/code/moz/domcrypt/src/layout/xul/base/src/tree/src -I/home/ddahl/code/moz/domcrypt/src/ipc/chromium/src -I/home/ddahl/code/moz/domcrypt/src/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/home/ddahl/code/moz/domcrypt/src/js/xpconnect/src -I/home/ddahl/code/moz/domcrypt/src/js/xpconnect/wrappers -I/home/ddahl/code/moz/domcrypt/src/xpcom/ds  -I/home/ddahl/code/moz/domcrypt/src/dom/base -I. -I../../dist/include -I../../dist/include/nsprpub  -I/home/ddahl/code/moz/domcrypt/obj-x86_64-unknown-linux-gnu-debug/dist/include/nspr -I/home/ddahl/code/moz/domcrypt/obj-x86_64-unknown-linux-gnu-debug/dist/include/nss      -fPIC -fno-rtti -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -std=gnu++0x -ffunction-sections -fdata-sections -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -fno-omit-frame-pointer  -pthread -I/usr/include/gtk-2.0 -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0     -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/nsDOMCryptAPI.pp /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp
In file included from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.h:8:0,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp:5:
../../dist/include/nsDOMCryptInternalAPI.h:12:27: fatal error: nsNSSShutDown.h: No such file or directory

I tried EXPORTING nsNSSShutdown to no avail
(Assignee)

Comment 106

5 years ago
Created attachment 614909 [details] [diff] [review]
new attempt at nsNSSShutdownObject inheritance

trying to get the nsNSSShutdownObject inheritance working. This always fails once we build in dom/base/nsDOMCryptAPI.cpp/.h

I thought maybe my previous BadMemory erros might have something to do with this. Also, we cannot use NSS APIs without nsIDOMCryptInternalAPI deriving from nsNSSShutdwonObject
Attachment #614909 - Flags: feedback?(Ms2ger)
nsNSSShutDown.h is not exported by PSM so you cannot (currently) extend nsNSSShutdownObject in a class that is outside of PSM. If you need to do so in the interim, add that header to the EXPORTS section of PSM while you hack, until we find a more permanent solution.
(Assignee)

Comment 108

5 years ago
(In reply to Brian Smith (:bsmith) from comment #107)
> in the interim, add that header to the EXPORTS section of PSM while you
> hack, until we find a more permanent solution.

I tried to add the file to the EXPORTS in security/manager/ssl/Makefile.in - is that what you mean by 'the EXPORTS section of PSM'?
(Assignee)

Comment 109

5 years ago
Created attachment 615009 [details] [diff] [review]
nss loading issues
Attachment #615009 - Flags: feedback?(bsmith)
(Assignee)

Comment 110

5 years ago
based on irc conversation it looks like we need to authenticate with the keydb in order to use isPerm = PR_TRUE in PK11_GenerateKeyPair()

Is this the kind of thing we need here:
http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/signtool/util.c#921

if so, how is the password handled? Is there some other way to authenticate?
(Assignee)

Comment 111

5 years ago
Created attachment 615943 [details] [diff] [review]
5.4 GenerateKeyPair is working, test is passing

w00t!
Attachment #614197 - Attachment is obsolete: true
Attachment #614909 - Attachment is obsolete: true
Attachment #615009 - Attachment is obsolete: true
Attachment #614197 - Flags: feedback?(bsmith)
Attachment #614909 - Flags: feedback?(Ms2ger)
Attachment #615009 - Flags: feedback?(bsmith)
(Assignee)

Comment 112

5 years ago
Created attachment 616011 [details] [diff] [review]
v 5.5 Patch - PKGenerateKeyPair

Fixed a few things wrong with the pubkey property getters, added more checks to the test
Attachment #615943 - Attachment is obsolete: true
(In reply to David Dahl :ddahl from comment #112)
> Created attachment 616011 [details] [diff] [review]
> v 5.5 Patch - PKGenerateKeyPair
> 
> Fixed a few things wrong with the pubkey property getters, added more checks
> to the test

Wow. cool. How about separating bug for each methods? Very long :)
(Assignee)

Comment 114

5 years ago
(In reply to Channy Yun from comment #113)
> (In reply to David Dahl :ddahl from comment #112)
> > Created attachment 616011 [details] [diff] [review]
> > v 5.5 Patch - PKGenerateKeyPair
> > 
> > Fixed a few things wrong with the pubkey property getters, added more checks
> > to the test
> 
> Wow. cool. How about separating bug for each methods? Very long :)

Good idea. This is also only the 'internal' API - there is a separate bug for the DOM bindings: bug 721199. Perhaps this bug will just be for GenerateKeyPair. I will create bugs for encrypt/decrypt, sign/verify, hash, hmac, and symmetric crypto.
(Assignee)

Updated

5 years ago
Blocks: 746467
(Assignee)

Updated

5 years ago
Blocks: 746469
(Assignee)

Updated

5 years ago
Blocks: 746471
(Assignee)

Updated

5 years ago
Blocks: 746472
(Assignee)

Updated

5 years ago
Blocks: 746473
(Assignee)

Updated

5 years ago
Blocks: 746476
(Assignee)

Comment 115

5 years ago
Created attachment 617147 [details] [diff] [review]
v 5.5 Moved keygen to a thread, needs a test
Attachment #616011 - Attachment is obsolete: true

Updated

5 years ago
Duplicate of this bug: 741677
Whiteboard: [secr:curtisk:744938] → [sec-assigned:curtisk:744938]
(Assignee)

Comment 117

5 years ago
Created attachment 619745 [details] [diff] [review]
Patch 5.6 - Keygen on thread tests passing

Saving work, seeing WARNING: nsExceptionService ignoring thread destruction after shutdown: file /home/ddahl/code/moz/domcrypt/src/xpcom/base/nsExceptionService.cpp, line 199
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /home/ddahl/code/moz/domcrypt/obj-x86_64-unknown-linux-gnu-debug/xpcom/build/nsThreadUtils.cpp, line 168
Attachment #617147 - Attachment is obsolete: true
(Assignee)

Comment 118

5 years ago
Created attachment 620935 [details] [diff] [review]
Patch 5.7 build fails, just saving work

Working on PKEncrypt
(Assignee)

Comment 119

5 years ago
Created attachment 621242 [details] [diff] [review]
Patch 5.8: PKEncrypt builds

Saving work, on to the scriptable interface with off main thread encrypt
Attachment #619745 - Attachment is obsolete: true
Attachment #620935 - Attachment is obsolete: true
(Assignee)

Comment 120

5 years ago
Created attachment 641179 [details] [diff] [review]
Added PKEncryptResult interface, fixed some crashes
Attachment #621242 - Attachment is obsolete: true
Flags: sec-review?(curtisk)
Keywords: sec-review-needed
(Assignee)

Comment 121

5 years ago
Created attachment 687244 [details] [diff] [review]
Latest Patch, builds!

This thing builds, going to test it
Attachment #641179 - Attachment is obsolete: true
(Assignee)

Comment 122

5 years ago
Test is failing like so:

TEST-INFO | (xpcshell/head.js) | test 3 pending
TEST-INFO | /home/ddahl/code/moz/domcrypt/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/dom/tests/unit/test_domcryptapi.js | Starting testPKEncrypt

TEST-PASS | undefined | [undefined : undefined] true == true
WARNING: NS_ENSURE_TRUE(subjectPublicKeyInfo != NULL) failed: file /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp, line 481

the code here is:

  CERTSubjectPublicKeyInfo* subjectPublicKeyInfo;
  subjectPublicKeyInfo = SECKEY_DecodeDERSubjectPublicKeyInfo(derEncodedKey);
  NS_ENSURE_TRUE(subjectPublicKeyInfo != NULL, NS_ERROR_FAILURE);

I am wondering if the key that is generated is incorrect kind/size/whatever - I can only generate a 512 bit key otherwise things crash.

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPIPKEncryptRunnable.cpp, line 77
(Assignee)

Comment 123

5 years ago
Segfault inside PK11_GenerateKeyPair when generating a 2048 key. more debug info later
(Assignee)

Updated

5 years ago
Blocks: 820058
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
(Assignee)

Comment 124

4 years ago
Resolving Invalid as the W3C API is vastly different, see bug 865789
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
Keywords: dev-doc-needed
Flags: sec-review?(curtisk)
You need to log in before you can comment on or make changes to this bug.