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

[Simo Sorce]

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.

-- 
Simo Sorce * Red Hat, Inc * New York