diff mbox series

eap-wsc: Silence static analysis

Message ID 20240227212521.1510693-1-denkenz@gmail.com (mailing list archive)
State New
Headers show
Series eap-wsc: Silence static analysis | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
prestwoj/iwd-alpine-ci-fetch success Fetch PR
prestwoj/iwd-ci-gitlint success GitLint
prestwoj/iwd-ci-fetch success Fetch PR
prestwoj/iwd-alpine-ci-makedistcheck success Make Distcheck
prestwoj/iwd-ci-makedistcheck success Make Distcheck
prestwoj/iwd-alpine-ci-incremental_build success Incremental build not run PASS
prestwoj/iwd-ci-incremental_build success Incremental build not run PASS
prestwoj/iwd-ci-build success Build - Configure
prestwoj/iwd-alpine-ci-build success Build - Configure
prestwoj/iwd-ci-makecheck success Make Check
prestwoj/iwd-ci-makecheckvalgrind success Make Check w/Valgrind
prestwoj/iwd-ci-clang success clang PASS
prestwoj/iwd-alpine-ci-makecheckvalgrind success Make Check w/Valgrind
prestwoj/iwd-alpine-ci-makecheck success Make Check
prestwoj/iwd-ci-testrunner success test-runner PASS

Commit Message

Denis Kenzior Feb. 27, 2024, 9:25 p.m. UTC 
static analysis complains that authenticator is used uninitialized.
This isn't strictly true as memory region is reserved for the
authenticator using the contents of the passed in structure.  This
region is then overwritten once the authenticator is actually computed
by authenticator_put().  Silence this warning by explicitly setting
authenticator bytes to 0.
---
 src/eap-wsc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Paul Menzel Feb. 28, 2024, 6:26 a.m. UTC | #1 
Dear Denis,


Thank you for your patch.

Am 27.02.24 um 22:25 schrieb Denis Kenzior:
> static analysis complains that authenticator is used uninitialized.
> This isn't strictly true as memory region is reserved for the
> authenticator using the contents of the passed in structure.  This
> region is then overwritten once the authenticator is actually computed
> by authenticator_put().  Silence this warning by explicitly setting
> authenticator bytes to 0.

Too bad. Out of curiosity. What static analyzer is this?

For the commit message summary, I would be more specific. Maybe:

Zero authenticator bytes to fix static analysis warning

> ---
>   src/eap-wsc.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/src/eap-wsc.c b/src/eap-wsc.c
> index 65a97da4ee5e..789a20f032b9 100644
> --- a/src/eap-wsc.c
> +++ b/src/eap-wsc.c
> @@ -695,6 +695,8 @@ static void eap_wsc_send_m5(struct eap_state *eap,
>   	size_t encrypted_len;
>   	bool r;
>   
> +	memset(m5.authenticator, 0, sizeof(m5.authenticator));
> +
>   	memcpy(m5es.e_snonce1, wsc->local_snonce1, sizeof(wsc->local_snonce1));
>   	pdu = wsc_build_m5_encrypted_settings(&m5es, &pdu_len);
>   	explicit_bzero(m5es.e_snonce1, sizeof(wsc->local_snonce1));
> @@ -797,6 +799,8 @@ static void eap_wsc_send_m3(struct eap_state *eap,
>   	uint8_t *pdu;
>   	size_t pdu_len;
>   
> +	memset(m3.authenticator, 0, sizeof(m3.authenticator));
> +
>   	len = strlen(wsc->device_password);
>   
>   	/* WSC 2.0.5, Section 7.4:
> @@ -975,6 +979,8 @@ static void eap_wsc_r_send_m8(struct eap_state *eap,
>   	unsigned int auth_types =
>   		wsc->m1->auth_type_flags & wsc->m2->auth_type_flags;
>   
> +	memset(m8.authenticator, 0, sizeof(m8.authenticator));
> +
>   	if (auth_types & WSC_AUTHENTICATION_TYPE_OPEN)
>   		memcpy(&creds[creds_cnt++], &wsc->open_cred,
>   			sizeof(struct wsc_credential));
> @@ -1022,6 +1028,9 @@ static void eap_wsc_r_handle_m7(struct eap_state *eap,
>   	struct wsc_m7_encrypted_settings m7es;
>   	enum wsc_configuration_error error = WSC_CONFIGURATION_ERROR_NO_ERROR;
>   
> +	memset(m7es.authenticator, 0, sizeof(m7es.authenticator));
> +	memset(m7.authenticator, 0, sizeof(m7.authenticator));
> +
>   	/* Spec unclear what to do here, see comments in eap_wsc_send_nack */
>   	if (wsc_parse_m7(pdu, len, &m7, &encrypted) != 0)
>   		goto send_nack;
> @@ -1084,6 +1093,9 @@ static void eap_wsc_r_send_m6(struct eap_state *eap,
>   	size_t encrypted_len;
>   	bool r;
>   
> +	memset(m6es.authenticator, 0, sizeof(m6es.authenticator));
> +	memset(m6.authenticator, 0, sizeof(m6.authenticator));
> +
>   	memcpy(m6es.r_snonce2, wsc->local_snonce2, sizeof(wsc->local_snonce2));
>   	pdu = wsc_build_m6_encrypted_settings(&m6es, &pdu_len);
>   	explicit_bzero(m6es.r_snonce2, sizeof(wsc->local_snonce2));
> @@ -1123,6 +1135,8 @@ static void eap_wsc_r_handle_m5(struct eap_state *eap,
>   	struct wsc_m5_encrypted_settings m5es;
>   	enum wsc_configuration_error error = WSC_CONFIGURATION_ERROR_NO_ERROR;
>   
> +	memset(m5es.authenticator, 0, sizeof(m5es.authenticator));
> +
>   	/* Spec unclear what to do here, see comments in eap_wsc_send_nack */
>   	if (wsc_parse_m5(pdu, len, &m5, &encrypted) != 0)
>   		goto send_nack;
> @@ -1188,6 +1202,9 @@ static void eap_wsc_r_send_m4(struct eap_state *eap,
>   	size_t len_half1;
>   	struct iovec iov[4];
>   
> +	memset(m4es.authenticator, 0, sizeof(m4es.authenticator));
> +	memset(m4.authenticator, 0, sizeof(m4.authenticator));
> +
>   	memcpy(m4es.r_snonce1, wsc->local_snonce1, sizeof(wsc->local_snonce1));
>   	pdu = wsc_build_m4_encrypted_settings(&m4es, &pdu_len);
>   	explicit_bzero(m4es.r_snonce1, sizeof(wsc->local_snonce1));

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul
Denis Kenzior Feb. 28, 2024, 3:14 p.m. UTC | #2 
Hi Paul,

> 
> Too bad. Out of curiosity. What static analyzer is this?

In this particular case: coverity.  It is very easily availabile for open source 
projects and does a pretty good job.

> 
> For the commit message summary, I would be more specific. Maybe:
> 
> Zero authenticator bytes to fix static analysis warning

Great suggestion!  Done.

> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

Thanks for the review.  Added your Reviewed-by to the commit and pushed out.

Regards,
-Denis
diff mbox series

Patch

diff --git a/src/eap-wsc.c b/src/eap-wsc.c
index 65a97da4ee5e..789a20f032b9 100644
--- a/src/eap-wsc.c
+++ b/src/eap-wsc.c
@@ -695,6 +695,8 @@  static void eap_wsc_send_m5(struct eap_state *eap,
 	size_t encrypted_len;
 	bool r;
 
+	memset(m5.authenticator, 0, sizeof(m5.authenticator));
+
 	memcpy(m5es.e_snonce1, wsc->local_snonce1, sizeof(wsc->local_snonce1));
 	pdu = wsc_build_m5_encrypted_settings(&m5es, &pdu_len);
 	explicit_bzero(m5es.e_snonce1, sizeof(wsc->local_snonce1));
@@ -797,6 +799,8 @@  static void eap_wsc_send_m3(struct eap_state *eap,
 	uint8_t *pdu;
 	size_t pdu_len;
 
+	memset(m3.authenticator, 0, sizeof(m3.authenticator));
+
 	len = strlen(wsc->device_password);
 
 	/* WSC 2.0.5, Section 7.4:
@@ -975,6 +979,8 @@  static void eap_wsc_r_send_m8(struct eap_state *eap,
 	unsigned int auth_types =
 		wsc->m1->auth_type_flags & wsc->m2->auth_type_flags;
 
+	memset(m8.authenticator, 0, sizeof(m8.authenticator));
+
 	if (auth_types & WSC_AUTHENTICATION_TYPE_OPEN)
 		memcpy(&creds[creds_cnt++], &wsc->open_cred,
 			sizeof(struct wsc_credential));
@@ -1022,6 +1028,9 @@  static void eap_wsc_r_handle_m7(struct eap_state *eap,
 	struct wsc_m7_encrypted_settings m7es;
 	enum wsc_configuration_error error = WSC_CONFIGURATION_ERROR_NO_ERROR;
 
+	memset(m7es.authenticator, 0, sizeof(m7es.authenticator));
+	memset(m7.authenticator, 0, sizeof(m7.authenticator));
+
 	/* Spec unclear what to do here, see comments in eap_wsc_send_nack */
 	if (wsc_parse_m7(pdu, len, &m7, &encrypted) != 0)
 		goto send_nack;
@@ -1084,6 +1093,9 @@  static void eap_wsc_r_send_m6(struct eap_state *eap,
 	size_t encrypted_len;
 	bool r;
 
+	memset(m6es.authenticator, 0, sizeof(m6es.authenticator));
+	memset(m6.authenticator, 0, sizeof(m6.authenticator));
+
 	memcpy(m6es.r_snonce2, wsc->local_snonce2, sizeof(wsc->local_snonce2));
 	pdu = wsc_build_m6_encrypted_settings(&m6es, &pdu_len);
 	explicit_bzero(m6es.r_snonce2, sizeof(wsc->local_snonce2));
@@ -1123,6 +1135,8 @@  static void eap_wsc_r_handle_m5(struct eap_state *eap,
 	struct wsc_m5_encrypted_settings m5es;
 	enum wsc_configuration_error error = WSC_CONFIGURATION_ERROR_NO_ERROR;
 
+	memset(m5es.authenticator, 0, sizeof(m5es.authenticator));
+
 	/* Spec unclear what to do here, see comments in eap_wsc_send_nack */
 	if (wsc_parse_m5(pdu, len, &m5, &encrypted) != 0)
 		goto send_nack;
@@ -1188,6 +1202,9 @@  static void eap_wsc_r_send_m4(struct eap_state *eap,
 	size_t len_half1;
 	struct iovec iov[4];
 
+	memset(m4es.authenticator, 0, sizeof(m4es.authenticator));
+	memset(m4.authenticator, 0, sizeof(m4.authenticator));
+
 	memcpy(m4es.r_snonce1, wsc->local_snonce1, sizeof(wsc->local_snonce1));
 	pdu = wsc_build_m4_encrypted_settings(&m4es, &pdu_len);
 	explicit_bzero(m4es.r_snonce1, sizeof(wsc->local_snonce1));