Make secure_compare fail on undef or empty string

[CandyAngel]

Summary

Make secure_compare fail if either of its arguments are not defined or an empty string

Motivation

secure_compare is most likely to be used in a security context but (undef, undef), (undef, '') and ('', '') all return true. This means that an error in the configuration file (e.g. typo in hash key) could lead to unexpected success as the undefined value from the error would be regarded as equal to the lack of credentials from the client (e.g. Basic authentication in an under route).

I know this goes against what Perl regards as equal (especially two empty strings). May still be worth doing from a security perspective though.. can't think of any reason you would intentionally use an empty string in this context.

Possibly important but this implementation fails faster for ('', '1') than ('1', ''), which violates the "constant compare" time I think?

[firnsy]

To avoid failing faster for ('', '1') than ('1', '') due to the shortcut conditional on $one only, you could multiply both lengths ensuring an equal operation on both $one and $two.

Suggested change

	return undef if !length $one or length $one != length $two;
	return undef if !(length $one * length $two) or length $one != length $two;

[pink-mist]

I'd rather see something like this

Suggested change

	return undef if !length $one or length $one != length $two;
	return undef if !length $one != length $two;
	return undef unless length $one;

[karenetheridge]

The new line 268 is confusing to read without more parentheses. I'm not sure what part of the expression the ! in front of length is intending to surround.

[kraih]

Ok, consensus seems to be that the patch in its current form cannot be applied. So i'm closing this pull request for now.