util: add bounds check to util_get_{domain,username}

PATCH Applied James Prestwood 2 participants 8 messages Oct 16, 2019 – Oct 17, 2019

Share & tools

James Prestwood

author

2019-10-16 23:43 UTC #0 lore raw

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

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

src/util.c +2 -2

index f787ce6b..f1860a33 100644
@@ -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);

Signature · 1 line

2.17.1

James Prestwood

author

2019-10-16 23:43 UTC #1 lore raw

PATCH 2/4 eapol: reorder eapol_sm_free

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(-)

src/eapol.c +2 -2

index 6732b370..cbaf0f4f 100644
@@ -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)

Signature · 1 line

2.17.1

Denis Kenzior

Applied

2019-10-17 02:18 UTC #2 lore raw

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

hi James,

Quoted 8 lines · James Prestwood

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

James Prestwood

author

2019-10-16 23:43 UTC #3 lore raw

PATCH 3/4 eapol: check return of ie_parse_rsne_from_data

In reply to James Prestwood

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

src/eapol.c +4 -2

index cbaf0f4f..6f04dbc5 100644
@@ -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))

Signature · 1 line

2.17.1

Denis Kenzior

Applied

2019-10-17 02:25 UTC #4 lore raw

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

hi James,

Quoted 4 lines · James Prestwood

---
  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

James Prestwood

author

2019-10-16 23:43 UTC #5 lore raw

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

In reply to James Prestwood

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(-)

src/ie.c +2 -2

index 4bc56589..ba5210c8 100644
@@ -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;

Signature · 1 line

2.17.1

Denis Kenzior

Applied

2019-10-17 02:25 UTC #6 lore raw

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

Hi James,

Quoted 5 lines · James Prestwood

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

Denis Kenzior

2019-10-17 02:15 UTC #7 lore raw

In reply to James Prestwood

Hi James,

Quoted 9 lines · James Prestwood

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

src/util.c +0 -0

index f787ce6b..f1860a33 100644

src/util.c const char *util_get_domain(const char *identity) · line 179

Quoted 5 lines

src/util.c +1 -1

@@ -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.

src/util.c const char *util_get_domain(const char *identity) · line 182

Quoted 3 lines

src/util.c +0 -0

  			return domain;
  		default:
  			continue;

Regards,
-Denis