[Freeipa-devel] [PATCH 0026] Prevent integer overflow when setting krbPasswordExpiration
Tomas Babej
tbabej at redhat.com
Thu Jan 17 14:29:37 UTC 2013
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:
>>>>>>>>>>>>> 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.
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;
> +}
> +
Tomas
>
> --
> Thank you,
> Dmitri Pal
>
> Sr. Engineering Manager for IdM portfolio
> Red Hat Inc.
>
>
> -------------------------------
> Looking to carve out IT costs?
> www.redhat.com/carveoutcosts/
>
>
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130117/fd0e4664/attachment.htm>
More information about the Freeipa-devel
mailing list