Calculation of Prev reset is wrong for reset = weekly

[dipixx]

Issue type

• Questions about the server or its usage should be posted to the users mailing list.
• Remote security exploits MUST be sent to security@freeradius.org.
• Defect - Crash or memory corruption.
• Defect - Non compliance with a standards document, or incorrect API usage.
• Defect - Unexpected behaviour (obvious or verified by project member).
• Feature request.

See here for debugging instructions.

Defect/Feature description

In module counter.conf, when a counter has a "weekly" reset, the calculation of the Prev Reset is wrong. In fact the Prev computed on 8 May 2016 is 2016-05-01 00:00:00. The Next Reset is ok (15 May 2016). This means that the period spans over 2 weeks instead of 1.

How to reproduce issue

1- edit counter.conf and add this:

sqlcounter weeklycounter {
    counter-name = Weekly-Session-Time
    check-name = Max-Weekly-Session
    reply-name = Session-Timeout
    sqlmod-inst = sql
    key = User-Name
    reset = weekly
    query = "SELECT COALESCE(SUM(acctsessiontime - \
                 GREATEST((%b - UNIX_TIMESTAMP(acctstarttime)), 0)), 0) \
                 FROM radacct WHERE username = '%{%k}' AND \
                 UNIX_TIMESTAMP(acctstarttime) + acctsessiontime > '%b'"
}

2- call this counter in the authorize section of the sites-available/default file

3- start freeradius -X

Output of [radiusd|freeradius] -X showing issue ocurring

 Module: Instantiating module "weeklycounter" from file /etc/freeradius/sql/mysql/counter.conf
  sqlcounter weeklycounter {
    counter-name = "Weekly-Session-Time"
    check-name = "Max-Weekly-Session"
    reply-name = "Session-Timeout"
    key = "User-Name"
    sqlmod-inst = "sql"
    query = "SELECT COALESCE(SUM(acctsessiontime -                  GREATEST((%b - UNIX_TIMESTAMP(acctstarttime)), 0)), 0)                  FROM radacct WHERE username = '%{%k}' AND                  UNIX_TIMESTAMP(acctstarttime) + acctsessiontime > '%b'"
    reset = "weekly"
    safe-characters = "@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_: /"
  }
rlm_sqlcounter: Reply attribute Session-Timeout is number 27
rlm_sqlcounter: Counter attribute Weekly-Session-Time is number 11281
rlm_sqlcounter: Check attribute Max-Weekly-Session is number 11282
rlm_sqlcounter: Current Time: 1462729723 [2016-05-08 19:48:43], Next reset 1463263200 [2016-05-15 00:00:00]
rlm_sqlcounter: Current Time: 1462729723 [2016-05-08 19:48:43], Prev reset 1462053600 [2016-05-01 00:00:00]

[arr2036]

Nice catch. If you wanted to be super helpful, you could submit a PR for a unit test which demonstrates this issue.

There are a fair few examples already for the SQL modules.

https://github.com/FreeRADIUS/freeradius-server/tree/v3.1.x/src/tests/modules

The interval calculations are done in C, not by the SQL server, so it should be reproducible with sqlite.

Thanks!

[dipixx]

I have very little time, so, I'm sorry but I think I can't be super helpful. However, the fix should be easy:
The source code is at http://doc.freeradius.org/rlm__sqlcounter_8c_source.html
and to fix it, just change line 214 to:
tm->tm_mday -= tm->tm_wday +(7*(num-1));
This will set the prev to the nearest sunday before today.

[alandekok]

Fixed by #1608