[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: getppid misused as entropy source



On Tue, Mar 08, 2005 at 10:46:52AM -0300, Fabio Olive Leite wrote:
> Recently I saw a commit from Chad Loder changing a getpid call in
> login_radius for an arc4random call, since the original (wrong)
> intention of the code was to use getpid to generate a random number.
> 
> I checked the simple change in the code, and noticed just below it
> there was a getppid call for the same purpose. Then I set out to
> imitate OpenBSD's code audit procedure and ran:
> 
> $ for i in $(grep -Rwl getppid /usr/src); do vim $i; done
> 
> I noticed bind also uses getppid as an entropy source, as well as
> login_radius, so I propose the patches below.

[ ... ]

> Index: libexec/login_radius/raddauth.c
> ===================================================================
> RCS file: /cvs/src/libexec/login_radius/raddauth.c,v
> retrieving revision 1.18
> diff -u -r1.18 raddauth.c
> --- libexec/login_radius/raddauth.c	2 Mar 2005 21:51:17 -0000	1.18
> +++ libexec/login_radius/raddauth.c	8 Mar 2005 12:17:51 -0000
> @@ -231,9 +231,7 @@
>  	sin.sin_port = svp->s_port;
>  
>  	req_id = (u_char) arc4random();
> -	auth_port = ttyslot();
> -	if (auth_port == 0)
> -		auth_port = (int)getppid();
> +	auth_port = (int) arc4random();
>  	if (strcmp(style, "radius") != 0) {
>  		int len = strlen(username) + strlen(style) + 2;

I'm afraid that this patch is wrong, the return value from getppid()
was constrained in the 2-65535 range and the call to arc4random()
isn't. And I think it's bad in that context. I'll look at it again tonight.

> Even though pids are random in OpenBSD, using getpid/getppid (and
> ttyslot!?) as entropy sources is a fairly bad example, so I hope those
> changes above will be useful.
> 
> I plan to grep -R for ttyslot now, but I doubt it will be misused
> much.
> 
> OK, I'm ready for all the flames!

-- 
If a child annoys you, quiet him by brushing his hair.  If this doesn't
work, use the other side of the brush on the other end of the child.