Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve argument error messages in ext/sodium #5197

Closed

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Feb 22, 2020

There are some notes:

  • Error messages starting with ...() expects argument ... are only usable for positive sentences, but not for negative ones (e.g. message too long for a single key). Usually, conversion isn't much of a problem (usually, the message even becomes more clear this way), but it seems to me that error messages starting with Argument ... passed to would be more generally usable.
  • Currently an Error exceptions must be derived from Error notice is generated when zend_argument_error() is called, because Sodium uses the SodiumException that is of course incompatible with Errors. I'm not sure what is the best course of action, maybe adding a zend_argument_exception() that can throw Exceptions?
  • Another question is whether we'd like to display the provided values in the error messages? Sometimes it would be trivial to do, but sometimes it's not (e.g. in messages in connection with length)

@Girgias
Copy link
Member

Girgias commented Feb 22, 2020

Wouldn't it maybe be better to have a sort of argument error messages formatter function? But maybe adding a dedicated function which can throw exceptions like that is better, or maybe just convert them to ValueErrors directly?

@nikic
Copy link
Member

nikic commented Feb 23, 2020

* Currently an `Error exceptions must be derived from Error` notice is generated when `zend_argument_error()` is called, because Sodium uses the `SodiumException` that is of course incompatible with `Error`s. I'm not sure what is the best course of action, maybe adding a `zend_argument_exception()` that can throw `Exception`s?

I think you can just lift that restriction in zend_throw_error. There's no particular reason why exceptions can't be used there.

* Another question is whether we'd like to display the provided values in the error messages? Sometimes it would be trivial to do, but sometimes it's not (e.g. in messages in connection with length)

I'd say, as a matter of general policy: No, values should not be included in error messages. People leaving display_errors=1 in production is unfortunately not as rare as it should be, and displaying values could leak sensitive information. Part of the reason why arguments in exception backtraces can be disabled since PHP 7.4.

ext/sodium/libsodium.c Outdated Show resolved Hide resolved
@nikic
Copy link
Member

nikic commented Feb 23, 2020

Uh, is this extension not getting tested on azure/travis?

ext/sodium/libsodium.c Outdated Show resolved Hide resolved
@kocsismate kocsismate force-pushed the consistent-error-messages3-sodium branch from 1cf0dd9 to f5fd02a Compare February 23, 2020 20:52
@kocsismate
Copy link
Member Author

kocsismate commented Feb 23, 2020

I'd say, as a matter of general policy: No, values should not be included in error messages. People leaving display_errors=1 in production is unfortunately not as rare as it should be, and displaying values could leak sensitive information.

Oh, I see now, thanks for the info. I haven't thought about this dimension of the question.

Uh, is this extension not getting tested on azure/travis?

Yeah, I was also surprised.

@kocsismate kocsismate force-pushed the consistent-error-messages3-sodium branch from f5fd02a to 042fc09 Compare February 24, 2020 09:25
@@ -3156,23 +3037,23 @@ PHP_FUNCTION(sodium_crypto_kdf_derive_from_key)
RETURN_THROWS();
}
if (subkey_len < crypto_kdf_BYTES_MIN) {
zend_throw_exception(sodium_exception_ce, "subkey cannot be smaller than SODIUM_CRYPTO_KDF_BYTES_MIN", 0);
zend_argument_error(sodium_exception_ce, 1, "to be more than or equal to SODIUM_CRYPTO_KDF_BYTES_MIN");
Copy link
Member

Choose a reason for hiding this comment

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

more than -> greater than

Copy link
Member

Choose a reason for hiding this comment

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

Also, it's possible to formulate this as to be at least BYTES_MIN and to be at most BYTES_MAX, which would be more compact.

Copy link
Member Author

Choose a reason for hiding this comment

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

So do you think we should use at least and at most instead of greater than or equal to and less than or equal to in general? I'll update my PRs then

Copy link
Member

Choose a reason for hiding this comment

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

I think so. It's more concise and, at least to me, just as clear. I could see an argument being made that "greater than or equal" is more obvious though.

Copy link
Member

Choose a reason for hiding this comment

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

Weell, it does depend on the case. "must be at least 10 bytes long" is definitely better than "must be greater than or equal to 10 bytes long", but I'm not so sure about "must be at least 10" and "must be greater than or equal to 10". The case of "most be at least 0" specifically sounds outright weird (but can be "must be non-negative" or "must not be negative").

So overall: Not sure :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@Girgias Do you maybe have any other suggestion or preference? On the one hand, I like the greater than or equal to form because it's obvious and it's the most consistent with greater than. On the other hand, at least is surely a more concise way to say the same. Additionally, we have positive/negative/non-negative etc. which - at least for me - need a little bit more processing time. Furthermore, according to what I remember, they are not ubiquitous everywhere (see my comment more below. Or it is just a myth?).

That said, my preference is to use greater/less than or equal to for pure minimum/maximum values, and at least/at most for minimum/maximum length etc.. It's not a strong opinion though, so I welcome any opposition. :)

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer the explicit greater/less than or equal to and at least/at most as it is straight forward, positivity and negativity is where French and English doesn't really agree on, famous example is how ℕ is ambiguous as French mathematician will include 0 in it whereas British won't for the most part (as it's meant to represent positive numbers).
This is mainly due to how in French "greater" and "less" are inclusive.

It also seems Japanese has this ambiguity (they literally have a brand called +-0 https://www.plusminuszero.jp/)

if (blocksize > SIZE_MAX) {
zend_throw_exception(sodium_exception_ce, "block size is too large", 0);
if (blocksize <= 0 || blocksize > SIZE_MAX) {
zend_argument_error(sodium_exception_ce, 2, "to be greater than 0");
Copy link
Member

Choose a reason for hiding this comment

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

As 0 is a particular common case: This can also be to be positive. Or to be non-negative for the >= case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to get rid of positive/negative from messages because I feel that the exact number is easier to understand - but maybe it's just me. :)

Furthermore, as far as I remember the position of 0 is not clear everywhere in the world. I found a comment about it: https://math.stackexchange.com/questions/26705/is-zero-positive-or-negative. But I might be wrong here as well.

RETURN_THROWS();
}
if (blocksize > SIZE_MAX) {
zend_throw_exception(sodium_exception_ce, "block size is too large", 0);
zend_argument_error(sodium_exception_ce, 2, "to be less than or equal to %d", SIZE_MAX);
Copy link
Member

Choose a reason for hiding this comment

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

Per your comment above, this shouldn't include SIZE_MAX.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed this, but the new message is a bit weird too. For now, it's .. to be less than or equal to the maximum allowed value.

RETURN_THROWS();
}
if (msg_len > crypto_secretstream_xchacha20poly1305_MESSAGEBYTES_MAX ||
msg_len > SIZE_MAX - crypto_secretstream_xchacha20poly1305_ABYTES) {
zend_throw_exception(sodium_exception_ce, "message cannot be larger than SODIUM_CRYPTO_SECRETSTREAM_XCHACHA20POLY1305_MESSAGEBYTES_MAX bytes", 0);
zend_argument_error(sodium_exception_ce, 2, "to be shorter than or equal to SODIUM_CRYPTO_SECRETSTREAM_XCHACHA20POLY1305_MESSAGEBYTES_MAX bytes");
Copy link
Member

Choose a reason for hiding this comment

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

Especially for cases like this, I think that phrasing this in the to be at most xxx bytes long style is more readable. "shorter than or equal" is pretty odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, it was very odd indeed. :)

@kocsismate kocsismate force-pushed the consistent-error-messages3-sodium branch 4 times, most recently from 386fff8 to 02f70d9 Compare February 24, 2020 22:01
@kocsismate kocsismate force-pushed the consistent-error-messages3-sodium branch from 02f70d9 to 4c65a39 Compare February 26, 2020 14:18
RETURN_THROWS();
}
if (hash_len >= 0xffffffff) {
zend_argument_error(sodium_exception_ce, 1, "must be less than the maximum allowed value");
Copy link
Member

Choose a reason for hiding this comment

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

Now that we are no longer restricted to positive messages, we can also use something like XXX is too large or YYY is too long here. I think that might be better for cases like this where we have a non-explicit upper bound.

Copy link
Member Author

Choose a reason for hiding this comment

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

I absolutely agree! Actually, I've already changed a similar message to "XXX is too large" in my other PR, so I'll review these cases here as well shortly.

@@ -1792,6 +1736,7 @@ PHP_FUNCTION(sodium_crypto_pwhash_str_needs_rehash)

if (zend_parse_parameters(ZEND_NUM_ARGS(), "sll",
&hash_str, &hash_str_len, &opslimit, &memlimit) == FAILURE) {
// TODO: what's this error message here?
Copy link
Member

Choose a reason for hiding this comment

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

Should just be removed, I believe.

RETURN_THROWS();
}
sodium_separate_string(state_zv);
state = (unsigned char *) Z_STRVAL(*state_zv);
state_len = Z_STRLEN(*state_zv);
if (state_len != sizeof (crypto_secretstream_xchacha20poly1305_state)) {
zend_throw_exception(sodium_exception_ce, "incorrect state length", 0);
zend_argument_error(sodium_exception_ce, 1, "to have a correct length");
Copy link
Member

Choose a reason for hiding this comment

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

Probably this should use negative form now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think "... must have a correct length" should do it as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

But if you have a better suggestion, I'm definitely open for a change.

@kocsismate kocsismate force-pushed the consistent-error-messages3-sodium branch from 4d97999 to e64af7d Compare February 27, 2020 08:07
@@ -558,7 +558,7 @@ PHP_FUNCTION(sodium_add)
val = (unsigned char *) Z_STRVAL(*val_zv);
val_len = Z_STRLEN(*val_zv);
if (val_len != addv_len) {
zend_throw_exception(sodium_exception_ce, "values must have the same length", 0);
zend_argument_error(sodium_exception_ce, 1, "and argument #2 ($string_2) must have the same size");
Copy link
Member Author

@kocsismate kocsismate Feb 27, 2020

Choose a reason for hiding this comment

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

Other possibilities:

  • and argument #2 ($string_2) must have the same length
  • must be as long as argument #2 ($string_2)
  • and argument #2 ($string_2) must be the same long (is it even correct?)

Copy link
Member

Choose a reason for hiding this comment

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

must have the same length looks like the best option to me.

@@ -558,7 +558,7 @@ PHP_FUNCTION(sodium_add)
val = (unsigned char *) Z_STRVAL(*val_zv);
val_len = Z_STRLEN(*val_zv);
if (val_len != addv_len) {
zend_throw_exception(sodium_exception_ce, "values must have the same length", 0);
zend_argument_error(sodium_exception_ce, 1, "and argument #2 ($string_2) must have the same size");
Copy link
Member

Choose a reason for hiding this comment

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

must have the same length looks like the best option to me.

RETURN_THROWS();
}
if (hash_len >= 0xffffffff) {
zend_argument_error(sodium_exception_ce, 1, "is too long");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
zend_argument_error(sodium_exception_ce, 1, "is too long");
zend_argument_error(sodium_exception_ce, 1, "is too large");

Unlike the similar message below, this is for an integer argument.

RETURN_THROWS();
}
if (padded_len < blocksize) {
zend_throw_exception(sodium_exception_ce, "invalid padding", 0);
zend_argument_error(sodium_exception_ce, 1, "must be less than the blocksize");
Copy link
Member Author

@kocsismate kocsismate Feb 28, 2020

Choose a reason for hiding this comment

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

sorry, I've just realized this error message makes zero sense. :/ So what about something like Argument # 1 ($string) must have a length greater than or equal to argument # 2 ($length) instead? Unfortunately, I couldn't come up with a better one.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to force ourselves to always mention all arguments in this format. Something like Argument #1 ($string) must not be shorter than the block size maybe?

PS: It's annoying that the error messages in this ext aren't getting tested, so it's not visible whether the result is actually good. For example, in this case the parameter name should really be changed to $blockSize instead of $length. We haven't cared a lot about quality of parameter names in arginfo before, but now with it appearing in error messages, we may have to fix some stuff...

Copy link
Member Author

@kocsismate kocsismate Feb 28, 2020

Choose a reason for hiding this comment

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

Something like Argument #1 ($string) must not be shorter than the block size maybe?

Seems good! I'll change the argument name too.

It's annoying that the error messages in this ext aren't getting tested

:/

now with it appearing in error messages, we may have to fix some stuff...

I agree. To make things even worse, param names in the documentation also have bad quality... Plus, I am sure there are a lots of inconsistencies between param names at php.net and param names in stubs (even though php.net uses $length in this case). That's why I've already planned a param name "overhaul" + sync. Hopefully, I can make it if I can finish my planned RFCs in time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, just realized that it would be very cool to be able to generate function signatures in the documentation from the stubs.

@php-pulls php-pulls closed this in 118b04b Feb 28, 2020
@kocsismate kocsismate deleted the consistent-error-messages3-sodium branch February 28, 2020 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants