Page MenuHomePhabricator

Please don't use `uniqid`
Closed, ResolvedPublic

Description

Hello,
the private .ics url is generated by using the uniqid function, but the documentation says:

This function does not generate cryptographically secure values, and should not be used for cryptographic purposes. If you need a cryptographically secure value, consider using random_int(), random_bytes(), or openssl_random_pseudo_bytes() instead.

This function does not guarantee uniqueness of return value. Since most systems adjust system clock by NTP or like, system time is changed constantly. Therefore, it is possible that this function does not return unique ID for the process/thread. Use more_entropy to increase likelihood of uniqueness.

It would be great, in order to avoid generating guessable secret urls, to use something better, like random_compat, that is already provided by DokuWiki.

Event Timeline

andreas lowered the priority of this task from Normal to Wishlist.Jun 20 2017, 7:53 AM
andreas added a subscriber: andreas.

Thanks for your suggestion, I did not know that random_compat exists.
However, I still consider the implementation with uniqid as appropriate, because:

  • We are not doing any cryptography here
  • A private URL without any authentication is per design not very secure
  • The ICS URL generation is disabled by default
  • ICS URLs are read only
  • I consider the private ICS feed as a workaround for broken clients, hence the client should be fixed

If you provide a patch, I'd be happy to apply it, though.

Well, we're generating secret material, so I guess it's cryptography ;)

Anyway, the attached patch should work.