paragonie / paseto

Platform-Agnostic Security Tokens
https://paseto.io
Other
3.23k stars 108 forks source link

Spec question on UInt64 requirement and language results for sizes #48

Closed aidantwoods closed 6 years ago

aidantwoods commented 6 years ago

Regarding the spec definition of LE64:

LE64() encodes a 64-bit unsigned integer into a little-endian binary string.

When this is used with PAE, LE64 is exclusively given lengths (of an array and of a byte string)

function PAE(pieces) {
    if (!Array.isArray(pieces)) {
        throw TypeError('Expected an array.');
    }
    var count = pieces.length;
    var output = LE64(count);
    for (var i = 0; i < count; i++) {
        output += LE64(pieces[i].length);
        output += pieces[i];
    }
    return output;
}

However, some languages (e.g. PHP, Swift, ...) give signed integer results for these lengths: reducing the effective number of bytes that LE64 will ever act on to 63 (even after conversion to a UInt64 if possible).

Given the goal of platform agnosticism I think the spec should define what to do in the case that the lengths used by PAE cannot be more than 63 (effective) bytes in the implemented language. (Indeed the reference PHP implementation has this restriction). Perhaps even given that languages with this restriction exist, implementations in languages that do give unsigned 64 bit lengths should instead replicate the behaviour, e.g. be instructed to throw if the most significant bit is ever populated (i.e. the result is greater than something like UInt64.max/2).

(actually hitting these numbers isn't at all likely to happen in practice of course by any sensible interpretation of the word, this mainly being a pedantic clarification on what the "right" (defined) thing to do is)

paragonie-scott commented 6 years ago

My solution would be: Always would set the MSB to 0, for interoperability with languages that don't have unsigned integer support.

be instructed to throw if the most significant bit is ever populated (i.e. the result is greater than something like UInt64.max/2).

+1

aidantwoods commented 6 years ago

Seems reasonable (that's how I've got it implemented at the moment even). This just a request to clarify the spec a little for this case :)

paragonie-scott commented 6 years ago

https://github.com/paragonie/paseto/commit/e4b75486f705e427028c1ae17e4ea6bd4d573d64 should be adequate for disambiguating the documentation. Good call on reporting this 👍

aidantwoods commented 6 years ago

LGTM!

PHP only has signed ints, so isn't & PHP_INT_MAX the identity map? ;p https://github.com/paragonie/paseto/blob/83fe4ede59a9f358121732954efd47cd7286003f/src/Util.php#L120

paragonie-scott commented 6 years ago

Yep. I included it since this is the reference implementation, and it'll decrease the odds that somebody accidentally overlooks it and somehow signs a 2^63+1 length message one day in another language.

aidantwoods commented 6 years ago

Aha, yeah okay that makes sense.

In that case maybe even something like $signedInt64Max = PHP_INT_MAX would make it abundantly clear (since unsigned ints are mentioned in the doc block), but at some point people should read the spec, or at least look up what PHP_INT_MAX refers to so 🤷‍♂️.

aidantwoods commented 6 years ago

https://github.com/aidantwoods/swift-paseto/commit/8ad4740dfc799a42b6b85abe1db742f3799469ee addresses this at my end.