Re: getentropy does not explicit_bzero if copyout fails

2018-02-21 Thread Mateusz Guzik 
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

2018-02-21 Thread Ted Unangst 
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

2018-02-21 Thread Mateusz Guzik 
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