On Wed, 2012-06-27 at 22:00 -0400, Anthony Ferrara wrote:
> Johannes,
>
> > Some comments on the "error behavior" part:
> >
> > E_WARNING - When CRYPT is not included in core (was disabled
> > compile-time, or is listed in disabled_functions declaration)
> >
> > Disabling a different function should have no effect. This is not
> > intuitive. If crypt is a dependency and is not available this function
> > shouldn't be available either.
>
> Hrm. Well, then I guess I could re-implement against crypt internally.
> That would take either a slight re-implementation of the crypt()
> internals, or slight refactoring of the PHP_FUNCTION(crypt) function
> to enable c calls to it (even if it's disabled).
I haven't looked at your patch. But if it has to call another
PHP_FuNCTION then it's not good. crypt implementation should be
accessible via C.
> I don't like the concept of core functions disappearing if they are
> not implemented. I think that does nothing but make it harder on the
> developers (now having to inject a function_exists(), etc).
I think it's rather the opposite. In that case the user has no way to
check whether the function is there without calling it and reacting to
errors. If the function "disappears" there is a possibility to check.
> Additionally, since this is a security issue, I think that always
> defining the function is the better approach. Otherwise, you can wind
> up in a situation where someone else comes in and implements function
> password_verify($password, $hash) { return true; }, which would be all
> sorts of bad... I can see the static linking to the function (instead
> of the dynamic call that's there), So in this case, I personally think
> the warning is appropriate.
Who should be the bad person in that scenario? The developer himself? I
think even the security goes the other way round - deelopers don't do
error checking, so they store an empty password in case of error. If the
function does not exist and is being called the script temrinates and
nothing further ad happens.
> > E_WARNING - When supplied an incorrect number of arguments.
> > E_WARNING - When supplied a non-string first parameter (password)
> >
> > This should follow common semantics of zend_parse_parameters(... "s").
> > i.e. it has to support objects with __toString(). Also other scalars are
> > fine. (if they can be casted to string)
> >
> > E_WARNING - If a non-string salt option is provided
> >
> > As above.
>
> Yeah, I guess that's fair. Is there a macro or function like
> Z_REPRESENTABLE_AS_STRING_P() or something? To make it easier to
> check?
No, but a simple zend_parse_parameters with "s" modifier should be
enough?
> > If any error is raise, false is returned by the function.
> >
> > In http://de.php.net/functions.internal it is documented that internal
> > functions return NULL on error during parameter parsing. New exceptions
> > for that should have a good reason.
>
> The good reason is consistency. Otherwise there would be three return
> values, `null`, `false` and `true` for password_verify(). Therefore, a
> check of `false === password_verify($foo)` would actually fail
> inadvertantly.
>
> My opinion is that it should do what's appropriate. The other 2 I can
> live with returning null (although I disagree with it significantly),
> but password_verify I think really should only ever return a
> boolean...
I don't see what makes password_verify special. If one wants a typesafe
check one can go for true === password_verify.
johannes
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
