openssh unix dev - Nov 2017 - OpenSSH 7.6p1 ssh-agent exiting if passed an invalid key blob

Hello,

I noticed a problem recently when running some test code against the OpenSSH
7.6p1 ssh-agent. These tests ran fine against OpenSSH 7.5p1 and earlier, but
with OpenSSH 7.6p1, they were suddenly causing ssh-agent to exit. The request
being made was a ?sign? request, and the point of the test was to have the sign
operation fail. To trigger this, I was passing in an invalid key blob
(specifically, the string ?xxx?). In OpenSSH 7.5p1, this resulted in the
following debug output:

debug2: fd 3 setting O_NONBLOCK
debug3: fd 4 is O_NONBLOCK
debug1: type 13
process_sign_request2: cannot parse key blob: invalid format
debug1: XXX shrink: 3 < 4

As expected, a failure was returned to this request, and the agent continued to
run, waiting for a new request. However, after upgrading to OpenSSH 7.6p1, I saw
very different behavior. In this case, the following debugging output was seen:

debug2: fd 3 setting O_NONBLOCK
debug3: fd 4 is O_NONBLOCK
debug1: process_message: socket 1 (fd=4) type 13
process_sign_request2: buffer error: invalid format
debug1: cleanup_socket: cleanup

The ssh-agent actually exited in this case, as the ?buffer error? here was a
fatal() condition. The message sent to the agent was well-formed, though. It was
only the key blob that was not recognizable. Looking more closely at the code,
the 7.5p1 code looked like:

        if ((r = sshbuf_get_string(e->request, &blob, &blen)) != 0 ||
            (r = sshbuf_get_string(e->request, &data, &dlen)) != 0 ||
            (r = sshbuf_get_u32(e->request, &flags)) != 0)
                fatal("%s: buffer error: %s", __func__, ssh_err(r));
        if (flags & SSH_AGENT_OLD_SIGNATURE)
                compat = SSH_BUG_SIGBLOB;
        if ((r = sshkey_from_blob(blob, blen, &key)) != 0) {
                error("%s: cannot parse key blob: %s", __func__,
ssh_err(r));
                goto send;
        }

However, in 7.6p1, this changed to:

        if ((r = sshkey_froms(e->request, &key)) != 0 ||
            (r = sshbuf_get_string_direct(e->request, &data, &dlen))
!= 0 ||
            (r = sshbuf_get_u32(e->request, &flags)) != 0)
                fatal("%s: buffer error: %s", __func__, ssh_err(r));
        if (flags & SSH_AGENT_OLD_SIGNATURE)
                compat = SSH_BUG_SIGBLOB;

Note that the first call to sshbuf_get_string() is changed to sshkey_froms(),
which does the equivalent of both sshbuf_get_string() call and
sshkey_from_blob(). Since they are combined, though, both types of failure are
treated as a fatal ?buffer error?, rather than being treated as a ?cannot parse
key blob? error.

While an argument can be made that an SSH agent client should not be passing an
invalid key blob to the agent, one could imagine this happening if the client
was a never version of SSH than the agent and it tried to pass a key type that
the older agent didn?t understand. In general, I think the old behavior here
where this was an error but didn?t cause the agent to exit is the better
behavior. Is there any chance of getting this changed back?

I?ll probably need to change my test code either way so that it doesn?t break
when testing against OpenSSH 7.6p1, but I think this change might be worth
reverting (or reworking in some way to preserve the previous error vs. fatal
distinction).

Thanks for listening!
-- 
Ron Frederick
ronf at timeheart.net

On Mon, 13 Nov 2017, Ron Frederick wrote:
> Hello,
> 
> I noticed a problem recently when running some test code against
> the OpenSSH 7.6p1 ssh-agent. These tests ran fine against OpenSSH
> 7.5p1 and earlier, but with OpenSSH 7.6p1, they were suddenly causing
> ssh-agent to exit.
Sorry, I've committed this fix:


diff --git a/ssh-agent.c b/ssh-agent.c
index 9693722..0c88ab1 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -272,8 +272,11 @@ process_sign_request2(SocketEntry *e)
 		fatal("%s: sshbuf_new failed", __func__);
 	if ((r = sshkey_froms(e->request, &key)) != 0 ||
 	    (r = sshbuf_get_string_direct(e->request, &data, &dlen)) != 0
||
-	    (r = sshbuf_get_u32(e->request, &flags)) != 0)
-		fatal("%s: buffer error: %s", __func__, ssh_err(r));
+	    (r = sshbuf_get_u32(e->request, &flags)) != 0) {
+		error("%s: couldn't parse request: %s", __func__, ssh_err(r));
+		goto send;
+	}
+
 	if (flags & SSH_AGENT_OLD_SIGNATURE)
 		compat = SSH_BUG_SIGBLOB;
 	if ((id = lookup_identity(key)) == NULL) {

On Nov 14, 2017, at 4:11 PM, Damien Miller <djm at mindrot.org>
wrote:
> On Mon, 13 Nov 2017, Ron Frederick wrote:
>> I noticed a problem recently when running some test code against
>> the OpenSSH 7.6p1 ssh-agent. These tests ran fine against OpenSSH
>> 7.5p1 and earlier, but with OpenSSH 7.6p1, they were suddenly causing
>> ssh-agent to exit.
> 
> Sorry, I've committed this fix:
> 
> 
> diff --git a/ssh-agent.c b/ssh-agent.c
> index 9693722..0c88ab1 100644
> --- a/ssh-agent.c
> +++ b/ssh-agent.c
> @@ -272,8 +272,11 @@ process_sign_request2(SocketEntry *e)
> 		fatal("%s: sshbuf_new failed", __func__);
> 	if ((r = sshkey_froms(e->request, &key)) != 0 ||
> 	    (r = sshbuf_get_string_direct(e->request, &data, &dlen)) !=
0 ||
> -	    (r = sshbuf_get_u32(e->request, &flags)) != 0)
> -		fatal("%s: buffer error: %s", __func__, ssh_err(r));
> +	    (r = sshbuf_get_u32(e->request, &flags)) != 0) {
> +		error("%s: couldn't parse request: %s", __func__,
ssh_err(r));
> +		goto send;
> +	}
> +
> 	if (flags & SSH_AGENT_OLD_SIGNATURE)
> 		compat = SSH_BUG_SIGBLOB;
> 	if ((id = lookup_identity(key)) == NULL) {

Thanks Damien, but I?m not sure this is a good fix. Now both cases turn into an
error(), but if there is a problem reading the initial pair of strings and u32
value, you really can?t safely keep the connection open to receive additional
requests. An error in reading any of those three values probably should turn
into a call to fatal() with a buffer error as it did in 7.5, as you have no idea
where the next request message on the connection will start. It?s only the case
where you try to parse the data inside these values (specifically the key blob
in this case) that it would be safe to call error() and still read another
request.
-- 
Ron Frederick
ronf at timeheart.net