User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:14.0) Gecko/20100101 Firefox/14.0.1 Build ID: 20120713225548 Steps to reproduce: you send chat messages Actual results: they are transferred either unencrypted in plain text or in an easy to crack encryption by the chat provider and governments. all your chat history will be stored in public hands... for all times! Expected results: the chat should have an OCR encryption (Off The Record). so they are only decrypteable by the chat partners.

There's probably a dupe of this somewhere, lots of people would like to see this.

(In reply to Daniel Veditz [:dveditz] from comment #1) > There's probably a dupe of this somewhere, lots of people would like to see > this. Yep in instant bird's bugzilla.

This should be fairly trivial for an extension to add (using js-ctypes, probably). I think we've added all the necessary hooks into the core to use this, if they aren't there we should definitely add them! I'm not sure if this is something we would want to fully include in the core code though. The Instantbird bug is: https://bugzilla.instantbird.org/show_bug.cgi?id=877 and includes some other links that might be of interest.

It seems reasonable to both have it and to have it on by default - Adium has had great success with that mode of OTR usage.

(In reply to Jacob Appelbaum from comment #4) > It seems reasonable to both have it and to have it on by default If it requires little to no effort for users, I think this is reasonable. If it requires manual intervention and complicated crypto setups, I strongly disagree. > Adium has had great success with that mode of OTR usage. What does this mean? "great success" in what way?

(In reply to Patrick Cloke [:clokep] from comment #5) > (In reply to Jacob Appelbaum from comment #4) > > It seems reasonable to both have it and to have it on by default > If it requires little to no effort for users, I think this is reasonable. If > it requires manual intervention and complicated crypto setups, I strongly > disagree. Sure - that's exactly the idea and why OTR has worked so well. It Just Works and if users wish to verify conversations, etc - they are able to do so. Otherwise, OTR isn't in the way for anyone except a data logging attacker. > > > Adium has had great success with that mode of OTR usage. > What does this mean? "great success" in what way? It's enabled by default, without users having to do anything at all, and it works automagically. They sadly log all their chats which has led to some rather epic failures. Other than that, it's actually pretty fantastic because it requires *no setup* at all.

where can we see the progress on this? I think it is really important to implement this, because otherwise Pidgin would stay the choice (which would be a pity)

Progress would be seen on this bug. Nobody is working on it atm, feel free to do so.

This is really a security breach in thunderbird. please someone take this and work on it please! please! 8-) maybe a quick fix would be an extension that could send all chat-messages through a running instance of pidgin in the background

The Instantbird bug is: https://bugzilla.instantbird.org/show_bug.cgi?id=877 and includes some other links that might be of interest.

(In reply to Ruben from comment #10) > This is really a security breach in thunderbird. This is an enhancement, it is not a critical issue. That's reserved for crash bugs, etc. (See https://bugzilla.mozilla.org/page.cgi?id=fields.html#bug_severity if you have more questions about this.) > maybe a quick fix would be an extension that could send all chat-messages > through a running instance of pidgin in the background That sounds awful. Please don't do that.

sure it sounds a hack, to use an external program to pipe encrypted messages, but why is no one taking control of this bug? I would even donate something if you would start working on this. Thunderbird is such a great communication central, if it only had chat-encryption...

(In reply to Ruben from comment #13) > sure it sounds a hack, to use an external program to pipe encrypted > messages It is a hack, and I doubt it would be a simple thing to do. > but why is no one taking control of this bug? All of the people that work on the chat backend are volunteers: we work on things that interest us. If someone wants to fix this bug, we will certainly help out, provide reviews, answers questions, etc. etc. If you have more questions about this, come talk to us on IRC (#maildev or #instantbird on irc.mozilla.org).

Please someone work on this. I really don't want to use chat anymore without encryption

Given recent revelations about mass, pervasive automated chat log collection and storage, this bug really should be up-voted in the priority list. To quote another poster here: it really a serious security breach in Thunderbird.

I had a look as to what it would take to implement this. The OTR library mentioned on the site is LGPL, so we could go ahead and use that if we desired. Otherwise, we'd need the following things: 1. Storage of keys for each account and each contact. Our keys need secure keystore, I think (we could probably get away with stashing them in the password manager and be hacky :-P), but I think everything else could be plaintext. 2. Modular exponentiation library. I don't think NSS exports its MPI routines publicly, and the WebCrypto drafts certainly aren't giving us any support for big integer other than as a giant byte array. 3. DH key exchange, AES, HMAC+SHA-256, DSA. WebCrpyto doesn't appear to have the last algorithm exported (I can't find rationale why...), but NSS almost certainly has it implemented somewhere. 4. Secure random number generation. This is obviously window.crypto.getRandomValues() :-). #2 appears to be the hardest to get from "standard" APIs, but also the easiest to reimplement from scratch. The only stumbling block I would see with using WebCrypto (besides the fact that it's not implemented yet) is the lack of support for DSA.

For reference, we discussed this on IRC a bit: http://logbot.glob.com.au/?c=mozilla%23maildev&s=2+Sep+2013&e=3+Sep+2013#c95469 Thanks for looking into how much work this would be Joshua. There's a few options on how we could do this though: 1. Reimplement it from scratch (which would involve the work in comment 17). 2. Shim the LGPL libotr library using ctypes. 3. Shim the MPL2 node/browser code from https://github.com/arlolra/otr I don't think 3. is a good idea, a lot of the code included uses JS versions of things that chrome code can probably access faster implementations of via XPCOM. Additionally, this is cross-browser, which is a waste for Gecko only code. For example, it uses a library called salsa20 to get good random numbers, which we can get from window.crypto.getRandomValues() and crypto.js most likely includes a lot of things we have from NSS. In terms of comment 17, #1: I had assumed we'd be using the password manager and generating unique URIs for each conversation or whatever, but maybe Florian had a better idea.

I'd also really like to see OTR support. For me it's a critical (read necessary) feature of any XMPP client.

This is a WIP patch that contains lot of extra code that will be removed once approved that it isn't needed.

How can I try this patch? Into which folder do I have to copy the file imOffTheRecord.js in my thunderbird profile? Would it work in thunderbird 17.0.8? Which other tools or extensions do I have to have installed?

(In reply to Ruben from comment #21) > How can I try this patch? Into which folder do I have to copy the file > imOffTheRecord.js in my thunderbird profile? > > Would it work in thunderbird 17.0.8? > > Which other tools or extensions do I have to have installed? This patch will not work it is a WIP ("work in progress") and is not functional.

Comment on attachment 814992 [details] [diff] [review] Primary patch Review of attachment 814992 [details] [diff] [review]: ----------------------------------------------------------------- For your own sanity, I would advise you to start with simpler steps of the OTR protocol: reading/writing MPI rules and packing/unpacking the data messages. ::: chat/modules/imOffTheRecord.js @@ +10,5 @@ > +const kWhitespaceTagStart = > + "\x20\x09\x20\x20\x09\x09\x09\x09" + "\x20\x09\x20\x09\x20\x09\x20\x20"; > + > +const kWhitespaceVersion = [ > + "\x20\x09\x20\x09\x20\x20\x09\x20", // Version 1 must be sent first. The webpage only lists documentation for versions 2 and 3, so make sure that you don't attempt to claim support for version 1. @@ +29,5 @@ > + "E485B576", "625E7EC6", "F44C42E9", "A637ED6B", "0BFF5CB6", "F406B7ED", > + "EE386BFB", "5A899FA5", "AE9F2411", "7C4B1FE6", "49286651", "ECE45B3D", > + "C2007CB8", "A163BF05", "98DA4836", "1C55D39A", "69163FA8", "FD24CF5F", > + "83655D23", "DCA3AD96", "1C62F356", "208552BB", "9ED52907", "7096966D", > + "670C354E", "4ABC9804", "F1746C08", "CA237327", "FFFFFFFF", "FFFFFFFF"].join(""); It might be better to use typed arrays here instead of strings; if you don't, then you might accidentally send the wrong thing to NSS or WebCrypto [if/when we ever switch to that]. @@ +50,5 @@ > +const PUBKEYTYPE = { > + DSA: 0x0000 > +} > + > +function otr() { What, no long documentation comments? :-P You'll definitely need comments to at least indicate how otr is managed here: is it a wrapper for a single connection, or is it more like a service? Also, while I'm mentioning it, you'll want to include pointers to the specification you're following somewhere in documentation. @@ +61,5 @@ > + myInstanceTag: 0x01010101, > +// libnss3: ctypes.open("../../hunderbird-binary/mozilla/dist/bin/libnss3.so"); > +// pkcs11c: ctypes.open("../../mozilla/security/nss/lib/softoken/pkcs11c.c"); > + // Supported version: so far, valid values are 1, 2, and 3. > + versions: [], I worry that you are attempting to be too clever here. We are probably not going to support version 1 (no documentation for it is easily accessible). @@ +456,5 @@ > + let senderInstanceTag = this.myInstanceTag; > + let receiverInstanceTag = 0; > + let data; > + let r = Math.floor(Math.random() * (1 << 128) + 1); > + let x = Math.floor(Math.random() * ((1 << 320) - (1 << 319) + 1) + 1 << 319); These two lines are irredeemably incorrect.

Comment on attachment 814992 [details] [diff] [review] Primary patch >diff --git a/chat/modules/imOffTheRecord.js b/chat/modules/imOffTheRecord.js >+Components.utils.import("resource://gre/modules/ctypes.jsm"); >+Components.utils.import("resource://gre/libnss3.so"); This doesn't seem right. >+const MSGSTATE = { >+const AUTHSTATE = { >+const PUBKEYTYPE = { Please use the kCamelCase for these types of constants (as the ones above them are done). >+function otr() { I'd like a comment above this saying what the otr object is and how it is used and what it implements (i.e. links to the specifications). >+otr.prototype = { >+ msgstate: MSGSTATE.PLAINTEXT, >+ authstate: AUTHSTATE.NONE, >+ myInstanceTag: 0x01010101, Add comments above these saying what they are! >+ // Set Query Message to send >+ setQueryMessage: function() { This comment doesn't describe anything you can't get by reading the function name, please provide more indepth details about what it's trying to do. >+ // Get the OTR version requested by the sender Can you please expand upon what this actually means? Also, nit: end your comments in a period. >+ getQueryMessage: function(qMsg) { >+ msg = qMsg.substring(4, qMsg.length - 1); Nit: Add a "let" before here. What if the string isn't four characters wrong, does that error? "msg" is also probably a vague variable name. >+ verifySenderInstance: function(tag) { >+ verifyRecipientInstance: function(tag) { Comments! >+ initNSS : function() { [...] >+ this.log("Initializing NSS types and function declarations..."); Is this defined? >+ this.nss = {}; >+ this.nss_t = {}; These should be defined in the prototype with comments. >+ sendDHCommitMessage: function() { Comments! I'm not going to review this function since the code seems very WIPy. Some thoughts need to be given to API design, most likely, will their be one OTR object (i.e. a service) for all current conversations or will each conversation own an OTR object? I'd like to see tests being developed alongside the code. If it isn't clear from my comments above...I want to see this code VERY heavily commented. You'll get tired of my reviews that say to add comments, I'm sorry...but you'll thank me later when we find bugs. :)

Another reason not to support v1 is that there were known weaknesses which were resolved in v2, http://www.dmi.unict.it/diraimondo/web/wp-content/uploads/papers/otr.pdf

There is a bounty for this bug (of currently 105 USD) at https://www.bountysource.com/issues/1012564-otr-encryption-in-thunderbird-chat

I started working on ctypes wrapper for libotr: https://github.com/arlolra/ctypes-otr/blob/master/chrome/content/libotr.js Would appreciate some early commentary. Thanks.

Sorry for taking so long, was busy with academics. There are a few areas in the patch which need discussion. There is not a considerable change from the last version. Please let me know how to proceed from here. Thanks.

Comment on attachment 8343982 [details] [diff] [review] Primary patch v2 Review of attachment 8343982 [details] [diff] [review]: ----------------------------------------------------------------- I gave some feedback on IRC already that I think is summarized as: - I see (by eye) a lot of typos/syntax errors. I'd really like this code to be "run" somehow before reviews are requested. (Even if dummy data is just fed in.) - I again encouraged developing some tests and working toward getting those working. - There was a lot of whitespace changes that created an invalid style. We discussed somewhat how this could be tested, it'd be great to know how you plan to do this in the future so that I can take a look at it. ::: chat/modules/imOffTheRecord.js @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +Components.utils.import("resource://gre/modules/ctypes.jsm"); > +//Components.utils.import("resource://gre/libnss3.so"); Can we remove this? @@ +13,5 @@ > +// Special tag for Requesting an OTR conversation. > +// The semantics of the whitespace tag are that the requester is merely > +// indicating that she is willing and able to have an OTR conversation. > +const kWhitespaceTagStart = > +"\x20\x09\x20\x20\x09\x09\x09\x09" + "\x20\x09\x20\x09\x20\x09\x20\x20"; This should have two spaces in front of it, this happens a lot in this file, I'm not going to comment again. Why is this split into two parts? @@ +39,5 @@ > + > +// Diffie-Hellman group computations from RFC 3526 with 1536-bit modulus (hex, > +// big-endian). > +// kDiffieHellmanGroup is a typed array of 32-bit unsigned integers. > +const kDiffieHellmanGroup = new Uint23Array ( Obvious typo in the class name. Remove the space before ( @@ +41,5 @@ > +// big-endian). > +// kDiffieHellmanGroup is a typed array of 32-bit unsigned integers. > +const kDiffieHellmanGroup = new Uint23Array ( > +[ "FFFFFFFF", "FFFFFFFF", "C90FDAA2", "2168C234", "C4C6628B", "80DC1CD1", > + "29024E08", "8A67CC74", "020BBEA6", "3B139B22", "514A0879", "8E3404DD", This formatting is wrong, I'd put the ] on the line above and have two spaces before it. @@ +60,5 @@ > +// as well as some other information (such as encryption keys). > +const kMsgState = { > + PLAINTEXT: 0, // State indicating that outgoing messages are sent without encryption > + ENCRYPTED: 1, // State indicating that outgoing messages are sent encrypted > + FINISHED: 2 // State indicating that outgoing messages are not delivered at all Do these comments really add that much? There's a lot of repetition. (This happens further on with some other constants.) Please use full sentences in comments (e.g. they should end in .!?‽) @@ +79,5 @@ > +} > + > +// XXX This needs a thought > +// otr object: A separate object will be instantiated for every > +// chat conversation started. We probably want a constructor then! function otr(<parameters>) { ...stuff here... } otr.prototype = { ... } @@ +403,5 @@ > + }, > + > + // OTR users have long-lived public keys that they use for authentication > + // (but not encryption). > + myPublicAuthenticationKey: new this.nss_t.SECItem, if SECItem is a function, please include (). Otherwise...I'm not sure why there's a new keyword here. @@ +434,5 @@ > + if (this.versions.length == 0) { > + queryMessage += "v?"; > + print(queryMessage); > + return; > + } This special case doesn't seem to be necessary, wouldn't the generic code below work? @@ +436,5 @@ > + print(queryMessage); > + return; > + } > + > + let versions = this.versions; This local variable seems unnecessary. @@ +448,5 @@ > + // Get the version(s) of the OTR protocol the > + // other person is willing to use for the communication. > + getQueryMessage: function(queryMessage) { > + // Assuming the first 5 letters of qMsg will be ?OTRv and the last letter > + // will be ? which is true for versions 2, 3 and for all the future versions. We should check this and not assume. @@ +472,5 @@ > + if (tag < 0x00000100) > + return false; > + > + return true; > + }, Maybe just: verifySenderInstanceTag: function(tag) tag >= 0x000001000, @@ +483,5 @@ > + if (tag > 0 && tag < 0x00000100) > + return false; > + > + return true; > + }, A similar thing to above can be done here.

Comment on attachment 8343982 [details] [diff] [review] Primary patch v2 Review of attachment 8343982 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/modules/imOffTheRecord.js @@ +23,5 @@ > +// The tag consists of 16 bytes, followed by one or more sets of 8 bytes > +// indicating the version of OTR the user is willing to use. > +const kWhitespaceVersion = [ > +"\x20\x20\x09\x09\x20\x20\x09\x20", // to indicate a willingness to use OTR version 2. > + "\x20\x20\x09\x09\x20\x20\x09\x09"]; // to indicate a willingness to use OTR version 3. I don't know style in chat code, but I usually assume style for things like this to be: const kWhitespaceVersion = [ blah, foo, ]; @@ +254,5 @@ > + this.nss.SEC_OID_HMAC_SHA1 = 294; > + this.nss.SEC_OID_PKCS1_RSA_ENCRYPTION = 16; > + > + // security/nss/lib/pk11wrap/pk11pub.h#286 > + // SECStatus PK11_GenerateRandom(unsigned char *data,int len); I don't think adding comments of the C version of the ctypes-declared functions is very helpful, personally. @@ +414,5 @@ > + let dsa_privateKey = new this.nss_t.DSAPrivateKey; > + > + // Generate the p, q, g parameters. > + this.nss.pqg_ParamGen(128, // length of p in bits > + 128, // length of q in bits Actually, the OTR specification says that q must be 20 bytes, or 160 bits.

Comment on attachment 8343982 [details] [diff] [review] Primary patch v2 Review of attachment 8343982 [details] [diff] [review]: ----------------------------------------------------------------- Keep it up! ::: chat/modules/imOffTheRecord.js @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +Components.utils.import("resource://gre/modules/ctypes.jsm"); Maybe use the shorthand you defined below? Cu.import ... @@ +89,5 @@ > + // @authState: State that indicates that the authentication protocol is not > + // currently in progress. This is the initial state. > + authState: kAuthState.NONE, > + > + myInstanceTag: 0x01010101, Instance tag needs to be initialized. @@ +97,5 @@ > + this.nss = {}, > + this.nss_t = {}, > + > + initNSS : function() { > + // We use NSS for the crypto ops, which needs to be initialized before Is this something that needs to happen for each conversation? @@ +403,5 @@ > + }, > + > + // OTR users have long-lived public keys that they use for authentication > + // (but not encryption). > + myPublicAuthenticationKey: new this.nss_t.SECItem, The long-lived authentication key will be used by many conversations, so maybe calling new here isn't what you want. @@ +407,5 @@ > + myPublicAuthenticationKey: new this.nss_t.SECItem, > + > + // The current version of the OTR protocol only supports DSA public keys, > + // but there is a key type marker for future extensibility. > + generatePublicAuthenticationKey: function() { Should generating a authentication key be part of the otr.prototype? @@ +495,5 @@ > + // this.nss_t.DHParams, this.nss_t.DHPrivateKey and this.nss_t.DHPublicKey > + // and functions this.nss.PK11_GenerateRandom(), this.nss.GenerateKeyPair(), > + // this.nss.DH_NewKey() and this.nss.DH_GenParam() useful, but wasn't sure > + // how to begin. > + sendDHCommitMessage: function() { Might want some sort of helper for building up these messages.

Comment on attachment 8343982 [details] [diff] [review] Primary patch v2 Review of attachment 8343982 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, why didn't that get marked as reviewed?

Comment on attachment 8343982 [details] [diff] [review] Primary patch v2 Thanks :)

The patch, addresses the nits, gives a shape to the idea. Few things don't work, would appreciate some functional advice on what the issue this patch faces. The patch also includes a way to trigger the testing function 'executeMe()' Just open the compose window and it gets executed. Needless to say, not much has been done from the previous patch and this patch has been ready for a while but couldn't find the ways to proceed, so thought to ask. Thanks.

Comment on attachment 8373028 [details] [diff] [review] WIP patch v3 Review of attachment 8373028 [details] [diff] [review]: ----------------------------------------------------------------- There is this issue which I would like to bring into notice. This should work fine, but it crashes (segmentation fault); not sure why. Any possible reasons? or suggestions that I can try? Thanks. ::: chat/modules/imOffTheRecord.jsm @@ +546,5 @@ > + let type = 0x02; // D-H commit message type. > + let senderInstanceTag = this.myInstanceTag; > + let receiverInstanceTag = 0; // since the other party may not have identified their instance tag yet. > + let r = new ctypes.unsigned_char; > +// nss.GenerateRandom(r.address(), 128); This line crashes TB. I don't know why.

(In reply to Suyash Agarwal (:sshagarwal) from comment #35) > Comment on attachment 8373028 [details] [diff] [review] > WIP patch v3 > > + let r = new ctypes.unsigned_char; > > +// nss.GenerateRandom(r.address(), 128); > > This line crashes TB. I don't know why. Do I need to create a typed array for handling these?

Comment on attachment 8373028 [details] [diff] [review] WIP patch v3 Review of attachment 8373028 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/modules/imOffTheRecord.jsm @@ +408,5 @@ > + > +// OTR users have long-lived public keys that they use for authentication > +// (but not encryption). > +var myPublicAuthenticationKey = null; > +var myPrivateAuthenticationKey = null; I don't know if the authentication keys should be unique per user or be unique per account. Without having played with an OTR-enabled application myself, I cannot answer this question, but it does matter for the difference. @@ +423,5 @@ > + let pqg_Params = new nss.PQGParams; > + let pqg_Verify = new nss.PQGVerify; > + > + // Generate the p, q, g parameters. > + if (nss.PQG_ParamGen(0, pqg_Params.address(), pqg_Verify.address())) { Looking at the implementation of NSS, I think this is wrong. The "0" here really ought to be an "8" (that's a 1024-bit DSA key, which is as large as we can go yet still have q be 160 bits per the OTR spec). @@ +429,5 @@ > + return; > + } > + // Get the public and private keys. > + myPublicAuthenticationKey = new nss.SECKEYPublicKey.ptr; > + myPrivateAuthenticationKey = new nss.SECKEYPrivateKey.ptr; This is redundant. @@ +432,5 @@ > + myPublicAuthenticationKey = new nss.SECKEYPublicKey.ptr; > + myPrivateAuthenticationKey = new nss.SECKEYPrivateKey.ptr; > + myPrivateAuthenticationKey = nss.GenerateKeyPair(this.slotInfoPtr, > + kMechanism.DSA, // CK_MECHANISM for DSA key pair #security/nss/lib/util/pkcs11t.h > + pqg_Params.address(), this.myPublicAuthenticationKey, You need to take the address of the key here, it's an outparam. @@ +447,5 @@ > +// chat conversation started. > +function otr() { > + log("running otr constructor"); > + // Initializing the NSS library datatypes and functions. > + initNSS(); You should only be initializing nss if it hasn't been initialized. @@ +452,5 @@ > + // If the DSA authentication keys aren't generated > + // so far, we generate them now. > + if (myPublicAuthenticationKey == null || > + myPrivateAuthenticationKey == null) > + generateAuthenticationKeys(); In any case, you'll eventually need to have logic that is able to persist the authentication key(s) somewhere. @@ +546,5 @@ > + let type = 0x02; // D-H commit message type. > + let senderInstanceTag = this.myInstanceTag; > + let receiverInstanceTag = 0; // since the other party may not have identified their instance tag yet. > + let r = new ctypes.unsigned_char; > +// nss.GenerateRandom(r.address(), 128); This is equivalent to saying in C: char x; GenerateRandom(&x, 128); What you really want is: char x[128]; GenerateRandom(x, sizeof(x)); For that, you'll need an array (I think that's ctypes.unsigned_char.array(128), but I could be mistaken).

(In reply to Joshua Cranmer [:jcranmer] from comment #37) > Comment on attachment 8373028 [details] [diff] [review] > WIP patch v3 > > Review of attachment 8373028 [details] [diff] [review]: Thanks for a quick look. > > > + // Get the public and private keys. > > + myPublicAuthenticationKey = new nss.SECKEYPublicKey.ptr; > > + myPrivateAuthenticationKey = new nss.SECKEYPrivateKey.ptr; > This is redundant. Why redundant? > @@ +432,5 @@ > > + myPrivateAuthenticationKey = nss.GenerateKeyPair(this.slotInfoPtr, > > + kMechanism.DSA, // CK_MECHANISM for DSA key pair > > + pqg_Params.address(), this.myPublicAuthenticationKey, > > You need to take the address of the key here, it's an outparam. I understand you mean |myPrivateAuthenticationKey.address()|. Please correct me if I'm wrong. > @@ +452,5 @@ > > + if (myPublicAuthenticationKey == null || > > + myPrivateAuthenticationKey == null) > > + generateAuthenticationKeys(); > > In any case, you'll eventually need to have logic that is able to persist > the authentication key(s) somewhere. Ya, I think once we decide whether to persist them per user or per account basis, we can do this as well. > @@ +546,5 @@ > > +// nss.GenerateRandom(r.address(), 128); > > What you really want is: > char x[128]; > GenerateRandom(x, sizeof(x)); > > For that, you'll need an array (I think that's > ctypes.unsigned_char.array(128), but I could be mistaken). Thanks.

(In reply to Suyash Agarwal (:sshagarwal) from comment #38) > > > + // Get the public and private keys. > > > + myPublicAuthenticationKey = new nss.SECKEYPublicKey.ptr; > > > + myPrivateAuthenticationKey = new nss.SECKEYPrivateKey.ptr; > > This is redundant. > Why redundant? You reassign myPrivateAuthenticationKey in the next line. > > @@ +432,5 @@ > > > + myPrivateAuthenticationKey = nss.GenerateKeyPair(this.slotInfoPtr, > > > + kMechanism.DSA, // CK_MECHANISM for DSA key pair > > > + pqg_Params.address(), this.myPublicAuthenticationKey, > > > > You need to take the address of the key here, it's an outparam. > I understand you mean |myPrivateAuthenticationKey.address()|. Please correct > me if I'm wrong. this.myPublicAuthenticatioKey.address()

Comment on attachment 8373028 [details] [diff] [review] WIP patch v3 I don't have other feedback to add right now besides my continued suggestion of making a real test harness for this.

Comment on attachment 8373028 [details] [diff] [review] WIP patch v3 Review of attachment 8373028 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/modules/imOffTheRecord.jsm @@ +40,5 @@ > + */ > +const kQueryPrefix = "?OTR"; > + > +// Protocol version. > +const kVersion = ["\x00\x01", "\x00\x02", "\x00\x03"]; don't implement v1 @@ +408,5 @@ > + > +// OTR users have long-lived public keys that they use for authentication > +// (but not encryption). > +var myPublicAuthenticationKey = null; > +var myPrivateAuthenticationKey = null; generally, per account @@ +464,5 @@ > + // @authState: State that indicates that the authentication protocol is not > + // currently in progress. This is the initial state. > + authState: kAuthState.NONE, > + > + myInstanceTag: 0x01010101, shouldn't be part of the prototype. maybe generateInstanceTag() instead @@ +466,5 @@ > + authState: kAuthState.NONE, > + > + myInstanceTag: 0x01010101, > + // Supported version: so far, valid values are 2 and 3. > + versions: [2], 3? @@ +475,5 @@ > + * Handle versions 2, 3 and future versions (if any, but not so far). > + * If sender is unwilling to speak any version of the protocol, > + * this.versions will be empty. > + */ > + setQueryMessage: function() { maybe this should be getQueryMessage() and the one below parseQueryMessage(). "set" doesn't seem right @@ +493,5 @@ > + log("Incorrect query message received."); > + return; > + } > + > + let messageString = queryMessage.substring(5, queryMessage.length - 1); are you going to check the last char is a ? @@ +499,5 @@ > + // If the sender has mentioned any version(s) of the protocol she is willing to use. > + if (messageString.length) { > + let index = 0; > + while (index < messageString.length) > + this.versions.push(messageString[index++]); you're pushing this onto the prototype. probably not what was intended. also, above the version is an int. @@ +544,5 @@ > + sendDHCommitMessage: function() { > + let version = 0x0003; // protocol version for this protocol. > + let type = 0x02; // D-H commit message type. > + let senderInstanceTag = this.myInstanceTag; > + let receiverInstanceTag = 0; // since the other party may not have identified their instance tag yet. you probably want this.theirInstanceTag, which would be defaulted to 0 @@ +564,5 @@ > + return encryptedMsg; > + } > +}; > + > +// Test runs I'm going to agree with pcloke that this is begging for a test harness.

Comment on attachment 8373028 [details] [diff] [review] WIP patch v3 Thanks :)

Why are you using jsctypes to call a bunch of NSS functions, instead of writing just a few C/C++ functions and then using jsctypes (or XPCOM) to call them? The patch would be much simpler if the NSS-facing bits were redone in C++, AFAICT.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #43) > Why are you using jsctypes to call a bunch of NSS functions, instead of > writing just a few C/C++ functions and then using jsctypes (or XPCOM) to > call them? The patch would be much simpler if the NSS-facing bits were > redone in C++, AFAICT. For various development reasons, Thunderbird and Instantbird are driven towards wanting to do more in JS than C++. In the long term, we want to use WebCrypto, but that has not yet been implemented, so we're using NSS for now.

The bounty on this issue is up to 500 USD now https://www.bountysource.com/issues/1012564-otr-encryption-in-thunderbird-chat

I think the way we should move forward with this is bug 954310 instead of duplicating effort.

> arlolra submitted a claim on the bounty 3 hours ago. Is this implemented now? the bug at https://bugzilla.mozilla.org/show_bug.cgi?id=954310 is still open, so shouldn't the bounty be transferred to that bug?

The bounty of 650$ should be transferred here: https://www.bountysource.com/issues/5925617-add-support-for-otr-and-encrypted-chats