Re: [patch] isdn: fix information leak

[Changli Gao <xiaosuo@gmail.com>]

On Thu, Aug 5, 2010 at 9:55 PM, Dan Carpenter <error27@gmail.com> wrote:
> First of all, I'm happy that you're reviewing these patches.  A lot of
> learner, often untested, patches go through kernel-janitors and the
> entire purpose of the list is to check each other's work.
>
> Here is what I meant by an information leak.  The original code did:
>        spid = kmalloc(SCIOC_SPIDSIZE, GFP_KERNEL);
>        ...
>        strcpy(spid, rcvmsg->msg_data.byte_array);
>        ...
>        if (copy_to_user(data->dataptr, spid, SCIOC_SPIDSIZE)) {
>
> When you allocate memory with kmalloc() it doesn't clear that memory.  It
> could have anything in there including passwords etc.  If there is a
> short string in "rcvmsg->msg_data.byte_array" like "123" then the
> strcpy() puts that in the first 4 bytes of "spid", but the rest of the
> buffer is still full of passwords.
>
> The copy_to_user() sends it to the user, and normally the user only reads
> as far as the terminator, but if they read past that then they would see
> all the passwords etc that were saved there.
>
> This function is in the ioctl() probably only root has open() permission
> on the device so it's not a big deal because root can already find your
> password.  But I didn't dig that far.  It's just simpler to zero out the
> memory instead of worrying about if it really is exploitable or not.
>
> Also it's quite possible that the strcpy can't overflow.  But it's weird
> for the code to leave space for the terminator and then not use it.  In
> the end, I decided to do the cautious thing and be done with it.
>

Thanks, Dan. I got it.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)