Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Fri, 2011-11-04 at 16:55 -0400, Nalin Dahyabhai wrote: > On Fri, Nov 04, 2011 at 04:45:02PM -0400, Simo Sorce wrote: > > After a quick review with nalin offline I decided for a different > > approach that properly covers the range of values we want and is more > > similar to the initial code. > > > > New patches attached. > > Looks good to me. Please bump up KRB5P_SALT_SIZE, say, to 20, unless > there's a good reason not to, though. I decided to open a separate ticket to investigate salt sizes. > Either way, ACK. Pushed to master and ipa-2-1 Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Fri, Nov 04, 2011 at 04:45:02PM -0400, Simo Sorce wrote: > After a quick review with nalin offline I decided for a different > approach that properly covers the range of values we want and is more > similar to the initial code. > > New patches attached. Looks good to me. Please bump up KRB5P_SALT_SIZE, say, to 20, unless there's a good reason not to, though. Either way, ACK. Nalin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Fri, 2011-11-04 at 16:14 -0400, Simo Sorce wrote:
> On Fri, 2011-11-04 at 15:59 -0400, Simo Sorce wrote:
> > On Fri, 2011-11-04 at 15:15 -0400, Nalin Dahyabhai wrote:
> > > On Thu, Nov 03, 2011 at 06:26:15PM -0400, Simo Sorce wrote:
> > > > As stated in the bug in order to attain better interoperability with
> > > > Windows clients we need to change the way we generate the random salt.
> > >
> > > Nack. The data in a krb5_data is of type 'char', and if it's signed,
> > > the math used here doesn't produce a printable result. Might also want
> > > to increase KRB5P_SALT_SIZE.
> >
> > Ah crap, right.
> >
> > I initially used a safe construct: data[i] &= 0x5F
> > Then realized that one of the possible values (5F + 20 = 7F) is
> > unprintable, so I switched to this unsafe one.
> >
> > Will get a revised patch for ipa-2-1 and an amendment for master.
> >
> > Thanks a lot for spotting this one!
>
> Attached amendment patch for master and an already amended new patch for
> ipa-2-1.
After a quick review with nalin offline I decided for a different
approach that properly covers the range of values we want and is more
similar to the initial code.
New patches attached.
--
Simo Sorce * Red Hat, Inc * New York
>From cae692dc4ed817185d51f438a4f1a170b92c324c Mon Sep 17 00:00:00 2001
From: Simo Sorce
Date: Fri, 4 Nov 2011 16:40:25 -0400
Subject: [PATCH] Amend #2038 fix
The math was unsafe, thanks to Nalin for spotting it.
---
util/ipa_krb5.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/util/ipa_krb5.c b/util/ipa_krb5.c
index ba9d3cefce0944d790715c3249f158b9f0ae232d..d03680a6ed3bceb73516d17f5dcef8594fbc382e 100644
--- a/util/ipa_krb5.c
+++ b/util/ipa_krb5.c
@@ -13,7 +13,7 @@ static krb5_error_code ipa_get_random_salt(krb5_context krbctx,
krb5_data *salt)
{
krb5_error_code kerr;
-int i;
+int i, v;
/* make random salt */
salt->length = KRB5P_SALT_SIZE;
@@ -30,8 +30,10 @@ static krb5_error_code ipa_get_random_salt(krb5_context krbctx,
* To avoid any compatibility issue, limits octects only to
* the ASCII printable range, or 0x20 <= val <= 0x7E */
for (i = 0; i < salt->length; i++) {
-salt->data[i] %= 0x5E; /* 7E - 20 */
-salt->data[i] += 0x20; /* add base */
+v = (unsigned char)salt->data[i];
+v %= 0x5E; /* 7E - 20 */
+v += 0x20; /* add base */
+salt->data[i] = v;
}
return 0;
--
1.7.7
>From e82ee7c2fed958b2532adb224a8dcb21fa7f6caa Mon Sep 17 00:00:00 2001
From: Simo Sorce
Date: Thu, 3 Nov 2011 16:15:10 -0400
Subject: [PATCH] Modify random salt creation for interoperability
port to ipa-2-1
ameneded math safety issue
See:
https://fedorahosted.org/freeipa/ticket/2038
---
.../ipa-pwd-extop/ipapwd_encoding.c| 40
1 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c
index cd4610c6ffd6f1b4eae61521335a7e26d319fa9d..fd51ed5db50eb25935b7943859c6d29097d73445 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c
@@ -47,6 +47,7 @@
#include
#include
#include
+#include
#include
#include
@@ -249,6 +250,36 @@ void encode_int16(unsigned int val, unsigned char *p)
p[0] = (val ) & 0xff;
}
+static krb5_error_code ipa_get_random_salt(krb5_context krbctx,
+ krb5_data *salt)
+{
+krb5_error_code kerr;
+int i, v;
+
+/* make random salt */
+salt->length = KRB5P_SALT_SIZE;
+salt->data = malloc(KRB5P_SALT_SIZE);
+if (!salt->data) {
+return ENOMEM;
+}
+kerr = krb5_c_random_make_octets(krbctx, salt);
+if (kerr) {
+return kerr;
+}
+
+/* Windows treats the salt as a string.
+ * To avoid any compatibility issue, limits octects only to
+ * the ASCII printable range, or 0x20 <= val <= 0x7E */
+for (i = 0; i < salt->length; i++) {
+v = (unsigned char)salt->data[i];
+v %= 0x5E; /* 7E - 20 */
+v += 0x20; /* add base */
+salt->data[i] = v;
+}
+
+return 0;
+}
+
static Slapi_Value **encrypt_encode_key(struct ipapwd_krbcfg *krbcfg,
struct ipapwd_data *data,
char **errMesg)
@@ -376,14 +407,7 @@ static Slapi_Value **encrypt_encode_key(struct ipapwd_krbcfg *krbcfg,
case KRB5_KDB_SALTTYPE_SPECIAL:
-/* make random salt */
-salt.length = KRB5P_SALT_SIZE;
-salt.data = malloc(KRB5P_SALT_SIZE);
-if (!salt.data) {
-LOG_OOM();
-goto enc_error;
-}
-krberr = krb5_c_random_make_octets(krbctx, &salt);
+krberr = ipa_get_random_salt(krbctx, &salt);
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Fri, 2011-11-04 at 15:59 -0400, Simo Sorce wrote:
> On Fri, 2011-11-04 at 15:15 -0400, Nalin Dahyabhai wrote:
> > On Thu, Nov 03, 2011 at 06:26:15PM -0400, Simo Sorce wrote:
> > > As stated in the bug in order to attain better interoperability with
> > > Windows clients we need to change the way we generate the random salt.
> >
> > Nack. The data in a krb5_data is of type 'char', and if it's signed,
> > the math used here doesn't produce a printable result. Might also want
> > to increase KRB5P_SALT_SIZE.
>
> Ah crap, right.
>
> I initially used a safe construct: data[i] &= 0x5F
> Then realized that one of the possible values (5F + 20 = 7F) is
> unprintable, so I switched to this unsafe one.
>
> Will get a revised patch for ipa-2-1 and an amendment for master.
>
> Thanks a lot for spotting this one!
Attached amendment patch for master and an already amended new patch for
ipa-2-1.
--
Simo Sorce * Red Hat, Inc * New York
>From 40034df9def29b1a649a5b3d1586966eb186c97e Mon Sep 17 00:00:00 2001
From: Simo Sorce
Date: Fri, 4 Nov 2011 16:04:19 -0400
Subject: [PATCH] Amend #2038 fix
The math was unsafe, thanks to Nalin for spotting it.
---
util/ipa_krb5.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/util/ipa_krb5.c b/util/ipa_krb5.c
index ba9d3cefce0944d790715c3249f158b9f0ae232d..0d487fb8aa1df47295c76e09f841f475a6d0e3de 100644
--- a/util/ipa_krb5.c
+++ b/util/ipa_krb5.c
@@ -30,8 +30,13 @@ static krb5_error_code ipa_get_random_salt(krb5_context krbctx,
* To avoid any compatibility issue, limits octects only to
* the ASCII printable range, or 0x20 <= val <= 0x7E */
for (i = 0; i < salt->length; i++) {
-salt->data[i] %= 0x5E; /* 7E - 20 */
+/* math must be sign-safe as krb5_data octets use signed chars */
+salt->data[i] &= 0x5F; /* Cut down and ... */
salt->data[i] += 0x20; /* add base */
+/* add a pseudo random substitute for unprintable DEL */
+if (salt->data[i] == 0x7F) {
+salt->data[i] = 0x30 + i;
+}
}
return 0;
--
1.7.7
>From d07db98f70759c98a046042100828d3debc4cdcb Mon Sep 17 00:00:00 2001
From: Simo Sorce
Date: Thu, 3 Nov 2011 16:15:10 -0400
Subject: [PATCH] Modify random salt creation for interoperability
port to ipa-2-1
ameneded math safety issue
See:
https://fedorahosted.org/freeipa/ticket/2038
---
.../ipa-pwd-extop/ipapwd_encoding.c| 43
1 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c
index cd4610c6ffd6f1b4eae61521335a7e26d319fa9d..6f61e92be54018d0f3d2c35b2879716d16d96512 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c
@@ -47,6 +47,7 @@
#include
#include
#include
+#include
#include
#include
@@ -249,6 +250,39 @@ void encode_int16(unsigned int val, unsigned char *p)
p[0] = (val ) & 0xff;
}
+static krb5_error_code ipa_get_random_salt(krb5_context krbctx,
+ krb5_data *salt)
+{
+krb5_error_code kerr;
+int i;
+
+/* make random salt */
+salt->length = KRB5P_SALT_SIZE;
+salt->data = malloc(KRB5P_SALT_SIZE);
+if (!salt->data) {
+return ENOMEM;
+}
+kerr = krb5_c_random_make_octets(krbctx, salt);
+if (kerr) {
+return kerr;
+}
+
+/* Windows treats the salt as a string.
+ * To avoid any compatibility issue, limits octects only to
+ * the ASCII printable range, or 0x20 <= val <= 0x7E */
+for (i = 0; i < salt->length; i++) {
+/* math must be sign-safe as krb5_data octets use signed chars */
+salt->data[i] &= 0x5F; /* Cut down and ... */
+salt->data[i] += 0x20; /* add base */
+/* add a pseudo random substitute for unprintable DEL */
+if (salt->data[i] == 0x7F) {
+salt->data[i] = 0x30 + i;
+}
+}
+
+return 0;
+}
+
static Slapi_Value **encrypt_encode_key(struct ipapwd_krbcfg *krbcfg,
struct ipapwd_data *data,
char **errMesg)
@@ -376,14 +410,7 @@ static Slapi_Value **encrypt_encode_key(struct ipapwd_krbcfg *krbcfg,
case KRB5_KDB_SALTTYPE_SPECIAL:
-/* make random salt */
-salt.length = KRB5P_SALT_SIZE;
-salt.data = malloc(KRB5P_SALT_SIZE);
-if (!salt.data) {
-LOG_OOM();
-goto enc_error;
-}
-krberr = krb5_c_random_make_octets(krbctx, &salt);
+krberr = ipa_get_random_salt(krbctx, &salt);
if (krberr) {
LOG_FATAL("krb5_c_random_make_octets failed [%s]\n",
krb5_get_error_message(krbctx, krberr));
--
1.7.7
___
Fr
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Fri, 2011-11-04 at 15:15 -0400, Nalin Dahyabhai wrote: > On Thu, Nov 03, 2011 at 06:26:15PM -0400, Simo Sorce wrote: > > As stated in the bug in order to attain better interoperability with > > Windows clients we need to change the way we generate the random salt. > > Nack. The data in a krb5_data is of type 'char', and if it's signed, > the math used here doesn't produce a printable result. Might also want > to increase KRB5P_SALT_SIZE. Ah crap, right. I initially used a safe construct: data[i] &= 0x5F Then realized that one of the possible values (5F + 20 = 7F) is unprintable, so I switched to this unsafe one. Will get a revised patch for ipa-2-1 and an amendment for master. Thanks a lot for spotting this one! Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Thu, Nov 03, 2011 at 06:26:15PM -0400, Simo Sorce wrote: > As stated in the bug in order to attain better interoperability with > Windows clients we need to change the way we generate the random salt. Nack. The data in a krb5_data is of type 'char', and if it's signed, the math used here doesn't produce a printable result. Might also want to increase KRB5P_SALT_SIZE. Nalin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Fri, 2011-11-04 at 08:03 -0400, Simo Sorce wrote:
> On Fri, 2011-11-04 at 12:55 +0100, Martin Kosek wrote:
> > On Fri, 2011-11-04 at 07:41 -0400, Simo Sorce wrote:
> > > On Fri, 2011-11-04 at 11:14 +0100, Martin Kosek wrote:
> > > > On Fri, 2011-11-04 at 10:04 +0200, Alexander Bokovoy wrote:
> > > > > On Thu, 03 Nov 2011, Simo Sorce wrote:
> > > > > > As stated in the bug in order to attain better interoperability with
> > > > > > Windows clients we need to change the way we generate the random
> > > > > > salt.
> > > > > ACK.
> > > > >
> > > >
> > > > Pushed to master.
> > >
> > > Should we backport this to 2.x as well ?
> > >
> > > Simo.
> > >
> >
> > Hm, looks important enough to do it. You are talking about
> >
> > daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c
>
> Yes
>
> > right? It should be pretty straightforward to backport it there.
>
> Yes
Patch against ipa-2-1 attached.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
>From a94cc05c563240b2ad4058aeac918790065ac886 Mon Sep 17 00:00:00 2001
From: Simo Sorce
Date: Thu, 3 Nov 2011 16:15:10 -0400
Subject: [PATCH] Modify random salt creation for interoperability
port to ipa-2-1
See:
https://fedorahosted.org/freeipa/ticket/2038
---
.../ipa-pwd-extop/ipapwd_encoding.c| 38 +++
1 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c
index cd4610c6ffd6f1b4eae61521335a7e26d319fa9d..4cd2451a4ebaae0a8dd642ca2fb88aeea37cebdb 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c
@@ -47,6 +47,7 @@
#include
#include
#include
+#include
#include
#include
@@ -249,6 +250,34 @@ void encode_int16(unsigned int val, unsigned char *p)
p[0] = (val ) & 0xff;
}
+static krb5_error_code ipa_get_random_salt(krb5_context krbctx,
+ krb5_data *salt)
+{
+krb5_error_code kerr;
+int i;
+
+/* make random salt */
+salt->length = KRB5P_SALT_SIZE;
+salt->data = malloc(KRB5P_SALT_SIZE);
+if (!salt->data) {
+return ENOMEM;
+}
+kerr = krb5_c_random_make_octets(krbctx, salt);
+if (kerr) {
+return kerr;
+}
+
+/* Windows treats the salt as a string.
+ * To avoid any compatibility issue, limits octects only to
+ * the ASCII printable range, or 0x20 <= val <= 0x7E */
+for (i = 0; i < salt->length; i++) {
+salt->data[i] %= 0x5E; /* 7E - 20 */
+salt->data[i] += 0x20; /* add base */
+}
+
+return 0;
+}
+
static Slapi_Value **encrypt_encode_key(struct ipapwd_krbcfg *krbcfg,
struct ipapwd_data *data,
char **errMesg)
@@ -376,14 +405,7 @@ static Slapi_Value **encrypt_encode_key(struct ipapwd_krbcfg *krbcfg,
case KRB5_KDB_SALTTYPE_SPECIAL:
-/* make random salt */
-salt.length = KRB5P_SALT_SIZE;
-salt.data = malloc(KRB5P_SALT_SIZE);
-if (!salt.data) {
-LOG_OOM();
-goto enc_error;
-}
-krberr = krb5_c_random_make_octets(krbctx, &salt);
+krberr = ipa_get_random_salt(krbctx, &salt);
if (krberr) {
LOG_FATAL("krb5_c_random_make_octets failed [%s]\n",
krb5_get_error_message(krbctx, krberr));
--
1.7.7
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Fri, 2011-11-04 at 12:55 +0100, Martin Kosek wrote: > On Fri, 2011-11-04 at 07:41 -0400, Simo Sorce wrote: > > On Fri, 2011-11-04 at 11:14 +0100, Martin Kosek wrote: > > > On Fri, 2011-11-04 at 10:04 +0200, Alexander Bokovoy wrote: > > > > On Thu, 03 Nov 2011, Simo Sorce wrote: > > > > > As stated in the bug in order to attain better interoperability with > > > > > Windows clients we need to change the way we generate the random salt. > > > > ACK. > > > > > > > > > > Pushed to master. > > > > Should we backport this to 2.x as well ? > > > > Simo. > > > > Hm, looks important enough to do it. You are talking about > > daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c Yes > right? It should be pretty straightforward to backport it there. Yes Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Fri, 2011-11-04 at 07:41 -0400, Simo Sorce wrote: > On Fri, 2011-11-04 at 11:14 +0100, Martin Kosek wrote: > > On Fri, 2011-11-04 at 10:04 +0200, Alexander Bokovoy wrote: > > > On Thu, 03 Nov 2011, Simo Sorce wrote: > > > > As stated in the bug in order to attain better interoperability with > > > > Windows clients we need to change the way we generate the random salt. > > > ACK. > > > > > > > Pushed to master. > > Should we backport this to 2.x as well ? > > Simo. > Hm, looks important enough to do it. You are talking about daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd_encoding.c right? It should be pretty straightforward to backport it there. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Fri, 2011-11-04 at 11:14 +0100, Martin Kosek wrote: > On Fri, 2011-11-04 at 10:04 +0200, Alexander Bokovoy wrote: > > On Thu, 03 Nov 2011, Simo Sorce wrote: > > > As stated in the bug in order to attain better interoperability with > > > Windows clients we need to change the way we generate the random salt. > > ACK. > > > > Pushed to master. Should we backport this to 2.x as well ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Fri, 2011-11-04 at 10:04 +0200, Alexander Bokovoy wrote: > On Thu, 03 Nov 2011, Simo Sorce wrote: > > As stated in the bug in order to attain better interoperability with > > Windows clients we need to change the way we generate the random salt. > ACK. > Pushed to master. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] #2038 modify salt creation
On Thu, 03 Nov 2011, Simo Sorce wrote: > As stated in the bug in order to attain better interoperability with > Windows clients we need to change the way we generate the random salt. ACK. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
