Re: getentropy does not explicit_bzero if copyout fails
On Wed, Feb 21, 2018 at 10:28 PM, Ted Unangst wrote: > Mateusz Guzik wrote: > > As the subject states. By the time the code gets to copyout, buf is > > already populated. Clearing it only if copyout succeeds looks like a > > braino, thus the following trivial proposal: > > If the secret random data is not copied out, it will not be used, and > there's > nothing to leak. I think this just complicates the code. > > It is a nit. However, leaving the buf like this gives me an uneasy feeling and the patch is most definitely not a complication. That said, your OS - your call. > > > diff --git a/sys/dev/rnd.c b/sys/dev/rnd.c > > index e33cb5fd7c0..fa876a950b9 100644 > > --- a/sys/dev/rnd.c > > +++ b/sys/dev/rnd.c > > @@ -932,9 +932,9 @@ sys_getentropy(struct proc *p, void *v, register_t > > *retval) > > if (SCARG(uap, nbyte) > sizeof(buf)) > > return (EIO); > > arc4random_buf(buf, SCARG(uap, nbyte)); > > - if ((error = copyout(buf, SCARG(uap, buf), SCARG(uap, nbyte))) > != 0) > > - return (error); > > + error = copyout(buf, SCARG(uap, buf), SCARG(uap, nbyte)); > > explicit_bzero(buf, sizeof(buf)); > > - retval[0] = 0; > > - return (0); > > + if (error == 0) > > + retval[0] = 0; > > + return (error); > > } > -- Mateusz Guzik
Re: getentropy does not explicit_bzero if copyout fails
Mateusz Guzik wrote: > As the subject states. By the time the code gets to copyout, buf is > already populated. Clearing it only if copyout succeeds looks like a > braino, thus the following trivial proposal: If the secret random data is not copied out, it will not be used, and there's nothing to leak. I think this just complicates the code. > > diff --git a/sys/dev/rnd.c b/sys/dev/rnd.c > index e33cb5fd7c0..fa876a950b9 100644 > --- a/sys/dev/rnd.c > +++ b/sys/dev/rnd.c > @@ -932,9 +932,9 @@ sys_getentropy(struct proc *p, void *v, register_t > *retval) > if (SCARG(uap, nbyte) > sizeof(buf)) > return (EIO); > arc4random_buf(buf, SCARG(uap, nbyte)); > - if ((error = copyout(buf, SCARG(uap, buf), SCARG(uap, nbyte))) != 0) > - return (error); > + error = copyout(buf, SCARG(uap, buf), SCARG(uap, nbyte)); > explicit_bzero(buf, sizeof(buf)); > - retval[0] = 0; > - return (0); > + if (error == 0) > + retval[0] = 0; > + return (error); > } > > > -- > Mateusz Guzik
getentropy does not explicit_bzero if copyout fails
As the subject states. By the time the code gets to copyout, buf is already populated. Clearing it only if copyout succeeds looks like a braino, thus the following trivial proposal: diff --git a/sys/dev/rnd.c b/sys/dev/rnd.c index e33cb5fd7c0..fa876a950b9 100644 --- a/sys/dev/rnd.c +++ b/sys/dev/rnd.c @@ -932,9 +932,9 @@ sys_getentropy(struct proc *p, void *v, register_t *retval) if (SCARG(uap, nbyte) > sizeof(buf)) return (EIO); arc4random_buf(buf, SCARG(uap, nbyte)); - if ((error = copyout(buf, SCARG(uap, buf), SCARG(uap, nbyte))) != 0) - return (error); + error = copyout(buf, SCARG(uap, buf), SCARG(uap, nbyte)); explicit_bzero(buf, sizeof(buf)); - retval[0] = 0; - return (0); + if (error == 0) + retval[0] = 0; + return (error); } -- Mateusz Guzik