anqp: fix potential NULL pointer dereference

PATCH In review James Prestwood 2 participants 12 messages Oct 16, 2019

Share & tools

James Prestwood

author

2019-10-16 22:29 UTC #0 lore raw

PATCH 1/10 anqp: fix potential NULL pointer dereference

---
 src/anqp.c | 3 +++
 1 file changed, 3 insertions(+)

src/anqp.c +3 -0

index a47530ab..5e867656 100644
@@ -287,6 +287,9 @@ uint32_t anqp_request(uint32_t ifindex, const uint8_t *addr,
 	uint32_t duration = 300;
 	struct netdev *netdev = netdev_find(ifindex);
 
+	if (!netdev)
+		return 0;
+
 	/*
 	 * TODO: Netdev dependencies will eventually be removed so we need
 	 * another way to figure out wiphy capabilities.

Signature · 1 line

2.17.1

James Prestwood

author

2019-10-16 22:29 UTC #1 lore raw

PATCH 2/10 erp: check return of hkdf_expand

---
 src/erp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

src/erp.c +3 -2

index 22938c32..2884eb83 100644
@@ -509,8 +509,9 @@ int erp_rx_packet(struct erp_state *erp, const uint8_t *pkt, size_t len)
 	l_put_be16(64, ptr);
 	ptr += 2;
 
-	hkdf_expand(L_CHECKSUM_SHA256, erp->r_rk, erp->cache->emsk_len,
-			info, ptr - info, erp->rmsk, erp->cache->emsk_len);
+	if (!hkdf_expand(L_CHECKSUM_SHA256, erp->r_rk, erp->cache->emsk_len,
+			info, ptr - info, erp->rmsk, erp->cache->emsk_len))
+		goto eap_failed;
 
 	return 0;

Signature · 1 line

2.17.1

James Prestwood

author

2019-10-16 22:29 UTC #2 lore raw

PATCH 3/10 eap-pwd: fix potential memory leak

In reply to James Prestwood

---
 src/eap-pwd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

src/eap-pwd.c +3 -1

index e70fc37c..3aa16222 100644
@@ -115,8 +115,10 @@ static bool kdf(uint8_t *key, size_t key_len, const char *label,
 		iov[iov_pos].iov_base = &L;
 		iov[iov_pos++].iov_len = 2;
 
-		if (!l_checksum_updatev(hmac, iov, iov_pos))
+		if (!l_checksum_updatev(hmac, iov, iov_pos)) {
+			l_checksum_free(hmac);
 			return false;
+		}
 
 		l_checksum_get_digest(hmac, out + len, minsize(olen - len, 32));
 		l_checksum_free(hmac);

Signature · 1 line

2.17.1

James Prestwood

author

2019-10-16 22:29 UTC #3 lore raw

PATCH 4/10 crypto: fix potential memory leak

In reply to James Prestwood

---
 src/crypto.c | 1 +
 1 file changed, 1 insertion(+)

src/crypto.c +1 -0

index 62edd447..638098a9 100644
@@ -706,6 +706,7 @@ bool hkdf_extract(enum l_checksum_type type, const uint8_t *key,
 	}
 
 	if (!l_checksum_updatev(hmac, iov, num_args)) {
+		l_checksum_free(hmac);
 		va_end(va);
 		return false;
 	}

Signature · 1 line

2.17.1

James Prestwood

author

2019-10-16 22:29 UTC #4 lore raw

PATCH 5/10 hotspot: fix multiple potential memory leaks

In reply to James Prestwood

---
 src/hotspot.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

src/hotspot.c +9 -1

index 20ebb2fd..1a783958 100644
@@ -342,7 +342,11 @@ static struct hs20_config *hs20_config_new(struct l_settings *settings,
 	config = l_new(struct hs20_config, 1);
 
 	if (hessid_str) {
-		util_string_to_address(hessid_str, config->hessid);
+		if (!util_string_to_address(hessid_str, config->hessid)) {
+			l_error("Invalid HESSID in settings");
+			memset(config->hessid, 0, 6);
+		}
+
 		l_free(hessid_str);
 	}

@@ -423,6 +427,8 @@ static void hs20_dir_watch_cb(const char *filename,
 
 		l_queue_push_head(hs20_settings, config);
 
+		l_settings_free(new);
+
 		break;
 	case L_DIR_WATCH_EVENT_REMOVED:
 		config = l_queue_remove_if(hs20_settings, match_filename,

@@ -453,6 +459,8 @@ static void hs20_dir_watch_cb(const char *filename,
 
 		known_network_update(&config->super, new, connected_time);
 
+		l_settings_free(new);
+
 		break;
 	case L_DIR_WATCH_EVENT_ACCESSED:
 		break;

Signature · 1 line

2.17.1

Denis Kenzior

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

Re: [PATCH 05/10] hotspot: fix multiple potential memory leaks

Hi James,

Quoted 9 lines · James Prestwood

---
  src/hotspot.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)

src/hotspot.c +0 -0

index 20ebb2fd..1a783958 100644

src/hotspot.c static struct hs20_config *hs20_config_new(struct l_settings *settings, · line 347

Quoted 7 lines

src/hotspot.c +3 -1

@@ -342,7 +342,11 @@ static struct hs20_config *hs20_config_new(struct l_settings *settings,
  	config = l_new(struct hs20_config, 1);
 
  	if (hessid_str) {
-		util_string_to_address(hessid_str, config->hessid);
+		if (!util_string_to_address(hessid_str, config->hessid)) {
+			l_error("Invalid HESSID in settings");
+			memset(config->hessid, 0, 6);

Why the memset? Also, shouldn't we fail here? Or at the very least delay checking the presence of hessid || nai_realms || rc_str until after the parsing has been validated?

src/hotspot.c static struct hs20_config *hs20_config_new(struct l_settings *settings, · line 352

Quoted 5 lines

src/hotspot.c +2 -0

+		}
+
  		l_free(hessid_str);
  	}

Also this seems to be unrelated to the patch description?

Regards,
-Denis

James Prestwood

author

2019-10-16 22:29 UTC #6 lore raw

PATCH 6/10 ie: fix uninitialized rx/tx_nss values

In reply to James Prestwood

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

src/ie.c +2 -2

index e90b5cfa..4bc56589 100644
@@ -1912,9 +1912,9 @@ static int ie_parse_vht_capability(struct ie_tlv_iter *vht_iter,
 	uint8_t rx_mcs_map[2];
 	uint8_t tx_mcs_map[2];
 	unsigned int max_rx_mcs = 0;
-	unsigned int rx_nss;
+	unsigned int rx_nss = 1;
 	unsigned int max_tx_mcs = 0;
-	unsigned int tx_nss;
+	unsigned int tx_nss = 1;
 	uint8_t ht_cap;
 	bool short_gi_20mhz;
 	bool short_gi_40mhz;

Signature · 1 line

2.17.1

James Prestwood

author

2019-10-16 22:29 UTC #7 lore raw

PATCH 7/10 owe: check for error return getting shared_secret

In reply to James Prestwood

---
 src/owe.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

src/owe.c +3 -1

index daaf1f30..9af3a0de 100644
@@ -165,9 +165,11 @@ static bool owe_compute_keys(struct owe_sm *owe, const void *public_key,
 	l_ecc_point_free(other_public);
 
 	nbytes = l_ecc_scalar_get_data(shared_secret, ss_buf, sizeof(ss_buf));
-
 	l_ecc_scalar_free(shared_secret);
 
+	if (nbytes < 0)
+		return false;
+
 	ptr += l_ecc_point_get_x(owe->public_key, ptr, sizeof(key));
 	memcpy(ptr, public_key, nbytes);
 	ptr += nbytes;

Signature · 1 line

2.17.1

James Prestwood

author

2019-10-16 22:29 UTC #8 lore raw

PATCH 8/10 owe: fix potential double free on error

In reply to James Prestwood

---
 src/owe.c | 1 -
 1 file changed, 1 deletion(-)

src/owe.c +0 -1

index 9af3a0de..9fff986f 100644
@@ -217,7 +217,6 @@ static bool owe_compute_keys(struct owe_sm *owe, const void *public_key,
 
 failed:
 	memset(ss_buf, 0, sizeof(ss_buf));
-	l_ecc_scalar_free(shared_secret);
 	return false;
 }

Signature · 1 line

2.17.1

James Prestwood

author

2019-10-16 22:29 UTC #9 lore raw

PATCH 9/10 owe: fix potential uninitialized variable

In reply to James Prestwood

---
 src/owe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

src/owe.c +1 -1

index 9fff986f..afdd32ea 100644
@@ -246,7 +246,7 @@ static int owe_rx_associate(struct auth_proto *ap, const uint8_t *frame,
 	size_t owe_dh_len = 0;
 	const uint8_t *owe_dh = NULL;
 	struct ie_rsn_info info;
-	bool akm_found;
+	bool akm_found = false;
 	const void *data;
 
 	mpdu = mpdu_validate(frame, len);

Signature · 1 line

2.17.1

James Prestwood

author

2019-10-16 22:29 UTC #10 lore raw

PATCH 10/10 sae: check return getting k_point

In reply to James Prestwood

---
 src/sae.c | 3 +++
 1 file changed, 3 insertions(+)

src/sae.c +3 -0

index 8f9425f1..232a63b9 100644
@@ -498,6 +498,9 @@ static int sae_process_commit(struct sae_sm *sm, const uint8_t *from,
 
 	l_ecc_point_free(k_point);
 
+	if (klen < 0)
+		goto reject;
+
 	/* keyseed = H(<0>32, k) */
 	hmac_sha256(zero_key, 32, k, klen, keyseed, 32);

Signature · 1 line

2.17.1

Denis Kenzior

2019-10-16 23:11 UTC #11 lore raw

In reply to James Prestwood

Hi James,

Quoted 4 lines · James Prestwood

---
  src/anqp.c | 3 +++
  1 file changed, 3 insertions(+)

All except patch 5 applied now.

Regards,
-Denis