On 01/17/2013 05:18 PM, Simo Sorce wrote:
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@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.
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?
Tomas
>From d393255a2645bb47639351cd3ef9d805be137b8c Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Mon, 14 Jan 2013 10:19:44 -0500
Subject: [PATCH] Prevent integer overflow when setting krbPasswordExpiration
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).
The timestamp value is truncated to Jan 1, 2038 in ipa-kdc driver.
https://fedorahosted.org/freeipa/ticket/3312
https://fedorahosted.org/freeipa/ticket/3114
---
daemons/ipa-kdb/ipa_kdb.h | 3 +++
daemons/ipa-kdb/ipa_kdb_common.c | 36 ++++++++++++++++++++++++++++++++++++
daemons/ipa-kdb/ipa_kdb_passwords.c | 5 +++++
daemons/ipa-kdb/ipa_kdb_principals.c | 32 +++++++++++++++++++-------------
util/ipa_pwd.h | 3 +++
5 files changed, 66 insertions(+), 13 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h
index 0a179dbcf0e9c17c0eb468638cd7436dc60d31a5..fe79a382d2ef3d41acd24f038b093396064d56f5 100644
--- a/daemons/ipa-kdb/ipa_kdb.h
+++ b/daemons/ipa-kdb/ipa_kdb.h
@@ -140,6 +140,9 @@ int ipadb_ldap_attr_to_bool(LDAP *lcontext, LDAPMessage *le,
char *attrname, bool *result);
int ipadb_ldap_attr_to_time_t(LDAP *lcontext, LDAPMessage *le,
char *attrname, time_t *result);
+int ipadb_ldap_attr_to_krb5_timestamp(LDAP *lcontext, LDAPMessage *le,
+ char *attrname, krb5_timestamp *result,
+ bool check_overflow);
int ipadb_ldap_attr_has_value(LDAP *lcontext, LDAPMessage *le,
char *attrname, char *value);
diff --git a/daemons/ipa-kdb/ipa_kdb_common.c b/daemons/ipa-kdb/ipa_kdb_common.c
index 71df9634c4e25378494b165db9a9381f2b8fc206..672fb6fc9f489caff06a1082474425b8bcd14593 100644
--- a/daemons/ipa-kdb/ipa_kdb_common.c
+++ b/daemons/ipa-kdb/ipa_kdb_common.c
@@ -480,6 +480,42 @@ int ipadb_ldap_attr_to_time_t(LDAP *lcontext, LDAPMessage *le,
return ret;
}
+int ipadb_ldap_attr_to_krb5_timestamp(LDAP *lcontext, LDAPMessage *le,
+ char *attrname, krb5_timestamp *result,
+ bool check_overflow)
+{
+ time_t res_time;
+ long long res_long;
+
+ int ret = ipadb_ldap_attr_to_time_t(lcontext, le,
+ attrname, &res_time);
+ if (ret) return ret;
+
+ /* this will cast correctly maintaing sign to a 64bit variable */
+ res_long = res_time;
+
+ /* Not all attributes that use krb5_timestamp are suspectible to
+ * problems with time overflow, e.g krbLastSuccessfulAuth.
+ * In general, this is only problem with attributes that are set
+ * to some date in the future, e.g. expiration dates.
+ * We want to check those only, since attributes that refer to
+ * dates in the past/present may be purposefully set to negative
+ * values, to trigger some specific behaviour */
+
+ /* For dates beyond IPAPWD_END_OF_TIME, rest_time might oveflow
+ * on 32-bit platforms. This does not apply for 64-bit platforms.
+ * However, since krb5 uses 32-bit time representation, we need
+ * to limit the result.*/
+
+ if (check_overflow && (res_long < 0 || res_long > IPAPWD_END_OF_TIME) ) {
+ *result = IPAPWD_END_OF_TIME; // 1 Jan 2038, 00:00 GMT
+ } else {
+ *result = (krb5_timestamp)res_long;
+ }
+
+ return 0;
+}
+
int ipadb_ldap_attr_has_value(LDAP *lcontext, LDAPMessage *le,
char *attrname, char *value)
{
diff --git a/daemons/ipa-kdb/ipa_kdb_passwords.c b/daemons/ipa-kdb/ipa_kdb_passwords.c
index b6520ea75a78474f6f7761311c9d165924e88b27..974ae8fc872f13d8fcae133527f7a505ea23340d 100644
--- a/daemons/ipa-kdb/ipa_kdb_passwords.c
+++ b/daemons/ipa-kdb/ipa_kdb_passwords.c
@@ -246,6 +246,11 @@ krb5_error_code ipadb_get_pwd_expiration(krb5_context context,
*expire_time = mod_time;
}
+ /* in the case of integer owerflow, set expiration to IPAPWD_END_OF_TIME */
+ if ((*expire_time) < 0 || (*expire_time) > IPAPWD_END_OF_TIME) {
+ *expire_time = IPAPWD_END_OF_TIME; // 1 Jan 2038, 00:00 GMT
+ }
+
kerr = 0;
done:
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index 62155816201f705b7828c861915bf63c6b00177b..53dfda8df34fa318143ad2c39106d61822b92d16 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -237,7 +237,7 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
krb5_kvno mkvno = 0;
char **restrlist;
char *restring;
- time_t restime;
+ krb5_timestamp restime;
bool resbool;
int result;
int ret;
@@ -286,8 +286,9 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
*polmask |= MAXRENEWABLEAGE_BIT;
}
- ret = ipadb_ldap_attr_to_time_t(lcontext, lentry,
- "krbPrincipalexpiration", &restime);
+ ret = ipadb_ldap_attr_to_krb5_timestamp(lcontext, lentry,
+ "krbPrincipalexpiration", &restime,
+ true);
switch (ret) {
case 0:
entry->expiration = restime;
@@ -298,8 +299,9 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
goto done;
}
- ret = ipadb_ldap_attr_to_time_t(lcontext, lentry,
- "krbPasswordExpiration", &restime);
+ ret = ipadb_ldap_attr_to_krb5_timestamp(lcontext, lentry,
+ "krbPasswordExpiration", &restime,
+ true);
switch (ret) {
case 0:
entry->pw_expiration = restime;
@@ -310,8 +312,9 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
goto done;
}
- ret = ipadb_ldap_attr_to_time_t(lcontext, lentry,
- "krbLastSuccessfulAuth", &restime);
+ ret = ipadb_ldap_attr_to_krb5_timestamp(lcontext, lentry,
+ "krbLastSuccessfulAuth", &restime,
+ false);
switch (ret) {
case 0:
entry->last_success = restime;
@@ -322,8 +325,9 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
goto done;
}
- ret = ipadb_ldap_attr_to_time_t(lcontext, lentry,
- "krbLastFailedAuth", &restime);
+ ret = ipadb_ldap_attr_to_krb5_timestamp(lcontext, lentry,
+ "krbLastFailedAuth", &restime,
+ false);
switch (ret) {
case 0:
entry->last_failed = restime;
@@ -471,8 +475,9 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
ied->pw_history = restrlist;
}
- ret = ipadb_ldap_attr_to_time_t(lcontext, lentry,
- "krbLastPwdChange", &restime);
+ ret = ipadb_ldap_attr_to_krb5_timestamp(lcontext, lentry,
+ "krbLastPwdChange", &restime,
+ false);
if (ret == 0) {
krb5_int32 time32le = htole32((krb5_int32)restime);
@@ -487,8 +492,9 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
ied->last_pwd_change = restime;
}
- ret = ipadb_ldap_attr_to_time_t(lcontext, lentry,
- "krbLastAdminUnlock", &restime);
+ ret = ipadb_ldap_attr_to_krb5_timestamp(lcontext, lentry,
+ "krbLastAdminUnlock", &restime,
+ false);
if (ret == 0) {
krb5_int32 time32le = htole32((krb5_int32)restime);
diff --git a/util/ipa_pwd.h b/util/ipa_pwd.h
index 00de889ff53cdc113a6c926e35c87e7b08238e4a..a6990cac6333bf2582fb071a507001b10145df6d 100644
--- a/util/ipa_pwd.h
+++ b/util/ipa_pwd.h
@@ -27,6 +27,9 @@
#define IPAPWD_DEFAULT_PWDLIFE (90 * 24 *3600)
#define IPAPWD_DEFAULT_MINLEN 0
+/* 1 Jan 2038, 00:00 GMT */
+#define IPAPWD_END_OF_TIME 2145916800
+
/*
* IMPORTANT: please update error string table in ipa_pwd.c if you change this
* error code table.
--
1.8.1
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
