[Freeipa-devel] [PATCH 0026] Prevent integer overflow when setting krbPasswordExpiration

[Tomas Babej]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0026-4-Prevent-integer-overflow-when-setting-krbPasswordExp.patch
Type: text/x-patch
Size: 2999 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130116/e73bafcc/attachment.bin>