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

[Ron Frederick]

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