Guard Against long Timestamp Overflow

[wojciech-piotrowiak]

My fix is bit complicated, had other ideas however they failed. Problem is forwardInterval parameter which is of type long and contain seconds. AFAIK we have to process with millis and to transform seconds to millis we need to multiply, so result value will not fit into long. In result had to use bigger variable. It means using BigInteger which from my understanding is not supported by Interval class from joda-time

[wojciech-piotrowiak]

Alternative approach is to forget about millis precision (because skew and forwardInterval are is seconds) and do the same math on seconds. This means using longs and LongRange which should be more performance friendly. Already tried this approach and have working code.

[jzheaux]

@wojciech-piotrowiak, I agree that the code is planning on forwardInterval being in the int range, even if it accepts a long (due to the cast).

So, I think we should first validate the inputs. Something like:

Assert.isTrue(forwardInterval >= 0 && forwardInterval <= Integer.MAX_VALUE, ...

That way if anyone is accidentally using a bigger value, they get alerted to their misconfiguration.

Also, JodaTime has a class called Duration that could be helpful here. The size of the Duration will be 2 * skew + forwardInterval, so you could do:

Duration duration = Duration.standardSeconds(forwardInterval)
        .withDurationAdded(Duration.standardSeconds(skewInSec), 2);
DateTime end = new DateTime().plusSeconds(skewInSec);
Interval interval = new Interval(duration, end);

There are probably dozens of ways to skin this, but the nice thing about this one is that it relies on JodaTime for the arithmetic.

Last, I think while we are here, the JavaDoc should get updated. Currently, it refers to a non-existent parameter (backwardInterval). Would you mind taking a swing at correcting the description?

[wojciech-piotrowiak]

Fixed javaDoc, added input validation and changed logic a bit to allow maximum integer.

In my opinion it is technically possible to keep using forwardInterval as long, but not sure if makes sense. Max integer in seconds is 24 days where max long is 292,471,205,752 in years. Seen two usages in this project of this method, one use int and second using long.

I don't know the context of this project and approach to backward compatibility. Having public method is kind of contract, keeping forwardInterval as long with a new limitation might break such contract.

[jzheaux]

Max integer in seconds is 24 days

How do you figure? The way I see it is MAX_INTEGER is equal to 2147483647, and that many seconds is about 60 years:

2147483647 / 60 / 60 / 24 / 365 ~= 68

[jzheaux]

keeping forwardInterval as long with a new limitation might break such contract.

It's not a new limitation, though, because the class internally casts forwardInterval to int. So, actually, if someone is passing something larger than Integer.MAX_VALUE into the method, it is overflowing and behaving incorrectly.

[wojciech-piotrowiak]

Sorry for wrong calculation! 68 years is super enough, so maybe this method can be marked as deprecated and a new one with forwardInterval as int should be added? This would be even stronger than javaDoc.

Some of client might even not noticed overflow problem because their values after being overflown are positive :)

[wojciech-piotrowiak]

@jzheaux I mean keeping the same calculation logic in one place, just a new suggested method added and old marked as deprecated:

@Deprecated
    public static boolean isDateTimeSkewValid(int skewInSec, long forwardInterval, DateTime time) {
        Assert.isTrue(forwardInterval >= 0 && forwardInterval <= Integer.MAX_VALUE,"forwardInterval param is too high. Only positive integer value is allowed.");

        return isDateTimeSkewValid(skewInSec,(int)forwardInterval,time);
    }

    public static boolean isDateTimeSkewValid(int skewInSec, int forwardInterval, DateTime time) {
        final DateTime reference = new DateTime();
        final Interval validTimeInterval = new Interval(
                reference.minusSeconds(skewInSec).minusSeconds(forwardInterval),
                reference.plusSeconds(skewInSec)
        );
        return validTimeInterval.contains(time);
    }

Of course javaDoc will be added

[jzheaux]

Sorry for wrong calculation!

No worries, date arithmetic bends my brain all the time. :)

deprecation

I agree that introducing a new method and marking the old one as deprecated could help clarify to developers what's going on.

Since these are patch releases, however, we ought to avoid adding deprecation markers if we can. And actually, we want to avoid adding to the public API (introducing new methods).

Additionally, there's a downstream effect that can complicate maintenance:

Since it's usually best for the project itself to not use any deprecated methods, it'd be important to also change WebSSOProfileConsumerImpl#verifyAuthenticationStatement to call the new undeprecated SAMLUtil method. That would be unfortunate in this case, though, since then it would continue to silently overflow for folks who were using a too-high maxAuthenticationAge.

On the other hand, leaving verifyAuthenticationStatement calling the deprecated method is a bit of a time bomb since it could be forgotten one of these days that it's intentionally calling the deprecated method to ensure correct behavior.

suggested method

final Interval validTimeInterval = new Interval(
                reference.minusSeconds(skewInSec).minusSeconds(forwardInterval),
                reference.plusSeconds(skewInSec)
);

Yes, this implementation is simpler. Not sure what I was thinking with my earlier suggestion. :) It was probably an artifact from trying to get long to work.

[wojciech-piotrowiak]

@jzheaux Thanks for explanation, that makes sense in maintenance mode. Do you accept current state of this pull request?

[jzheaux]

@wojciech-piotrowiak The PR looks good! Will you please squash your commits and, while you're there, format the commit message like this:

Limit forwardInterval Value

Fixes gh-478

[wojciech-piotrowiak]

@jzheaux Done!

[jzheaux]

Thanks, @wojciech-piotrowiak! This is now merged into master via 982fc793ce804d1f00ec3965cec5a719ecfdf079. Also, I did a small polish in 3cb5ea1c872c64374e37994544cbc1decd9cfca0.

Note that I've taken some time to simplify the branch structure since this project is in maintenance mode - for future pull requests, feel free to pull request straight to master.

[wojciech-piotrowiak]

@jzheaux Thank you too!