1307603 - SASL ECDSA-NIST256P-CHALLENGE

Status: UNCONFIRMED

(Chat Core :: IRC, defect)

Description

[arlolra]

Attachment: 0001-SASL-ECDSA-NIST256P-CHALLENGE.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36

Steps to reproduce:

Here's WIP patch just to see if something like this will ever be uplifted.

Comment 1

[Patrick Cloke [:clokep]]

Potentially, can you give some links to:
* Documentation
* What networks actually use this

Comment 2

[arlolra]

Comment on attachment 8797777 [details] [diff] [review]
0001-SASL-ECDSA-NIST256P-CHALLENGE.patch

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

There's some more discussion about this here,
https://trac.torproject.org/projects/tor/ticket/20205#comment:4

::: chat/protocols/irc/ircSASL.jsm
@@ +25,5 @@
> +  the public key on the server.
> +
> +  crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign"])
> +  .then((kp) => {
> +    crypto.subtle.exportKey("jwk", kp.privateKey).then((o) => console.log(JSON.stringify(o)));

Grrr, this isn't the right export format for the private key anymore.  Needs fixin'

Comment 3

[arlolra]

(In reply to Patrick Cloke [:clokep] from comment #1)
> Potentially, can you give some links to:
> * Documentation

see https://trac.torproject.org/projects/tor/ticket/20205#comment:4
but yeah, i'll improve that

> * What networks actually use this

freenode, see https://freenode.net/news/tor-online

Comment 4

[arlolra]

fwiw, this patch works as is ... I'm using it right now to connect to freenode's onion.  some things to clean up though,

 * the docs (as you said),
 * is elliptic.js really the right approach here?  (see the above link to trac where the webcrypto api decidedly wasn't)

Comment 5

[arlolra]

Attachment: 0001-SASL-ECDSA-NIST256P-CHALLENGE.patch from comment 4

Updated the doc comment so that you can test it.

Comment 6

[arlolra]

Attachment: 0001-SASL-ECDSA-NIST256P-CHALLENGE.patch from comment 5

Cleanup cases.

Comment 7

[aleth [:aleth]]

Comment on attachment 8802390 [details] [diff] [review]
0001-SASL-ECDSA-NIST256P-CHALLENGE.patch from comment 5

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

::: chat/protocols/irc/ircSASL.jsm
@@ +47,5 @@
> +    });
> +  });
> +
> +  Then, copy the private key to the preference `messenger.account.accountN.ecdsa`
> +  in the config editor.

That's not really secure storage you know ;) The password manager would be better (but a lot more work).

@@ +59,4 @@
>  
>    commands: {
>      "AUTHENTICATE": function(aMessage) {
> +      let ecdsa = this.imAccount.wrappedJSObject.prefBranch.getCharPref("ecdsa");

There must be a better way to pass this along.

@@ +75,5 @@
> +        let challenge = aMessage.params[0];
> +
> +        let EC = elliptic.ec;
> +        let ec = new EC('p256');
> +        let key = ec.keyFromPrivate(ecdsa, 'hex');

SubtleCrypto.* can't be used for this? (to replace the library)

Comment 8

[aleth [:aleth]]

Comment on attachment 8802390 [details] [diff] [review]
0001-SASL-ECDSA-NIST256P-CHALLENGE.patch from comment 5

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

::: chat/protocols/irc/ircSASL.jsm
@@ +21,5 @@
> +  ECDSA-NIST256P-CHALLENGE
> +
> +  Paste the following code snippit in the Firefox web console to generate a
> +  private key (logged as hex string) and the irc command to execute to set
> +  the public key on the server.

Why not add an IRC command to do this? Expecting users to read code comments is optimistic...

Comment 9

[arlolra]

Comment on attachment 8802390 [details] [diff] [review]
0001-SASL-ECDSA-NIST256P-CHALLENGE.patch from comment 5

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

::: chat/protocols/irc/ircSASL.jsm
@@ +47,5 @@
> +    });
> +  });
> +
> +  Then, copy the private key to the preference `messenger.account.accountN.ecdsa`
> +  in the config editor.

Good point.

@@ +75,5 @@
> +        let challenge = aMessage.params[0];
> +
> +        let EC = elliptic.ec;
> +        let ec = new EC('p256');
> +        let key = ec.keyFromPrivate(ecdsa, 'hex');

For a more complete explanation, see https://trac.torproject.org/projects/tor/ticket/20205#comment:4

But, basically, the WebCrypto API hashes the value before signing it, which isn't what the protocol wants.

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 8802390 [details] [diff] [review]
0001-SASL-ECDSA-NIST256P-CHALLENGE.patch from comment 5

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

::: chat/protocols/irc/elliptic.jsm
@@ +1,1 @@
> +/* This software is licensed under the MIT License.

I didn't look at this file in-depth, but it doesn't seem too crazy. Did you make any modifications?

::: chat/protocols/irc/ircSASL.jsm
@@ +21,5 @@
> +  ECDSA-NIST256P-CHALLENGE
> +
> +  Paste the following code snippit in the Firefox web console to generate a
> +  private key (logged as hex string) and the irc command to execute to set
> +  the public key on the server.

This is probably obvious, but this isn't an appropriate way for a user to interact with this. Is Tor Messenger planning to build any UI?

@@ +59,4 @@
>  
>    commands: {
>      "AUTHENTICATE": function(aMessage) {
> +      let ecdsa = this.imAccount.wrappedJSObject.prefBranch.getCharPref("ecdsa");

I think this.getString("ecdsa") would do what you wanted here. You don't need to have the imAccount have a wrappedJSObject then.

@@ +59,5 @@
>  
>    commands: {
>      "AUTHENTICATE": function(aMessage) {
> +      let ecdsa = this.imAccount.wrappedJSObject.prefBranch.getCharPref("ecdsa");
> +      let hasECDSA = !!ecdsa.length;

You could use this.prefs.prefHasUserValue("ecdsa") here, which is saner.

@@ +61,5 @@
>      "AUTHENTICATE": function(aMessage) {
> +      let ecdsa = this.imAccount.wrappedJSObject.prefBranch.getCharPref("ecdsa");
> +      let hasECDSA = !!ecdsa.length;
> +
> +      if (aMessage.params[0] === "+") {

I'm confused at why you go into this part of the if, vs. the other. I still haven't seen any documentation about how this SASL exchange is supposed to work over IRC.

@@ +196,5 @@
>        else if (aMessage.cap.subcommand == "ACK") {
>          // The server acknowledges our choice to use SASL, send the first
>          // message.
> +        this.sendMessage("AUTHENTICATE",
> +          hasECDSA ? "ECDSA-NIST256P-CHALLENGE" : "PLAIN");

It looks like you'll want to remember what type of SASL mode you're in so that you don't have to keep looking up this information...and so we don't weirdly switch from one to the other in the middle of the exchange.

It looks like IRCv3.2 will tell us what SASL mechanisms are supported: http://ircv3.net/specs/extensions/sasl-3.2.html.

Comment 11

[arlolra]

> Why not add an IRC command to do this? Expecting users to read code comments is optimistic...

Also a good idea, but it needs to be done over the clearnet, so not something Tor Messenger users could make use of directly.  I'm not opposed though.

Comment 12

[arlolra]

Comment on attachment 8802390 [details] [diff] [review]
0001-SASL-ECDSA-NIST256P-CHALLENGE.patch from comment 5

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

::: chat/protocols/irc/elliptic.jsm
@@ +1,1 @@
> +/* This software is licensed under the MIT License.

Just the boilerplate to wrap it and the license.

::: chat/protocols/irc/ircSASL.jsm
@@ +21,5 @@
> +  ECDSA-NIST256P-CHALLENGE
> +
> +  Paste the following code snippit in the Firefox web console to generate a
> +  private key (logged as hex string) and the irc command to execute to set
> +  the public key on the server.

I needs to be done over the clearnet, so not a priority for TM, but I'm not opposed to doing something for the Instantbird users.

@@ +61,5 @@
>      "AUTHENTICATE": function(aMessage) {
> +      let ecdsa = this.imAccount.wrappedJSObject.prefBranch.getCharPref("ecdsa");
> +      let hasECDSA = !!ecdsa.length;
> +
> +      if (aMessage.params[0] === "+") {

I haven't been able to find any documentation.  I just reverse engineered it based on,
https://github.com/kaniini/ecdsatool#mechanism-spec

@@ +196,5 @@
>        else if (aMessage.cap.subcommand == "ACK") {
>          // The server acknowledges our choice to use SASL, send the first
>          // message.
> +        this.sendMessage("AUTHENTICATE",
> +          hasECDSA ? "ECDSA-NIST256P-CHALLENGE" : "PLAIN");

Yup, I wasn't sure what approach to take to make this stateful.  Any thoughts?

Comment 13

[Patrick Cloke [:clokep]]

(In reply to arlolra from comment #12)
> ::: chat/protocols/irc/ircSASL.jsm
> @@ +21,5 @@
> > +  ECDSA-NIST256P-CHALLENGE
> > +
> > +  Paste the following code snippit in the Firefox web console to generate a
> > +  private key (logged as hex string) and the irc command to execute to set
> > +  the public key on the server.
> 
> I needs to be done over the clearnet, so not a priority for TM, but I'm not
> opposed to doing something for the Instantbird users.

Sorry, but I have no idea what this means. What is "clearnet"?

> https://github.com/kaniini/ecdsatool#mechanism-spec

Thanks. I'll check through that. Is this the actual module used on Freenode?

> Yup, I wasn't sure what approach to take to make this stateful.  Any
> thoughts?

I know the XMPP code replaces some auth stuff with generators, which is pretty nifty (and a good way to track state...), see:
https://dxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp-authmechs.jsm

Comment 14

[arlolra]

> Sorry, but I have no idea what this means. What is "clearnet"?

My bad.  It means "without using Tor".

> Is this the actual module used on Freenode?

Maybe, but I'm not sure.

Comment 15

[Patrick Cloke [:clokep]]

I'm curious if freenode uses IRCv3 to provide this as a SASL mechanism now that we support IRCv3.2 CAPs in bug 1446689.