[Freeipa-devel] [PATCH 0026] Prevent integer overflow when setting krbPasswordExpiration
Simo Sorce
simo at redhat.com
Thu Jan 17 16:18:39 UTC 2013
On Thu, 2013-01-17 at 15:29 +0100, Tomas Babej wrote:
> 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;
> > +}
> > +
Ok this mail made me look again in the issue.
I see 2 problems here.
1) platform dependent issues.
On i686 time_t is a signed 32bit integer (int)
On x86_64 time_t is a signed 64bit integer (long long)
So when you test you need to be aware on what platform you are testing
in order to know what to expect.
2) The current patch returns time_t *result for the new wrapper function
which is wrong, it shoul return krb5_timestamp as the type.
The actual test done in the code looks ok but only if you think time_t
is a 32bit signed integer.
In that case it will overflow. But on x86_64 it will not.
Sorry for not catching it earlier.
So the way to handle this is to actually check this is to change the
wrapper function to look like this:
int ipadb_ldap_attr_to_krb5_timestamp(LDAP *lcontext, LDAPMessage *le,
char *attrname, krb5_timestamp *result)
{
time_t restime;
long long reslong;
int ret = ipadb_ldab_attr_to_time_t(lcontext, le,
attrname, restime);
if (ret) return ret;
reslong = restime; // <- this will cast correctly maintaing sign to a 64bit variable
if (reslong < 0 || reslong > IPAPWD_END_OF_TIME) {
*result = IPAPWD_END_OF_TIME;
} else {
*result = (krb5_timestamp)reslong;
}
return 0;
}
All calls in ipadb_parse_ldap_entry() that expects a singed 32 bit time
as output should be hanged to use ipadb_ldap_attr_to_krb5_timestamp()
This includes 2 additional calls Tomas pointed to me on a IRC
conversation.
So Nack again :-/
Simo.
--
Simo Sorce * Red Hat, Inc * New York
More information about the Freeipa-devel
mailing list