paragonie / paseto

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

Security vulnerability in documentation #106

Closed TheWolfH closed 2 years ago

TheWolfH commented 4 years ago

It may be a bit strange to call an error in the documentation a security vulnerability, but I think it is justified here.

The documentation claims that

$parser = Parser::getLocal($sharedKey, ProtocolCollection::v2());

is equivalent to

$parser = (new Parser())
    ->setKey($sharedKey)
    // Adding rules to be checked against the token
    ->addRule(new NotExpired)
    ->addRule(new IssuedBy('issuer defined during creation'))
    ->setPurpose(Purpose::local())
    // Only allow version 2
    ->setAllowedVersions(ProtocolCollection::v2());

From my understand, however, this is not the case. In particular, the former call does not add a rule banning expired tokens from successful validation. I'm afraid many users of the library, relying on the documentation, will use the former call without explicitly adding such a rule and therefore opening their applications up to misuse of expired tokens.

Please fix this documentation error (or let me know in case I misunderstood something fundamentally here).

paragonie-scott commented 4 years ago

Not every PASETO is going to even have an exp header. It's an optional claim (as it is in JWT).

TheWolfH commented 4 years ago

That fact is undisputed. I exclusively disputed that the two code snippets mentioned are equivalent, while the line between the two snippets reads // This is the same as:.

paragonie-scott commented 4 years ago

I don't understand.

First you say...

. I'm afraid many users of the library, relying on the documentation, will use the former call without explicitly adding such a rule and therefore opening their applications up to misuse of expired tokens.

Then I say "those are always optional headers" and then you say "That fact is undisputed"

If it's always optional and everyone expects it to be optional how is its absence a vulnerability?

TheWolfH commented 4 years ago

The issue is not that that the first line I quoted does not add a check for expired tokens. The issue is that the documentation claims that line is equivalent to the second block of code I quoted, which does add such a check.

paragonie-security commented 2 years ago

This will be resolved in #128 to prevent confusion or false senses of security.