Hello, On Thu, Apr 09, 2009 at 10:22:40PM +0200, m...@linux.it wrote: > > On Apr 09, Kees Cook <k...@debian.org> wrote: > > > Additionally, the code is buggy and not very random: > > > > srand(time(NULL) + getpid()); > > > > This needs to at least use /dev/urandom, or sec+usec as done in shadow. > Initializing the rand(3) seed with the time and maybe the PID is a > common tecnique. > The result is only used to generate the salt, and I see no reason why > it needs to be cryptographically strong random. Do you?
I was asked to use srandom/random in shadow, but on glibc, that's not a big issue to use srand/rand. In shadow, I had to use gettimeofday(&tv, NULL); srandom (tv.tv_sec + tv.tv_usec) because many passwords had to be generated per second and by the same process. I don't think this would be an issue for mkpasswd (the PID will change if multiple passwords are generated at the same time(NULL)) Also, the security is not coming from hiding the salt, so having a strong cryptographically random does not seems required to me (it was just simpler to me to use srandom in shadow than checking if my assumption was true) I don't think the PID increases the number of possible salts (i.e. if I want to create rainbow tables for the passwords created a given day, I need 86400 tables, and the PID will only add 1000). But the PID is vary important to make sure that two identical passwords generated at the same time will have different hashes. If you use tv.tv_sec + tv.tv_usec, I don't think you would need to keep the PID, though. > > There is also a bug that it does not accept salt smaller than 16 bytes for > > sha-256 and sha-512. This does not conform to > This was a design choice to keep the initial code simpler and I had no > time so far to improve it, feel free to send a patch. If time permit, I will do. At some time, I was willing to use mkpasswd for the shadow testsuite, and it failed when the provided salt was < 16 bytes. Generating always 16 bytes salt looks OK to me (having a variable size only increase the salt-space by *2). The patch should be simple (just being less strict with an input salt). > > I would recommend dropping mkpasswd (potentially in favor of a PAM-based > > tool as discussed in bug 505640). > I have read this bug and I do not understand which additional features > PAM support would provide, but I will consider adding it if you can > provide a good rationale. I did not meant using PAM in mkpasswd. My comment was just for chpasswd, whose goal is to change /etc/shadow and these changes should be consistent with the machine's password policy. I think mkpasswd should just use the method it is given in argument. Best Regards, -- Nekral -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
