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

[Martin Kosek]

On 01/25/2013 12:45 AM, Simo Sorce wrote:
> On Wed, 2013-01-23 at 14:00 +0100, Tomas Babej wrote:
>> On 01/22/2013 07:39 PM, Dmitri Pal wrote:
>>> On 01/22/2013 10:57 AM, Simo Sorce wrote:
>>>> On Tue, 2013-01-22 at 15:50 +0100, Tomas Babej wrote:
>>>>> Here I bring the updated version of the patch. Please note, that I
>>>>> *added* a flag attribute to ipadb_ldap_attr_to_krb5_timestamp
>>>>> function, that controls whether the timestamp will be checked for
>>>>> overflow or not. The reasoning behind this is that some attributes
>>>>> will not be set to future dates, due to their inherent nature - such
>>>>> as krbLastSuccessfulAuth or krbLastAdminUnlock.
>>>>>
>>>>> These are all related to past dates, and it would make no sense to set
>>>>> them to future dates, even manually. Therefore I'd rather represent
>>>>> negative values in these attributes as past dates. They would have to
>>>>> be set manually anyway, because they would represent timestamps before
>>>>> the beginning of the unix epoch, however, I find this approach better
>>>>> than pushing them up to year 2038 in case such things happens.
>>>>>
>>>>> Any objections to this approach?
>>>>>
>>>> I am not sure I understand what is the point of giving this option to
>>>> callers. A) How does an API user know when to use one or the other
>>>> option. B) What good does it make to have the same date return different
>>>> results based on a flag ?
>>>>
>>>> What will happen later on when MIT will 'fix' the 2038 limit by changing
>>>> the meaning of negative timestamps ? Keep in mind that right now
>>>> negative timestamps are not really valid in the MIT code.
>>>>
>>>> Unless there is a 'use' for getting negative timestamps I think it is
>>>> only harmful to allow it and consumers would only be confused on whether
>>>> it should be used or not.
>>>>
>>>> So my first impression is that you are a bit overthinking here and we
>>>> should instead always force the same behavior for all callers and always
>>>> check and enforce endoftime dates.
>>>>
>>>> Simo.
>>>>
>>> +1
>> Ok, the patch does not distinguish between 'past' and 'future' 
>> timestamps anymore.
>>
>> Please respond if you see any issues.
> 
> I am sorry I haven't replied yet. I meant to test the patch this time
> before ACKing given how fiddly this time issue seem to be but haven't
> had found the time so far.
> 
> Just want to say I do not see any problem in the last incarnation of the
> patch, so if someone beats me to testing you have my blessing for an
> ACK.
> 
> Simo.
> 

I tested the change and it worked fine. With Simo's blessing on ipa-kdb changes
I am giving a second ACK.

Pushed to master, ipa-3-1.

Martin