Skip to content

Commit

Permalink
remove excessive checks and fix warnings
Browse files Browse the repository at this point in the history
  • Loading branch information
@weltling
weltling committed Jul 11, 2017
1 parent cbbf579 commit c620dae
Showing 1 changed file with 7 additions and 9 deletions.
16 changes: 7 additions & 9 deletions ext/sodium/libsodium.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1636,8 +1636,7 @@ PHP_FUNCTION(sodium_crypto_pwhash_scryptsalsa208sha256)
&passwd, &passwd_len,
&salt, &salt_len,
&opslimit, &memlimit) == FAILURE ||
hash_len <= 0 || hash_len >= SIZE_MAX ||
opslimit <= 0 || memlimit <= 0 || memlimit > SIZE_MAX) {
hash_len <= 0 || opslimit <= 0 || memlimit <= 0) {
zend_throw_exception(sodium_exception_ce, "crypto_pwhash_scryptsalsa208sha256(): invalid parameters", 0);
return;
}
Expand All @@ -1650,11 +1649,11 @@ PHP_FUNCTION(sodium_crypto_pwhash_scryptsalsa208sha256)
0);
return;
}
if (opslimit < crypto_pwhash_scryptsalsa208sha256_opslimit_interactive()) {
if ((size_t)opslimit < crypto_pwhash_scryptsalsa208sha256_opslimit_interactive()) {
zend_error(E_WARNING,
"number of operations for the scrypt function is low");
}
if (memlimit < crypto_pwhash_scryptsalsa208sha256_memlimit_interactive()) {
if ((size_t)memlimit < crypto_pwhash_scryptsalsa208sha256_memlimit_interactive()) {
zend_error(E_WARNING,
"maximum memory for the scrypt function is low");
}
Expand Down Expand Up @@ -1683,7 +1682,7 @@ PHP_FUNCTION(sodium_crypto_pwhash_scryptsalsa208sha256_str)
if (zend_parse_parameters(ZEND_NUM_ARGS(), "sll",
&passwd, &passwd_len,
&opslimit, &memlimit) == FAILURE ||
opslimit <= 0 || memlimit <= 0 || memlimit > SIZE_MAX) {
opslimit <= 0 || memlimit <= 0) {
zend_throw_exception(sodium_exception_ce,
"crypto_pwhash_scryptsalsa208sha256_str(): invalid parameters",
0);
Expand All @@ -1692,11 +1691,11 @@ PHP_FUNCTION(sodium_crypto_pwhash_scryptsalsa208sha256_str)
if (passwd_len <= 0) {
zend_error(E_WARNING, "empty password");
}
if (opslimit < crypto_pwhash_scryptsalsa208sha256_opslimit_interactive()) {
if ((size_t)opslimit < crypto_pwhash_scryptsalsa208sha256_opslimit_interactive()) {
zend_error(E_WARNING,
"number of operations for the scrypt function is low");
}
if (memlimit < crypto_pwhash_scryptsalsa208sha256_memlimit_interactive()) {
if ((size_t)memlimit < crypto_pwhash_scryptsalsa208sha256_memlimit_interactive()) {
zend_error(E_WARNING,
"maximum memory for the scrypt function is low");
}
Expand Down Expand Up @@ -1760,8 +1759,7 @@ PHP_FUNCTION(sodium_crypto_pwhash)
&passwd, &passwd_len,
&salt, &salt_len,
&opslimit, &memlimit) == FAILURE ||
hash_len <= 0 || hash_len >= SIZE_MAX ||
opslimit <= 0 || memlimit <= 0 || memlimit > SIZE_MAX) {
hash_len <= 0 || opslimit <= 0 || memlimit <= 0) {
zend_throw_exception(sodium_exception_ce, "crypto_pwhash(): invalid parameters", 0);
return;
}
Expand Down

6 comments on commit c620dae

@jedisct1
Copy link
Contributor

@jedisct1 jedisct1 commented on c620dae Jul 11, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SIZE_MAX is only defined to be at least 0xffff.

As far as I know, nothing prevents an implementation from defining SIZE_MAX smaller than INT_MAX. Off the top of my head, I don't know of such an implementation, but having 64-bit integers and 32-bit address offsets isn't far fetched.

These checks were intentional. So was the absence of type casting. By removing these, you're not changing anything to the compiler output on your platform; you're just introducing a potential vulnerability on other platforms.

@nikic
Copy link
Member

@nikic nikic commented on c620dae Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jedisct1 sizeof(size_t) is indeed larger than sizeof(zend_long) on the x32 ABI (though I must say that we don't have official support for it and there are certainly things not working properly there).

@weltling
Copy link
Contributor Author

@weltling weltling commented on c620dae Jul 11, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that we speak about it. Leaving aside the x32 ABI, as ofc things will go terribly wrong there anyway. Besides these places, there are others like

size_t         passwd_len ......
if (passwd_len <= 0) { ....

or

zend_long      hash_len .... ;
if hash_len <= .....

and then somewhere (unsigned long long) hash_len

In first case, actually don't care much as that's anyway just one processor instruction, be it <= or !, etc. But it looks clearly like a leftover from PHP 5 where sizes were int.

In second case - it's an overkill on a 32-bit platform, it were enough to cast to size_t.

Regarding things in this patch - in absense of exact typecast like zend_long vs. size_t, the warning about signed/unsigned mismatch is a false positive, because the corresponding var is checked to be in the correct range before. Even with the check agains't SIZE_MAX, it's ensured the cast is in the safe range between zero and SIZE_MAX. So this way there's a bit less warning storm. And i must say, last time i'm looking for the exact opposite to remove casts where possible, when I've a free hour. Casts are sometimes good, sometimes tehy're aren't :)

Another word on this topic - there's that file Zend/zend_range_check.h which indeed can be extended to support x32, why not. On the platforms we support today, either sizeof(zend_long) == sizeof(size_t) or sizeof(zend_long) < sizeof(size_t) and signed < unsigned. So not checking against zend_long > size_t means in PHP in most case one saved instruction.

@jedisct1 so what would you suggest now, revert this completely? Unfortunately i've no x32 machines on hand to test the changes, otherwise we could also extend the range check header for these cases, so every platform would be happy.

Thanks.

@jedisct1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can be sure that many people are running PHP on environments that are not officially supported. And also that we are bound to forget things like this when we officially add support for these.
If a compiler is able to spit out a warning on a condition that will always be true, it also means that it is not going to emit any code for it.

I'd suggest reverting these changes.

@jedisct1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

size_t         passwd_len ......
if (passwd_len <= 0) { ....

Nothing wrong with that. Checking for an inequality takes the same time as an equality, but the compiler will figure out that passwd_len cannot be negative anyway.
Checking for <= 0 instead of == 0 on a value supposed to be unsigned may save you from vulnerabilities if the type doesn't happen to be the one you assume. Once again, this has no cost.

and then somewhere (unsigned long long) hash_len

Yes, the function requires an unsigned long long length, not a size_t. This is not common, hence the cast in the function call to make this explicit.

@weltling
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is fine to revert it, whereby I still suggest to keep the casts. The range is ensured before, and less noise is better. Users might use it anywhere, however the platforms we discuss are unlikely to not give PHP even a start. Anyway.

Regarding size_t <= 0 - yes, that's still one instruction. However "type doesn't happen to be the one you assume" - it's C, furthermore it's a scalar type :)

Casting to a 64-bit type on 32-bit platform is not the same as else where. It might be useful to check the generated assembler.

Gonna revert this for now.

Thanks.

Please sign in to comment.