4:48 a.m.
---
src/iwd.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/iwd.h b/src/iwd.h
index 22223526..5371d8e5 100644
--- a/src/iwd.h
+++ b/src/iwd.h
@@ -22,6 +22,15 @@
#define uninitialized_var(x) x = x
+/*
+ * Set a maximum to prevent sending too much data to the kernel when hashing
+ * the passphrase (or any other crypto operations involving the passphrase).
+ * This will also prevent potential stack overflows if the passphrase is put
+ * into EAP packets on the stack (EAP-GTC). This value is not tied to IEEE or
+ * any RFC's, just chosen to be long enough to not restrict a normal user.
+ */
+#define IWD_MAX_PASSPHRASE_LEN 2048
+
struct l_genl;
struct l_genl_family;
--
2.17.1
4:48 a.m.
New subject: [PATCH 2/4] eap: check MTU when loading identity
If the MTU was set very low an identity could exceed the maximum.
---
src/eap.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/eap.c b/src/eap.c
index 012bd58e..816b5f9a 100644
--- a/src/eap.c
+++ b/src/eap.c
@@ -568,10 +568,17 @@ bool eap_load_settings(struct eap_state *eap, struct l_settings
*settings,
* octets. Support for an NAI length of 253 octets is RECOMMENDED.
* ...
* RADIUS is unable to support NAI lengths beyond 253 octets
+ *
+ * We also need to fail if the identity is too large for the set MTU
+ * size minus 5 (header).
*/
- if (eap->identity && strlen(eap->identity) > 253) {
- l_error("Identity is too long");
- goto err;
+ if (eap->identity) {
+ size_t id_len = strlen(eap->identity);
+
+ if (id_len > 253 || id_len > eap->mtu - 5) {
+ l_error("Identity is too long");
+ goto err;
+ }
}
return true;
--
2.17.1
5:18 a.m.
New subject: [PATCH 2/4] eap: check MTU when loading identity
Hi James,
On 3/6/20 11:48 AM, James Prestwood wrote:
If the MTU was set very low an identity could exceed the maximum.
---
src/eap.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
Applied, thanks.
Regards,
-Denis
4:48 a.m.
New subject: [PATCH 3/4] agent: enforce a maximum passphrase length
---
src/agent.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/agent.c b/src/agent.c
index d213f3cd..44039a89 100644
--- a/src/agent.c
+++ b/src/agent.c
@@ -159,6 +159,9 @@ static void passphrase_reply(struct l_dbus_message *reply,
if (!l_dbus_message_get_arguments(reply, "s", &passphrase))
goto done;
+ if (strlen(passphrase) > IWD_MAX_PASSPHRASE_LEN)
+ goto done;
+
result = AGENT_RESULT_OK;
done:
@@ -181,6 +184,9 @@ static void user_name_passwd_reply(struct l_dbus_message *reply,
if (!l_dbus_message_get_arguments(reply, "ss", &username, &passwd))
goto done;
+ if (strlen(passwd) > IWD_MAX_PASSPHRASE_LEN)
+ goto done;
+
result = AGENT_RESULT_OK;
done:
--
2.17.1
5:27 a.m.
New subject: [PATCH 3/4] agent: enforce a maximum passphrase length
Hi James,
On 3/6/20 11:48 AM, James Prestwood wrote:
---
src/agent.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/agent.c b/src/agent.c
index d213f3cd..44039a89 100644
--- a/src/agent.c
+++ b/src/agent.c
@@ -159,6 +159,9 @@ static void passphrase_reply(struct l_dbus_message *reply,
if (!l_dbus_message_get_arguments(reply, "s", &passphrase))
goto done;
+ if (strlen(passphrase) > IWD_MAX_PASSPHRASE_LEN)
+ goto done;
+
This callback is used for three things from what I remember:
1. The passphrase for decrypting private keys
2. The passphrase for PSK networks.
3. The password for EAP methods.
This checking should only apply to 3, no?
result = AGENT_RESULT_OK;
done:
@@ -181,6 +184,9 @@ static void user_name_passwd_reply(struct l_dbus_message *reply,
if (!l_dbus_message_get_arguments(reply, "ss", &username, &passwd))
goto done;
+ if (strlen(passwd) > IWD_MAX_PASSPHRASE_LEN)
+ goto done;
+
result = AGENT_RESULT_OK;
done:
Regards,
-Denis
5:46 a.m.
New subject: [PATCH 3/4] agent: enforce a maximum passphrase length
On Fri, 2020-03-06 at 12:27 -0600, Denis Kenzior wrote:
Hi James,
On 3/6/20 11:48 AM, James Prestwood wrote:
> ---
> src/agent.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/src/agent.c b/src/agent.c
> index d213f3cd..44039a89 100644
> --- a/src/agent.c
> +++ b/src/agent.c
> @@ -159,6 +159,9 @@ static void passphrase_reply(struct
> l_dbus_message *reply,
> if (!l_dbus_message_get_arguments(reply, "s", &passphrase))
> goto done;
>
> + if (strlen(passphrase) > IWD_MAX_PASSPHRASE_LEN)
> + goto done;
> +
This callback is used for three things from what I remember:
1. The passphrase for decrypting private keys
2. The passphrase for PSK networks.
3. The password for EAP methods.
This checking should only apply to 3, no?
Well, we should probably have a maximum on all three... The private key
passphrase is sent to the kernel, so we probably don't want that to be
huge seems like we could put the same limitation on both 1 and 3. In
addition for PSK passphrases we should probably enforce 8-63 byte
lengths if we don't already somewhere else.
> result = AGENT_RESULT_OK;
>
> done:
> @@ -181,6 +184,9 @@ static void user_name_passwd_reply(struct
> l_dbus_message *reply,
> if (!l_dbus_message_get_arguments(reply, "ss", &username,
> &passwd))
> goto done;
>
> + if (strlen(passwd) > IWD_MAX_PASSPHRASE_LEN)
> + goto done;
> +
> result = AGENT_RESULT_OK;
>
> done:
>
Regards,
-Denis
_______________________________________________
iwd mailing list -- iwd(a)lists.01.org
To unsubscribe send an email to iwd-leave(a)lists.01.org
5:43 a.m.
New subject: [PATCH 3/4] agent: enforce a maximum passphrase length
Hi James,
> This callback is used for three things from what I remember:
> 1. The passphrase for decrypting private keys
> 2. The passphrase for PSK networks.
> 3. The password for EAP methods.
>
> This checking should only apply to 3, no?
Well, we should probably have a maximum on all three... The private key
passphrase is sent to the kernel, so we probably don't want that to be
huge seems like we could put the same limitation on both 1 and 3. In
addition for PSK passphrases we should probably enforce 8-63 byte
lengths if we don't already somewhere else.
I'd be fine with that, but then using a #define with the name PASSWORD
starts looking a bit funny.
Do note that the 8..63 limit for 2) is already enforced by
crypto_psk_from_passphrase.
Regards,
-Denis
5:59 a.m.
New subject: [PATCH 3/4] agent: enforce a maximum passphrase length
On Fri, 2020-03-06 at 12:43 -0600, Denis Kenzior wrote:
Hi James,
> > This callback is used for three things from what I remember:
> > 1. The passphrase for decrypting private keys
> > 2. The passphrase for PSK networks.
> > 3. The password for EAP methods.
> >
> > This checking should only apply to 3, no?
>
> Well, we should probably have a maximum on all three... The private
> key
> passphrase is sent to the kernel, so we probably don't want that to
> be
> huge seems like we could put the same limitation on both 1 and 3.
> In
> addition for PSK passphrases we should probably enforce 8-63 byte
> lengths if we don't already somewhere else.
>
I'd be fine with that, but then using a #define with the name
PASSWORD
starts looking a bit funny.
Do note that the 8..63 limit for 2) is already enforced by
crypto_psk_from_passphrase.
Indeed. So I'll leave the WPA passphrase path alone. Instead I can move
these checks into network.c, specific to pkeys and EAP passwords.
Either put checks in eap_secret_done, or in eap_password_callback and
eap_user_password_callback.
Thanks,
James
Regards,
-Denis
_______________________________________________
iwd mailing list -- iwd(a)lists.01.org
To unsubscribe send an email to iwd-leave(a)lists.01.org
4:48 a.m.
New subject: [PATCH 4/4] eap-gtc: limit password length to maximum
The password for EAP-GTC is directly used in an EAP response. The
response buffer is created on the stack so an overly large password
could cause a stack overflow.
---
src/eap-gtc.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/eap-gtc.c b/src/eap-gtc.c
index 7788d44c..8092eee1 100644
--- a/src/eap-gtc.c
+++ b/src/eap-gtc.c
@@ -31,6 +31,7 @@
#include "src/missing.h"
#include "src/eap.h"
#include "src/eap-private.h"
+#include "src/iwd.h"
struct eap_gtc_state {
char *password;
@@ -148,6 +149,14 @@ static bool eap_gtc_load_settings(struct eap_state *eap,
return false;
}
+ /*
+ * Limit length to prevent a stack overflow
+ */
+ if (strlen(password) > IWD_MAX_PASSPHRASE_LEN) {
+ l_free(password);
+ return false;
+ }
+
gtc = l_new(struct eap_gtc_state, 1);
gtc->password = password;
--
2.17.1
4:56 a.m.
On Fri, 2020-03-06 at 09:48 -0800, James Prestwood wrote:
---
src/iwd.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/iwd.h b/src/iwd.h
index 22223526..5371d8e5 100644
--- a/src/iwd.h
+++ b/src/iwd.h
@@ -22,6 +22,15 @@
#define uninitialized_var(x) x = x
+/*
+ * Set a maximum to prevent sending too much data to the kernel when
hashing
+ * the passphrase (or any other crypto operations involving the
passphrase).
+ * This will also prevent potential stack overflows if the
passphrase is put
+ * into EAP packets on the stack (EAP-GTC). This value is not tied
to IEEE or
+ * any RFC's, just chosen to be long enough to not restrict a normal
user.
+ */
+#define IWD_MAX_PASSPHRASE_LEN 2048
Tim pointed out this should really be called IWD_MAX_PASSWORD_LEN since
'PASSPHRASE' implies a WPA passphrase which does have a defined maximum
length of 63 characters. This value is only meant for EAP method
passwords that have no defined maximum.
Will change in v2 if everything else looks fine.
+
struct l_genl;
struct l_genl_family;
5:20 a.m.
Hi James,
> +#define IWD_MAX_PASSPHRASE_LEN 2048
Tim pointed out this should really be called IWD_MAX_PASSWORD_LEN since
'PASSPHRASE' implies a WPA passphrase which does have a defined maximum
length of 63 characters. This value is only meant for EAP method
passwords that have no defined maximum.
Yes, password would be better.
Regards,
-Denis
926
days inactive
926
days old
10 comments
2 participants
participants (2)
-
Denis Kenzior -
James Prestwood