On Sun, Aug 19, 2018 at 09:32:57AM +0100, Chris Lamb wrote:
> Hi,
>
> > openssh: CVE-2018-15473: delay bailout for invalid authenticating
> > user until after the packet
>
> I've started on a patch for wheezy (WIP attached).
>
> Would the security team be interested in one for stretch? If so, I can
> return with a proposed debdiff.
Your WIP mostly looks good, except:
> --- openssh-6.7p1.orig/auth2-pubkey.c
> +++ openssh-6.7p1/auth2-pubkey.c
> @@ -76,15 +76,11 @@ userauth_pubkey(Authctxt *authctxt)
> Buffer b;
> Key *key = NULL;
> char *pkalg, *userstyle;
> - u_char *pkblob, *sig;
> + u_char *pkblob, *sig = NULL;
> u_int alen, blen, slen;
> int have_sig, pktype;
> int authenticated = 0;
>
> - if (!authctxt->valid) {
> - debug2("userauth_pubkey: disabled because of invalid user");
> - return 0;
> - }
> have_sig = packet_get_char();
> if (datafellows & SSH_BUG_PKAUTH) {
> debug2("userauth_pubkey: SSH_BUG_PKAUTH");
> @@ -131,6 +127,11 @@ userauth_pubkey(Authctxt *authctxt)
> } else {
> buffer_put_string(&b, session_id2, session_id2_len);
> }
> + if (!authctxt->valid || authctxt->user == NULL) {
> + debug2("%s: disabled because of invalid user",
> + __func__);
> + goto done;
> + }
> /* reconstruct packet */
> buffer_put_char(&b, SSH2_MSG_USERAUTH_REQUEST);
> xasprintf(&userstyle, "%s%s%s", authctxt->user,
> @@ -162,11 +163,14 @@ userauth_pubkey(Authctxt *authctxt)
> buffer_len(&b))) == 1)
> authenticated = 1;
> buffer_free(&b);
> - free(sig);
> } else {
> debug("test whether pkalg/pkblob are acceptable");
> packet_check_eom();
> -
> + if (!authctxt->valid || authctxt->user == NULL) {
> + debug2("%s: disabled because of invalid user",
> + __func__);
> + goto done;
> + }
> /* XXX fake reply and always send PK_OK ? */
> /*
> * XXX this allows testing whether a user is allowed
> @@ -192,6 +196,7 @@ done:
> key_free(key);
> free(pkalg);
> free(pkblob);
> + free(sig);
> return authenticated;
> }
>
This bit has the same memory leak problem that I noticed in Sébastien's
backport for stretch: https://bugzilla.mindrot.org/show_bug.cgi?id=2898,
but the backported fix needs to be different because we're working with
a buffer on the stack rather than a pointer to the buffer on the heap.
My suggestion to Sébastien was to add "buffer_free(&b);" before the new
"goto done;" in (only) the have_sig branch; this is a bit clunky, but
seems like the easiest solution.
--
Colin Watson [[email protected]]
