On 01/17/2013 01:56 AM, Dmitri Pal wrote:
On 01/16/2013 12:32 PM, Tomas Babej wrote:
On 01/16/2013 06:01 PM, Simo Sorce wrote:
On Wed, 2013-01-16 at 17:57 +0100, Tomas Babej wrote:
On 01/16/2013 02:47 PM, Simo Sorce wrote:
On Wed, 2013-01-16 at 12:52 +0100, Tomas Babej wrote:
On 01/15/2013 11:55 PM, Simo Sorce wrote:
On Tue, 2013-01-15 at 17:36 -0500, Dmitri Pal wrote:
On 01/15/2013 03:59 PM, Simo Sorce wrote:
On Tue, 2013-01-15 at 15:53 -0500, Rob Crittenden wrote:
Dmitri Pal wrote:
On 01/15/2013 08:48 AM, Simo Sorce wrote:
On Mon, 2013-01-14 at 16:46 +0100, Tomas Babej wrote:

Since in Kerberos V5 are used 32-bit unix timestamps, setting
maxlife in pwpolicy to values such as 9999 days would cause
integer overflow in krbPasswordExpiration attribute.

This would result into unpredictable behaviour such as users
not being able to log in after password expiration if password
policy was changed (#3114) or new users not being able to log
in at all (#3312).

Given that we control the KDC LDAP driver I think we should not limit the time in LDAP but rather 'fix-it-up' for the KDC in the DAL driver.
Fix how? Truncate to max in the driver itself if it was entered beyond max? Shouldn't we also prevent entering the invalid value into the attribute?

I've been mulling the same question for a while. Why would we want to
let bad data get into the directory?
It is not bad data and the attribute holds a Generalize time date.

The data is valid it's the MIT code that has a limitation in parsing it.

Greg tells me he plans supporting additional time by using the
'negative' part of the integer to represent the years beyond 2038.

So we should represent data in the directory correctly, which means whtever date in the future and only chop it when feeding MIT libraries until they support the additional range at which time we will change and
chop further in the future (around 2067 or so).

If we chopped early in the directory we'd not be able to properly
represent/change rapresentation later when MIT libs gain additional
range capabilities.


We would have to change our code either way and the amount of change
will be similar so does it really matter?
Yes it really matters IMO.


Updated patch attached.
This part looks ok but I think you also need to properly set
krb5_db_entry-> {expiration, pw_expiration, last_success, last_failed}
in ipadb_parse_ldap_entry()

Perhaps the best way is to introduce a new function
ipadb_ldap_attr_to_krb5_timestamp() in ipa_kdb_common.c so that you do
all the overflow checkings once.


They all use ipadb_ldap_attr_to_time_t() to get their values,
so the following addition to the patch should be sufficient.
It will break dates for other users of the function that do not need to
artificially limit the results. Please add a new function.




Freeipa-devel mailing list
Nack from me, sorry.

+int ipadb_ldap_attr_to_krb5_timestamp(LDAP *lcontext, LDAPMessage *le,
+                                      char *attrname, time_t *result)
+    int ret = ipadb_ldab_attr_to_time_t(lcontext, le,
+                                        attrname, result);

This function converts the time from the LDAP time by reading the string into 
the struct elements and then constructing time by using timegm() which is 
really a wrapper around mktime().
According to mktime() man page:

        If  the  specified broken-down time cannot be represented as calendar 
time (seconds since
        the Epoch), mktime() returns a value of (time_t) -1 and does not alter 
the members of the
        broken-down time structure.

However it might not be the case so it would be nice to check if this function 
actually returns some negative value other than -1 if the time is overflown.
Regardles of that part if it always returns -1 or can return other negative 
values like -2 or -3 the check below would produce positive result thus causing 
the function to return whatever is currently in the *result.
I double checked the behaviour. It holds that mktime() and timegm() behave the same way, up to time-zone difference. I don't know whether the man page is not correct, or the implementation in the standard library is not compliant, however, mktime() never returns -1 as return value (if it was not given tm struct which refers to 31 Dec 1969 29:59:59).

I guess the implementation was changed as there would be no way how to distinguish between correct output of 31 Dec 1969 29:59:59 and incorrect output.

I tested both incorrect calendar days (like 14th month) and dates behind year 2038. As expected, dates after the end of unix epoch overflow big time (values such as -2143152000). Incorrect dates just get converted into correct ones - 14th month was interpreted as +1 year +3 months.

IMO the whole fix should be implemented inside the function above when the 
conversion to time_t happens so that it would never produce a negative result.
Simo had objections to this approach since this would limit the other uses of ipadb_ldap_attr_to_time_t() function.
My suggestion would be before calling timegm() to test if the year, month, and 
day returned by the break-down structure contain the day after the last day of 
epoch and then just use use last day of epoc without even calling timegm().
Indeed it would be simpler approach. However, for the current solution to malfunction (overflow back to the positive values), the date would have to be set to something beyond year ~2100.
That would correspond to maxlife of ~32 000 days with present dates.

I guess there might be admins setting crazy values like that, I'll send updated patch.

+    /* in the case of integer owerflow, set result to IPAPWD_END_OF_TIME */
+    if ((*result+86400) < 0) {

  +        *result = IPAPWD_END_OF_TIME; // 1 Jan 2038, 00:00 GMT
+    }
+    return ret;

Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.

Looking to carve out IT costs?

Freeipa-devel mailing list

Freeipa-devel mailing list

Reply via email to