[patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[Romano]

Cinap,

http://okturing.com/src/8326/body is a patch that I posted to #cat-v but was told to post to the mailing list for review by you. If you want more background of why this was helpful to me, I'd be willing to elaborate. I don't think it qualifies as a bug fix so I'm sending to the general discussion list. My first message did not go through, returned as FAILED without any discernible reason. I suspect it is due to it having a text/html part, so re-sending with the patch inlined and hopefully my email client is sending just plain text.

diff -r f9f922fd127d sys/src/cmd/ssh.c
--- a/sys/src/cmd/ssh.c	Mon Apr 20 00:08:35 2020 +0200
+++ b/sys/src/cmd/ssh.c	Mon Apr 20 17:07:11 2020 -0700
@@ -492,6 +492,14 @@
 	static char macalgs[] = "hmac-sha1";	/* work around for github.com */
 	static char langs[] = "";
 
+	if(debug){
+		fprint(2, "%s: %s\n", "client supported kex_algorithms", kexalgs);
+		fprint(2, "%s: %s\n", "client supported encryption algorithms", cipheralgs);
+		fprint(2, "%s: %s\n", "client supported compression algorithms", zipalgs);
+		fprint(2, "%s: %s\n", "client supported mac algorithms", macalgs);
+		fprint(2, "%s: %s\n", "client supported languages", langs);
+	}
+
 	uchar cookie[16], x[32], yc[32], z[32], k[32+1], h[SHA2_256dlen], *ys, *ks, *sig;
 	uchar k12[2*ChachaKeylen];
 	int i, nk, nys, nks, nsig;
@@ -530,21 +538,25 @@
 	}
 	ds = hashstr(recv.r, recv.w-recv.r, ds);
 
-	if(debug){
-		char *tab[] = {
-			"kexalgs", "hostalgs",
-			"cipher1", "cipher2",
-			"mac1", "mac2",
-			"zip1", "zip2",
-			"lang1", "lang2",
-			nil,
-		}, **t, *s;
-		uchar *p = recv.r+17;
-		int n;
-		for(t=tab; *t != nil; t++){
-			if(unpack(p, recv.w-p, "s.", &s, &n, &p) < 0)
-				break;
+	/* See RFC4253 Section 7.1 for descriptions. */
+	char *tab[] = {
+		"kexalgs", "srvhostalgs",
+		"clicipher", "srvcipher",
+		"climac1", "srvmac",
+		"clipzip", "srvzip",
+		"clilang", "srvlang",
+		nil,
+	}, **t, *s;
+	uchar *p = recv.r+17;
+	int n;
+	for(t=tab; *t != nil; t++){
+		if(unpack(p, recv.w-p, "s.", &s, &n, &p) < 0)
+			break;
+		if(debug)
 			fprint(2, "%s: %.*s\n", *t, utfnlen(s, n), s);
+		if(!strcmp(*t,"clicipher") && !strstr(s,cipheralgs)) {
+			fprint(2, "%s not found in %.*s\n", cipheralgs, utfnlen(s, n), s);
+			sysfatal("server does not support cipher");
 		}
 	}
 
@@ -1258,7 +1270,7 @@
 	fprint(fd, "%s\r\n", send.v);
 	recv.v = readline();
 	if(debug)
-		fprint(2, "server verison: %s\n", recv.v);
+		fprint(2, "server version: %s\n", recv.v);
 	if(strncmp("SSH-2.0-", recv.v, 8) != 0)
 		sysfatal("bad server version: %s", recv.v);
 	recv.v = strdup(recv.v);

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[Stanley Lieber]

On April 21, 2020 12:46:24 AM EDT, Romano <unobe@cpan.org> wrote:
>Cinap,
>
>http://okturing.com/src/8326/body is a patch that I posted to #cat-v but was told to post to the mailing list for review by you. If you want more background of why this was helpful to me, I'd be willing to elaborate. I don't think it qualifies as a bug fix so I'm sending to the general discussion list. My first message did not go through, returned as FAILED without any discernible reason. I suspect it is due to it having a text/html part, so re-sending with the patch inlined and hopefully my email client is sending just plain text.
>
>diff -r f9f922fd127d sys/src/cmd/ssh.c
>--- a/sys/src/cmd/ssh.c	Mon Apr 20 00:08:35 2020 +0200
>+++ b/sys/src/cmd/ssh.c	Mon Apr 20 17:07:11 2020 -0700
>@@ -492,6 +492,14 @@
> 	static char macalgs[] = "hmac-sha1";	/* work around for github.com */
> 	static char langs[] = "";
> 
>+	if(debug){
>+		fprint(2, "%s: %s\n", "client supported kex_algorithms", kexalgs);
>+		fprint(2, "%s: %s\n", "client supported encryption algorithms", cipheralgs);
>+		fprint(2, "%s: %s\n", "client supported compression algorithms", zipalgs);
>+		fprint(2, "%s: %s\n", "client supported mac algorithms", macalgs);
>+		fprint(2, "%s: %s\n", "client supported languages", langs);
>+	}
>+
> 	uchar cookie[16], x[32], yc[32], z[32], k[32+1], h[SHA2_256dlen], *ys, *ks, *sig;
> 	uchar k12[2*ChachaKeylen];
> 	int i, nk, nys, nks, nsig;
>@@ -530,21 +538,25 @@
> 	}
> 	ds = hashstr(recv.r, recv.w-recv.r, ds);
> 
>-	if(debug){
>-		char *tab[] = {
>-			"kexalgs", "hostalgs",
>-			"cipher1", "cipher2",
>-			"mac1", "mac2",
>-			"zip1", "zip2",
>-			"lang1", "lang2",
>-			nil,
>-		}, **t, *s;
>-		uchar *p = recv.r+17;
>-		int n;
>-		for(t=tab; *t != nil; t++){
>-			if(unpack(p, recv.w-p, "s.", &s, &n, &p) < 0)
>-				break;
>+	/* See RFC4253 Section 7.1 for descriptions. */
>+	char *tab[] = {
>+		"kexalgs", "srvhostalgs",
>+		"clicipher", "srvcipher",
>+		"climac1", "srvmac",
>+		"clipzip", "srvzip",
>+		"clilang", "srvlang",
>+		nil,
>+	}, **t, *s;
>+	uchar *p = recv.r+17;
>+	int n;
>+	for(t=tab; *t != nil; t++){
>+		if(unpack(p, recv.w-p, "s.", &s, &n, &p) < 0)
>+			break;
>+		if(debug)
> 			fprint(2, "%s: %.*s\n", *t, utfnlen(s, n), s);
>+		if(!strcmp(*t,"clicipher") && !strstr(s,cipheralgs)) {
>+			fprint(2, "%s not found in %.*s\n", cipheralgs, utfnlen(s, n), s);
>+			sysfatal("server does not support cipher");
> 		}
> 	}
> 
>@@ -1258,7 +1270,7 @@
> 	fprint(fd, "%s\r\n", send.v);
> 	recv.v = readline();
> 	if(debug)
>-		fprint(2, "server verison: %s\n", recv.v);
>+		fprint(2, "server version: %s\n", recv.v);
> 	if(strncmp("SSH-2.0-", recv.v, 8) != 0)
> 		sysfatal("bad server version: %s", recv.v);
> 	recv.v = strdup(recv.v);

yes, it was because of the html.

sl

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[cinap_lenrek]

> +		if(unpack(p, recv.w-p, "s.", &s, &n, &p) < 0)
> +			break;
> +		if(debug)
>  			fprint(2, "%s: %.*s\n", *t, utfnlen(s, n), s);
> +		if(!strcmp(*t,"clicipher") && !strstr(s,cipheralgs)) {
> +			fprint(2, "%s not found in %.*s\n", cipheralgs, utfnlen(s, n), s);
> +			sysfatal("server does not support cipher");
>  		}

hm... this is wrong as "s" is not NUL terminated here. also
not correct as you'd need to check if the overlap is properly
delimited.

also, this is not good enougth as to predict if the handshake
would fail you'd need to also check kex and zip algorithms.

overall, i'd prefer to implement the handshake proper and maybe
add support for some common older ciphersuits instead adding
code that does alot of clever things working around these
limitations.

--
cinap

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[Romano]

On April 21, 2020 6:46:13 PM UTC, cinap_lenrek@felloff.net wrote:
>> +		if(unpack(p, recv.w-p, "s.", &s, &n, &p) < 0)
>> +			break;
>> +		if(debug)
>>  			fprint(2, "%s: %.*s\n", *t, utfnlen(s, n), s);
>> +		if(!strcmp(*t,"clicipher") && !strstr(s,cipheralgs)) {
>> +			fprint(2, "%s not found in %.*s\n", cipheralgs, utfnlen(s, n),
>s);
>> +			sysfatal("server does not support cipher");
>>  		}
>
>hm... this is wrong as "s" is not NUL terminated here.

Cinap,

Thanks for the feedback! I am new to this and I appreciate the pointers.

By wrong, you mean that utfnlen() shouldn't be used with 's'? That usage already existed in the chunk I modified, so would that mean the implementation @ tip is also wrong in that way? Or  is there something else I'm missing?

> also not correct as you'd need to check if the overlap is properly
>delimited.

Even if there's only one supported cipher by the client at this point?

>also, this is not good enougth as to predict if the handshake
>would fail you'd need to also check kex and zip algorithms.

Agreed: I was scratching the itch I had, admittedly, abd thought less was more. I pondered doing more but as this is my first foray into this system and code base, I was trying to limit the effects of my changes. The generalized case would be nice to implement, but I fear there'd be so much of my crappy code for veterans like you to comment on, it would stall improvement. Do you think that adding the patch actually makes anything operate worse than it currently does on tip? Do you think it's a step backwards?

>overall, i'd prefer to implement the handshake proper and maybe
>add support for some common older ciphersuits instead adding
>code that does alot of clever things working around these
>limitations.

Yes, I'd prefer that too. I can review the RFC to see how far off-base the current implemenation is. Also, I am not a crypto expert and my beginning perusal of the 9front code didn't yield info for me on where to begin with adding new ciphers. So I guess this would be a good time to ask: do you (or does anyone else) have some pointers on where I would begin with adding other ciphers?

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[cinap_lenrek]

> By wrong, you mean that utfnlen() shouldn't be used with 's'?
> That usage already existed in the chunk I modified, so would t
> hat mean the implementation @ tip is also wrong in that way?
> Or  is there something else I'm missing?

no, utfnlen() is right because it doesnt rely on the \0 byte as
we explicitely pass the length.

what is wrong is the call to strstr() with s being a non-NUL
terminated string of length n.

--
cinap

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[Romano]

On April 21, 2020 11:00:06 PM UTC, cinap_lenrek@felloff.net wrote:
>> By wrong, you mean that utfnlen() shouldn't be used with 's'?
>> That usage already existed in the chunk I modified, so would t
>> hat mean the implementation @ tip is also wrong in that way?
>> Or  is there something else I'm missing?
>
>no, utfnlen() is right because it doesnt rely on the \0 byte as
>we explicitely pass the length.
>
>what is wrong is the call to strstr() with s being a non-NUL
>terminated string of length n.

Got it, thanks!

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[cinap_lenrek]

> Yes, I'd prefer that too. I can review the RFC to see how far
> off-base the current implemenation is. Also, I am not a crypto
> expert and my beginning perusal of the 9front code didn't yield
> info for me on where to begin with adding new ciphers.

that code was never written to support multiple ciphers (all
the other ciphers are slow and insecure/broken anyway). i just
had no time for bullshit like this as i was writing this over
a weekend, fast.

the initial version didnt even bother with password and
keyboard-interactive authentication, and only supported
RSA public key auth. only what i needed (for my dayjob
as admin).

the nice thing about chacha20/poly1305 is that is is a AEAD
cipher construction. that is, it does authentication (MAC)
and encrypion as a single operation. so that cuts down
complexity and pointless-combinations down.

we later learned that github.com doesnt understand this
and insist on at least ONE mac algorithm to be listed in
the negotiation, even tho it is never used as AEAD ciphers
imply the MAC. (thanks to ori's git client work).

> So I guess this would be a good time to ask: do you (or does anyone else)
> have some pointers on where I would begin with adding other ciphers?

the question is if you want to support all combinations or not.
i'd do some research what the most common cipher+mac combination
is supported on old ssh2 versions. maybe its not possible i
dont know. just be as lazy as possible.

and maybe just implement one encryption+mac algrithm. then
the code needs less indirection and you can just have to
adapt recvpkt() and sendpkt(). they do the encryption in
place (at the beginning of recvpkt() or at the end of
sendpkt()).

if the versions you want to support dont implement curve25519
diffie-hellman, then you might also need to implement FFDH.
but its easy and libsec has you covered. all the complexity is
mostly with the negotiation. thats why i avoided this.

anyway.

try it out. you dont need to me much of a cryptographer as
the design has already been done by the ssh people. the RFC's
is really all you need.

maybe consider if it really is required for our ssh client to
support old versions of sshd with flawed algorithms. the best
way might be to just wait it out until ther servers get hacked
or they update.

--
cinap

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[ori]

> maybe consider if it really is required for our ssh client to
> support old versions of sshd with flawed algorithms. the best
> way might be to just wait it out until ther servers get hacked
> or they update.

Yeah -- that was what prompted this patch; right now the connection
drops and the server reports that it got an unexpected packet. We
could at least say that we don't support the server's ciphers.

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[Romano]

On Tue, Apr 21, 2020, at 4:25 PM, cinap_lenrek@felloff.net wrote:
> maybe consider if it really is required for our ssh client to
> support old versions of sshd with flawed algorithms. the best
> way might be to just wait it out until ther servers get hacked
> or they update.

I agree that it would be better to not have servers hacked, and be up-to-date. I hope the sysadmin for the (work) server I need access to will be open to including the chacha20/poly1305 cipher.  I'll let the list know, hopefully sometime next week, what I hear.

> that code was never written to support multiple ciphers (all
> the other ciphers are slow and insecure/broken anyway). i just
> had no time for bullshit like this as i was writing this over
> a weekend, fast.

I've looked a bit online for more information about all the other ciphers being insecure/broken. I didn't see anything about AES256-CTR being insecure or broken, but I probably missed something.  I read that one has to be careful with the key used, and that AES-256 is the "gold standard" presumably because the U.S. government uses it (argument from authority--yay!). The server I was trying to connect to does support AES256-CTR, and I plan to follow-up next week w/the sysadmin of that server to understand why that's supported, but not chacha20/poly1305. Would you (or someone else reading this on the list) have a paper or something handy that I can read re: the insecurity and brokenness of that cipher specifically? That would help in my conversation with the sysadmin.

> the initial version didnt even bother with password and
> keyboard-interactive authentication, and only supported
> RSA public key auth. only what i needed (for my dayjob
> as admin).

Since I am now using this software, I really appreciate the time you took to update it to accept a password.  For one-off testing, that was helpful.  And I am grateful that you're taking the time to reply to me on this list.

> the nice thing about chacha20/poly1305 is that is is a AEAD
> cipher construction. that is, it does authentication (MAC)
> and encrypion as a single operation. so that cuts down
> complexity and pointless-combinations down.

Very cool! I did not know any of this.

> the question is if you want to support all combinations or not.
> i'd do some research what the most common cipher+mac combination
> is supported on old ssh2 versions. maybe its not possible i
> dont know. just be as lazy as possible.
> 
> and maybe just implement one encryption+mac algrithm. then
> the code needs less indirection and you can just have to
> adapt recvpkt() and sendpkt(). they do the encryption in
> place (at the beginning of recvpkt() or at the end of
> sendpkt()).
> 
> 
> if the versions you want to support dont implement curve25519
> diffie-hellman, then you might also need to implement FFDH.
> but its easy and libsec has you covered. all the complexity is
> mostly with the negotiation. thats why i avoided this.

Fortunately, the server in question does support curve25519 for key exchange.

> try it out. you dont need to me much of a cryptographer as
> the design has already been done by the ssh people. the RFC's
> is really all you need.

Hopefully this won't be needed, but I did look into what might be needed if I had to do it. I'd focus on AES-256-CTR. From what I see in 9front, there's aes(2) which provides OFB and is similar to CTR[1], but is not CTR.  So that implementation would have to be added to aes(2), no?  In terms of steps I'd take, I'd dig into how the OFB implementation works on 9front, then determine how to adjust in order to implement CTR.

For authentication, I'd focus on hmac-sha2-512, which is available from sechash(2) on 9front.

Thanks again for taking the time to respond.

[1] https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation#Counter_(CTR)

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[Romano]

As a follow-up: I started speaking with IT today. They're using the CIS CentOS Linux Benchmark v2.1.0, but the last check missed yhe faxt that an entire section, section 5.2.11, was removed. That section restricts ciphers but even the audit looks wrong considering what it describes. The latest version of the document, v2.2.0 does not restrict ciphers more than the defaults shipped with openssh, and so IT looks like they're fine with updating.

I'll still look into patching to provide better diagnostic info to yhe user when supported cipher/mac isn't available.


On April 22, 2020 5:40:29 AM UTC, Romano <unobe@cpan.org> wrote:
>On Tue, Apr 21, 2020, at 4:25 PM, cinap_lenrek@felloff.net wrote:
>> maybe consider if it really is required for our ssh client to
>> support old versions of sshd with flawed algorithms. the best
>> way might be to just wait it out until ther servers get hacked
>> or they update.
>
>I agree that it would be better to not have servers hacked, and be
>up-to-date. I hope the sysadmin for the (work) server I need access to
>will be open to including the chacha20/poly1305 cipher.  I'll let the
>list know, hopefully sometime next week, what I hear.
>
>> that code was never written to support multiple ciphers (all
>> the other ciphers are slow and insecure/broken anyway). i just
>> had no time for bullshit like this as i was writing this over
>> a weekend, fast.
>
>I've looked a bit online for more information about all the other
>ciphers being insecure/broken. I didn't see anything about AES256-CTR
>being insecure or broken, but I probably missed something.  I read that
>one has to be careful with the key used, and that AES-256 is the "gold
>standard" presumably because the U.S. government uses it (argument from
>authority--yay!). The server I was trying to connect to does support
>AES256-CTR, and I plan to follow-up next week w/the sysadmin of that
>server to understand why that's supported, but not chacha20/poly1305.
>Would you (or someone else reading this on the list) have a paper or
>something handy that I can read re: the insecurity and brokenness of
>that cipher specifically? That would help in my conversation with the
>sysadmin.
>
>> the initial version didnt even bother with password and
>> keyboard-interactive authentication, and only supported
>> RSA public key auth. only what i needed (for my dayjob
>> as admin).
>
>Since I am now using this software, I really appreciate the time you
>took to update it to accept a password.  For one-off testing, that was
>helpful.  And I am grateful that you're taking the time to reply to me
>on this list.
>
>> the nice thing about chacha20/poly1305 is that is is a AEAD
>> cipher construction. that is, it does authentication (MAC)
>> and encrypion as a single operation. so that cuts down
>> complexity and pointless-combinations down.
>
>Very cool! I did not know any of this.
>
>> the question is if you want to support all combinations or not.
>> i'd do some research what the most common cipher+mac combination
>> is supported on old ssh2 versions. maybe its not possible i
>> dont know. just be as lazy as possible.
>> 
>> and maybe just implement one encryption+mac algrithm. then
>> the code needs less indirection and you can just have to
>> adapt recvpkt() and sendpkt(). they do the encryption in
>> place (at the beginning of recvpkt() or at the end of
>> sendpkt()).
>> 
>> 
>> if the versions you want to support dont implement curve25519
>> diffie-hellman, then you might also need to implement FFDH.
>> but its easy and libsec has you covered. all the complexity is
>> mostly with the negotiation. thats why i avoided this.
>
>Fortunately, the server in question does support curve25519 for key
>exchange.
>
>> try it out. you dont need to me much of a cryptographer as
>> the design has already been done by the ssh people. the RFC's
>> is really all you need.
>
>Hopefully this won't be needed, but I did look into what might be
>needed if I had to do it. I'd focus on AES-256-CTR. From what I see in
>9front, there's aes(2) which provides OFB and is similar to CTR[1], but
>is not CTR.  So that implementation would have to be added to aes(2),
>no?  In terms of steps I'd take, I'd dig into how the OFB
>implementation works on 9front, then determine how to adjust in order
>to implement CTR.
>
>For authentication, I'd focus on hmac-sha2-512, which is available from
>sechash(2) on 9front.
>
>Thanks again for taking the time to respond.
>
>[1]
>https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation#Counter_(CTR)

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[ori]

> As a follow-up: I started speaking with IT today. They're using the CIS CentOS Linux Benchmark v2.1.0, but the last check missed yhe faxt that an entire section, section 5.2.11, was removed. That section restricts ciphers but even the audit looks wrong considering what it describes. The latest version of the document, v2.2.0 does not restrict ciphers more than the defaults shipped with openssh, and so IT looks like they're fine with updating.
> 
> I'll still look into patching to provide better diagnostic info to yhe user when supported cipher/mac isn't available.

Ok -- If you don't get to it first, I'll probably take a look at doing proper negotiation
and maybe adding some more ciphers a couple of weeks down the road. I'll let you know when
I start thinking about it.

Re: [9front] [patch] /sys/src/cmd/ssh.c notify user of unavailable cipher

[hiro]

i don't like the idea of adding more ciphers.

On 4/27/20, ori@eigenstate.org <ori@eigenstate.org> wrote:
>> As a follow-up: I started speaking with IT today. They're using the CIS
>> CentOS Linux Benchmark v2.1.0, but the last check missed yhe faxt that an
>> entire section, section 5.2.11, was removed. That section restricts
>> ciphers but even the audit looks wrong considering what it describes. The
>> latest version of the document, v2.2.0 does not restrict ciphers more than
>> the defaults shipped with openssh, and so IT looks like they're fine with
>> updating.
>>
>> I'll still look into patching to provide better diagnostic info to yhe
>> user when supported cipher/mac isn't available.
>
> Ok -- If you don't get to it first, I'll probably take a look at doing
> proper negotiation
> and maybe adding some more ciphers a couple of weeks down the road. I'll let
> you know when
> I start thinking about it.
>
>