OpenSSH 7.6p1 ssh-agent exiting if passed an invalid key blob

[Ron Frederick]

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