SASL ECDSA-NIST256P-CHALLENGE

UNCONFIRMED
Assigned to

Status

Chat Core
IRC
UNCONFIRMED
2 years ago
a year ago

People

(Reporter: arlolra, Assigned: arlolra)

Tracking

trunk

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8797777 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Attachment #8797777 - Attachment is patch: true
Attachment #8797777 - Attachment mime type: text/x-patch → text/plain
Attachment #8797777 - Flags: feedback?(clokep)
Potentially, can you give some links to:
* Documentation
* What networks actually use this
Assignee: nobody → arlolra
(Assignee)

Comment 2

2 years ago
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'
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

2 years ago
Created attachment 8801951 [details] [diff] [review]
0001-SASL-ECDSA-NIST256P-CHALLENGE.patch from comment 4

Updated the doc comment so that you can test it.
Attachment #8797777 - Attachment is obsolete: true
Attachment #8797777 - Flags: feedback?(clokep)
Attachment #8801951 - Flags: feedback?(clokep)
(Assignee)

Comment 6

2 years ago
Created attachment 8802390 [details] [diff] [review]
0001-SASL-ECDSA-NIST256P-CHALLENGE.patch from comment 5

Cleanup cases.
Attachment #8801951 - Attachment is obsolete: true
Attachment #8801951 - Flags: feedback?(clokep)
Attachment #8802390 - Flags: review?(clokep)

Comment 7

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

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

Comment 9

2 years ago
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 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.
Attachment #8802390 - Flags: review?(clokep) → feedback+
(Assignee)

Comment 11

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

Comment 12

2 years ago
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?
(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
(Assignee)

Comment 14

2 years ago
> 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.
You need to log in before you can comment on or make changes to this bug.