[Freeipa-devel] [PATCH 0026] Prevent integer overflow when setting krbPasswordExpiration
Dmitri Pal
dpal at redhat.com
Thu Jan 17 00:56:55 UTC 2013
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:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> 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).
>>>>>>>>>>>>
>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3312
>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3114
>>>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Simo.
>>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> Simo.
>>>>>>
>>>>> 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.
>>>>
>>>> Simo.
>>>>
>>> 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.
>>
>> Simo.
>>
> Done.
>
> Tomas
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
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.
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.
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().
+
+ /* 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?
www.redhat.com/carveoutcosts/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130116/6b7bdce4/attachment.htm>
More information about the Freeipa-devel
mailing list