security: epoch time expiry passes as valid

[pascaldekloe]

package jwt

import "testing"

// Tests issue #277 — epoch time expiry passes as valid.
func TestExpireEpoch(t *testing.T) {
        // has "exp" set to  0 (1970-01-01T00:00:00)
        serial := "eyJhbGciOiJIUzI1NiJ9.eyJpYXQiOjB9.FEX968C-vQ0GUk3_O5VmDTc5qoIuV9WLmR_3cOtj1B0"

        token, err := Parse(serial, func(*Token) (interface{}, error) {
                return []byte("secret"), nil
        })
        if err != nil {
                t.Fatal(err)
        }

        if token.Valid {
                t.Error("didn't expire")
        }
}

[pascaldekloe]

😴

[dgrijalva]

Wow. Good catch. Do you think there's a possible attack here?

[pascaldekloe]

I wrote some code which sets "exp" to zero when the exact moment of cancellation can't be determined. Could be worse if you actually use user data timestamps.

[pascaldekloe]

Two weeks and counting… 🤨

[APTy]

Since 0 is a default int64 value in go, there may be a strong usability and backwards-compatibility reasons to keep (but document) this behavior.

Currently, ExpiresAt: 0 means that the user is opting out of using an expiration time. The field is not serialized as a claim at generation time, and thus the validator does not see the claim and attempt to verify the expiration time of the token.

If this were to change, then anyone not using ExpiresAt will suddenly have all of their tokens fail validation.

It's a catch-22: accepting ExpiresAt: 0 as valid means that your validation isn't spec-compliant; but if you make ExpiresAt a required field, you're not compliant either. :man_shrugging:

[pascaldekloe]

Every bug fix breaks compatibility. Not a catch 22, this is a security leak.

[dgrijalva]

If you’d like to submit a PR, I’d be happy to review it. I don’t believe this constitutes a risk under normal use. The behavior should absolutely be documented and we should endeavor to resolve the ambiguity in the next major release.

-dave

On Jul 19, 2018, at 9:28 AM, Pascal S. de Kloe notifications@github.com wrote:

Every bug fix breaks compatibility. Not a catch 22, this is a security leak.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[jonhester]

Consider this comment a vote to leave it like it currently is. I'd hate to have to use a pointer or a null type for this field. I ran into this issue in a test where I was purposefully using an expired token. I was momentarily confused, until I realized that the token didn't have an expiration. Outside of tests, I can't think of a single reason to actually use the epoch for token expiration.

Fixing my test was as simple as changing the expiration to one second after epoch. If you are concerned about this in production then just require the token has an expiration claim when you verify it.

[dgrijalva]

There is a flag to make exp required (which will fail on zero value) but I don't think it's plumbed all the way out to the API. I'm going to see if I can make this happen in the next version. This way, the developer can choose what behavior is appropriate for them.

-dave

On Thu, Jul 26, 2018 at 12:00 PM, Jon Hester notifications@github.com wrote:

Consider this comment a vote to leave it like it currently is. I'd hate to have to use a pointer or a null type for this field. I ran into this issue in a test where I was purposefully using an expired token. I was momentarily confused, until I realized that the token didn't have an expiration. Outside of tests, I can't think of a single reason to actually use the epoch for token expiration.

Fixing my test was as simple as changing the expiration to one second after epoch. If you are concerned about this in production then just require the token has an expiration claim when you verify it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/dgrijalva/jwt-go/issues/277#issuecomment-408201048, or mute the thread https://github.com/notifications/unsubscribe-auth/AABcoTIK8Y8IylXpCGojZ2KrnrcNxagFks5uKhHPgaJpZM4U-BtD .

[pascaldekloe]

The developer should not choose how he wants JWT to work. We have a specification for that.

In my case it was only thanks to a good tester that we discovered that one service uses this library and fails to enforce some expiry policy. You can not possibly oversee all implications this flaw has and it is therefore irresponsible to chose for your user-base based on convenience.

[jonhester]

@dgrijalva Sounds good to me. Also, thanks for the great library. You've saved me a countless amount of time.

[pascaldekloe]

https://github.com/square/go-jose/commit/985395ef3b634972a0a831dbc22a1ae9078c5607

[dgrijalva]

@pascaldekloe thanks for the suggestion. I'm looking to include something like that in v4