IceTV data causing denial of service

[prl]

I merged the first pull request and followed it up with a commit that puts in some TODO: markers near the code dealing with quantities that ought to be range checked. well, at least the things that seemed the most obvious.

I have already coded a future commit that includes range checking. I'll compare it with your TODOs to help make sure that everything's covered.

[prl]

Or perhaps MemAvailable might be a better measure to use than MemFree?

I don't know if you are still pursuing memory usage with the IceTV plugin, but...
I had a script running on my T2 for a few hours monitoring memory usage.
It reported the lowest MemFree of 11,264 KB (MemAvailable 69,584 KB), which was about 5 minutes after the GUI had started. At the initial IceTV fetch time it was around 18,328 KB.
Later it settled to range between 49,000 and 60,000 KB (MemAvailable 71,000 - 75,000 KB).

The T4 had its lowest MemFree at 34,296 KB (MemAvailable of 927,504 KB).
MemAvailable ranged from 922,000 to 996,000 KB.

If I use MemFree, I've seen the initial IceTV fetch on the V2 use batches of 1, 8 and 14 channels. It seems to depend on exactly when the IceTV code runs in relation to everything else that's happening at startup.

I've currently got it coded to use MemAvailable with 60 MB headroom, and to scale its estimate of the amount of data/channel by the number of days since the last update (rounded up). That means that after the first, full, fetch, subsequent incremental fetches will be done using a smaller number of batches, typically 1.

[Grumpy_Geoff]

If I use MemFree, I've seen the initial IceTV fetch on the V2 use batches of 1, 8 and 14 channels. It seems to depend on exactly when the IceTV code runs in relation to everything else that's happening at startup.

I've currently got it coded to use MemAvailable with 60 MB headroom, and to scale its estimate of the amount of data/channel by the number of days since the last update (rounded up). That means that after the first, full, fetch, subsequent incremental fetches will be done using a smaller number of batches, typically 1.

On the T2, the lowest MemAvailable I saw was 55,272 KB and was around the initial IceTV fetch time -

Code: Select all

{10294}<  9863.840> [IceTV] 2020-11-06 12:59:46: IceTV started
{10453}<  9866.947> [IceTV] 2020-11-06 12:59:49: Start update
{10453}<  9894.865> [IceTV] 2020-11-06 13:00:17: EPG download OK

Time,MemTotal,MemFree,MemAvailable,Buffers,Cached
201106-125909,231644,117196,138704,1548,37624
201106-125939,231644,69508,104768,2152,50476
201106-130009,231644,18328,55272,1968,52056
201106-130039,231644,39332,74476,1984,50456

GUI startup was 12:59:07, according to the debug log file name

[prl]

On the T2, the lowest MemAvailable I saw was 55,272 KB and was around the initial IceTV fetch time

It wouldn't surprise me if the fetch was at least in part the cause of the dip in MemAvailable. The idea of the batching is to reduce the size of that dip on machines with less usable memory.

[prl]

Peteru:

I have some questions about the TODOs related to the range checking in the IceTV plugin:

[EPG] TODO: Discard this entry if any of (start, stop, duration) are negative or bigger than 2147483647 (not maxint)

Why not sys.maxint? For example, in Mac Python, sys.maxint is 0x7fffffffffff and time is also 64 bits. Using sys.maxint seems potentially more portable to other architectures. There are also limits imposed by the 40-bit MJD EPG time in the DVB standards (which are greater than 0x7fffffff and less than 0x7fffffffffff. My current code currently chooses the smaller of sys.maxint and the 40-bit MJD time limit (the valid DVB time range is 1 Mar 1900 .. 28 Feb 2100 inclusive).

And why are negative time values not permitted? Why would 00:00:00 1 Jan 1970 be OK as a time, but not 23:59:59 31 Dec 1969? Both of them are probably equally incorrect for the current EPG.

Also the valid DVB EPG range for duration is 0..0xffffff, not 0..0x7fffffff.

[Timer]TODO: Check that neither start nor duration are negative or bigger than 2147483647 (not maxint)

Same question about why maxint and negative times (but not negative durations) should not be used here. Also, start, duration and start+duration all need to be less than the limit.

[prl]

My current time limit setting code is:

Code: Select all

    # The DVB EIT EPG supports times in the range
    # 1900-03-01 .. 2100-02-28 UTC inclusive,
    # but current implementations have typedef long int time_t,
    # which is usually 32 bits.

    # EIT EPG times should be TIME_LOWER <= t <= TIME_UPPER
    TIME_LOWER = timegm((1900, 3, 1, 0, 0, 0, 0, 0, 0))  # 1900-03-01 00:00:00
    TIME_UPPER = timegm((2100, 2, 28, 23, 59, 59, 0, 0, 0))  # 2100-02-28 23:59:59
    try:
        gmtime(TIME_LOWER)
    except ValueError:
        TIME_LOWER = -sys.maxint - 1
    try:
        gmtime(TIME_UPPER)
    except ValueError:
        TIME_UPPER = sys.maxint

[peteru]

Why not sys.maxint?

Because it's not available in Python 3. I'm trying to pre-empt a porting complication in the near future.

And why are negative time values not permitted?

Because they don't make any sense for any practical intents. The whole point of adding these range checks is to increase the robustness of the system. Ingesting values that have the potential to cause underflows and overflows is not a good idea. Supporting date ranges back to 1970 is purely academic. There really is no reason to go back that far for start or stop times - the lower bound could easily be 1 Jan 2000. Although the start value 999 is used to force an eviction of an entry from the EPG cache, it does not come via the IceTV API. The duration should not be negative because it does not make any sense to have an event that is supposed to start after it has finished. Performing sanity checks on the values received from the IceTV server (and logging a limited number of entries that fail the check) is a good defensive and diagnostic strategy. If the IceTV server starts sending events that go back to before 2020, you can be pretty sure that something is not right and you're better off ignoring whatever the server is sending. I tried to keep the TODO: comment to a single line, hence the wording to suggest a rule that can be applied to all three variables.

To reiterate, the idea behind these checks is not to check the input for all valid values, the idea is to limit the input to sensible values.

start, duration and start+duration all need to be less than the limit.

Good catch. It actually gets a little more complicated because of the padding being added later, which also has a potential to cause an overflow. Maybe the upper bound should be set to 2147400000 - this is less than a day away from the largest possible value. In the grand scheme of things, it makes no difference if the Y2k38 problem manifests itself a day earlier, but the revised value is very likely to account for any sensible padding value that the user configures.

I wonder if there should be more strict sanity checks on the durations. I can't see sensible EPG events spanning more than 24 hours, but for the benefit of doubt, let's say they could stretch for as long as 14 days! Similarly, timers longer than 24 hours are also suspect, but for consistency, timers should be allowed to be as long as events. Something worth contemplating.

[peteru]

My current time limit setting code is:

Code: Select all

    # The DVB EIT EPG supports times in the range
    # 1900-03-01 .. 2100-02-28 UTC inclusive,
    # but current implementations have typedef long int time_t,
    # which is usually 32 bits.

    # EIT EPG times should be TIME_LOWER <= t <= TIME_UPPER
    TIME_LOWER = timegm((1900, 3, 1, 0, 0, 0, 0, 0, 0))  # 1900-03-01 00:00:00
    TIME_UPPER = timegm((2100, 2, 28, 23, 59, 59, 0, 0, 0))  # 2100-02-28 23:59:59
    try:
        gmtime(TIME_LOWER)
    except ValueError:
        TIME_LOWER = -sys.maxint - 1
    try:
        gmtime(TIME_UPPER)
    except ValueError:
        TIME_UPPER = sys.maxint

That's a lot of code. I'd just make it:

Code: Select all

    # Prevent negative values
    TIME_LOWER = 0
    # Almost a day short of Y2k38 (rounded down to allow slack for timer padding)
    TIME_UPPER = 2147400000

Simple, maintainable, clear enough and faster.

[prl]

Yes, it's faster, but it's executed once at startup time, so it's not as though it's a huge difference.

I see your point about sys.maxint disappearing in Py3.

[prl]

I've been looking at this code:

Code: Select all

            event_id = int(show.get("eit_id"))
            if event_id is None or event_id <= 0 or event_id >= 0xFFF7:
                event_id = ice.showIdToEventId(show["id"])

partly because if show.get("eit_id") is None, the int() will throw an exception, and partly because I want to put the equivalent code into processTimers() to make sure that timer event ids correspond with the EPG cache event ids.

That got me thinking about the 0xFFF7 limit. I think that that check dates to when all internal event ids were hashed from the longer IceTV event id. I can't see a good reason why the check shouldn't be event_id <= 0xFFFF, but I've emailed Danial Hall@IceTV to ask what the value range actually is. The limit is definitely larger than 0x7FFF, because I have JSON captures of IceTV data where the eit_id is larger than that.

[prl]

Daniel says that the valid range for IceTV eit_id values is 1..65525 (0x0001 .. 0xFFF5) inclusive.