[PATCH 1/4] util: add bounds check to util_get_{domain,username}

[PATCH 1/4] util: add bounds check to util_get_{domain,username}

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

---
 src/util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util.c b/src/util.c
index f787ce6b..f1860a33 100644
--- a/src/util.c
+++ b/src/util.c
@@ -176,7 +176,7 @@ const char *util_get_domain(const char *identity)
 			strncpy(domain, identity, c - identity);
 			return domain;
 		case '@':
-			strcpy(domain, c + 1);
+			strncpy(domain, c + 1, sizeof(domain));
 			return domain;
 		default:
 			continue;
@@ -197,7 +197,7 @@ const char *util_get_username(const char *identity)
 	for (c = identity; *c; c++) {
 		switch (*c) {
 		case '\\':
-			strcpy(username, c + 1);
+			strncpy(username, c + 1, sizeof(username));
 			return username;
 		case '@':
 			strncpy(username, identity, c - identity);
-- 
2.17.1

[PATCH 2/4] eapol: reorder eapol_sm_free

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

This was reported by coverity as a use after free. Technically it is
correct, though this will not pose a problem since l_queue_remove will
not actually dereference the pointer. Reordering this should fix the
warning and not change behavior at all.
---
 src/eapol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/eapol.c b/src/eapol.c
index 6732b370..cbaf0f4f 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -886,9 +886,9 @@ struct eapol_sm *eapol_sm_new(struct handshake_state *hs)
 
 void eapol_sm_free(struct eapol_sm *sm)
 {
-	eapol_sm_destroy(sm);
-
 	l_queue_remove(state_machines, sm);
+
+	eapol_sm_destroy(sm);
 }
 
 void eapol_sm_set_listen_interval(struct eapol_sm *sm, uint16_t interval)
-- 
2.17.1

Re: [PATCH 2/4] eapol: reorder eapol_sm_free

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]

hi James,

On 10/16/19 6:43 PM, James Prestwood wrote:
> This was reported by coverity as a use after free. Technically it is
> correct, though this will not pose a problem since l_queue_remove will
> not actually dereference the pointer. Reordering this should fix the
> warning and not change behavior at all.
> ---
>   src/eapol.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

[PATCH 3/4] eapol: check return of ie_parse_rsne_from_data

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

---
 src/eapol.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/eapol.c b/src/eapol.c
index cbaf0f4f..6f04dbc5 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -1106,7 +1106,8 @@ static void eapol_handle_ptk_1_of_4(struct eapol_sm *sm,
 	pmkid = handshake_util_find_pmkid_kde(EAPOL_KEY_DATA(ek, sm->mic_len),
 					EAPOL_KEY_DATA_LEN(ek, sm->mic_len));
 
-	ie_parse_rsne_from_data(own_ie, own_ie[1] + 2, &rsn_info);
+	if (ie_parse_rsne_from_data(own_ie, own_ie[1] + 2, &rsn_info) < 0)
+		goto error_unspecified;
 
 	/*
 	 * Require the PMKID KDE whenever we've sent a list of PMKIDs in
@@ -1557,7 +1558,8 @@ static void eapol_handle_ptk_3_of_4(struct eapol_sm *sm,
 		const uint8_t *mde = sm->handshake->mde;
 		const uint8_t *fte = sm->handshake->fte;
 
-		ie_parse_rsne_from_data(rsne, rsne[1] + 2, &ie_info);
+		if (ie_parse_rsne_from_data(rsne, rsne[1] + 2, &ie_info) < 0)
+			goto error_ie_different;
 
 		if (ie_info.num_pmkids != 1 || memcmp(ie_info.pmkids,
 						sm->handshake->pmk_r1_name, 16))
-- 
2.17.1

Re: [PATCH 3/4] eapol: check return of ie_parse_rsne_from_data

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

hi James,

On 10/16/19 6:43 PM, James Prestwood wrote:
> ---
>   src/eapol.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 

Neither of these could ever happen, but okay, lets be paranoid.

Applied, thanks.

Regards,
-Denis

[PATCH 4/4] ie: reorder ie_parse_osen to fix uninitialized value

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

RSNE_ADVANCE could result in a jump to the done label where info would
be copied without being initialized.
---
 src/ie.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/ie.c b/src/ie.c
index 4bc56589..ba5210c8 100644
--- a/src/ie.c
+++ b/src/ie.c
@@ -827,13 +827,13 @@ int ie_parse_osen(struct ie_tlv_iter *iter, struct ie_rsn_info *out_info)
 	if (!is_ie_wfa_ie(iter->data, iter->len, IE_WFA_OI_OSEN))
 		return -EPROTOTYPE;
 
-	RSNE_ADVANCE(data, len, 4);
-
 	memset(&info, 0, sizeof(info));
 	info.group_cipher = IE_RSN_CIPHER_SUITE_NO_GROUP_TRAFFIC;
 	info.pairwise_ciphers = IE_RSN_CIPHER_SUITE_CCMP;
 	info.akm_suites = IE_RSN_AKM_SUITE_8021X;
 
+	RSNE_ADVANCE(data, len, 4);
+
 	if (parse_ciphers(data, len, ie_parse_osen_akm_suite, &info) < 0)
 		return -EBADMSG;
 
-- 
2.17.1

Re: [PATCH 4/4] ie: reorder ie_parse_osen to fix uninitialized value

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

Hi James,

On 10/16/19 6:43 PM, James Prestwood wrote:
> RSNE_ADVANCE could result in a jump to the done label where info would
> be copied without being initialized.
> ---
>   src/ie.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

applied, thanks.

Regards,
-Denis

Re: [PATCH 1/4] util: add bounds check to util_get_{domain,username}

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

Hi James,

On 10/16/19 6:43 PM, James Prestwood wrote:
> ---
>   src/util.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util.c b/src/util.c
> index f787ce6b..f1860a33 100644
> --- a/src/util.c
> +++ b/src/util.c
> @@ -176,7 +176,7 @@ const char *util_get_domain(const char *identity)
>   			strncpy(domain, identity, c - identity);
>   			return domain;
>   		case '@':
> -			strcpy(domain, c + 1);
> +			strncpy(domain, c + 1, sizeof(domain));

Ah but the input is guaranteed to be less than 256 bytes anyway.  But in 
general, using strncpy isn't safe like that either as the target won't 
be null terminated in some cases.  Use l_strlcpy instead.

>   			return domain;
>   		default:
>   			continue;

Regards,
-Denis