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

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

On Tue, 14 Nov 2017, Ron Frederick wrote:
> 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.
That's not the case: this function is called in the context of one
message with delimited length (see process_message()). A failure here
just disregards that message and doesn't need to kill the entire
connection.

There are some other input parsing cases that should be downgraded
from fatal() in ssh-agent.c, but I'll do those separately.

-d

On Nov 14, 2017, at 5:43 PM, Damien Miller <djm at mindrot.org>
wrote:
> On Tue, 14 Nov 2017, Ron Frederick wrote:
> 
>> 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.
> 
> That's not the case: this function is called in the context of one
> message with delimited length (see process_message()). A failure here
> just disregards that message and doesn't need to kill the entire
> connection.
> 
> There are some other input parsing cases that should be downgraded
> from fatal() in ssh-agent.c, but I'll do those separately.

Ah, ok - my mistake. I didn?t remember there was an overall length field on each
message, but going back and looking at my client code, I see you?re right. In
that case, I agree that it shouldn?t be a problem to allow parsing failures at
this level, or in similar places for other messages.

Thanks very much!
-- 
Ron Frederick
ronf at timeheart.net