diff mbox

WPS: fix nonce comparisons

Message ID 1344821173-30483-1-git-send-email-eyal@wizery.com
State Accepted
Commit b4e9e2659b97865f7950ae467a3d828b768c78de
Headers show

Commit Message

Eyal Shapira Aug. 13, 2012, 1:26 a.m. UTC 
Multiple memcmps of nonces were actually comparing
only the first byte instead of all 16 bytes.

Signed-hostap: Eyal Shapira <eyal@wizery.com>
---
 src/wps/wps_enrollee.c  |   10 +++++-----
 src/wps/wps_registrar.c |   14 +++++++-------
 2 files changed, 12 insertions(+), 12 deletions(-)

Comments

Jouni Malinen Aug. 13, 2012, 5:04 p.m. UTC | #1 
On Mon, Aug 13, 2012 at 04:26:13AM +0300, Eyal Shapira wrote:
> Multiple memcmps of nonces were actually comparing
> only the first byte instead of all 16 bytes.

Thanks, applied.
Baruch Siach Aug. 14, 2012, 5:48 a.m. UTC | #2 
Hi Eyal,

On Mon, Aug 13, 2012 at 04:26:13AM +0300, Eyal Shapira wrote:
> Multiple memcmps of nonces were actually comparing
> only the first byte instead of all 16 bytes.

Looks like a serious security bug.

Do you know what the security implications of this bug are? What versions of 
hostap are affected? What configurations? Is it WPS specific?

baruch

> Signed-hostap: Eyal Shapira <eyal@wizery.com>
> ---
Jouni Malinen Aug. 14, 2012, 4:19 p.m. UTC | #3 
On Tue, Aug 14, 2012 at 08:48:11AM +0300, Baruch Siach wrote:
> On Mon, Aug 13, 2012 at 04:26:13AM +0300, Eyal Shapira wrote:
> > Multiple memcmps of nonces were actually comparing
> > only the first byte instead of all 16 bytes.
> 
> Looks like a serious security bug.

Not really. Obviously, it is not good to have that type of memcmp
issues, but the particular ones here were mainly copy-pasted sanity
checks that are of limited significance.

> Do you know what the security implications of this bug are? What versions of 
> hostap are affected? What configurations? Is it WPS specific?

This is specific to WPS and the incorrect code was there from the
initial implementation. So far, I have not found any real security
implication from this.

The main authentication of WPS messages uses a separate mechanism and it
was not affected. In addition, the nonces were validated in correct way
in another function for the M2-M8 messages (i.e., throughout the main
WPS protocol run) and the nonces do not have significant importance for
the couple of messages (ACK,NACK,DONE) were there was no duplicated
validation code.

As far as WPS Enrollee behavior is concerned, the validation of Enrollee
nonce in the beginning of wps_process_wsc_msg() was done also in
wps_process_m{2,4,6,8}(), so for those cases, this was a duplicated
check and there was no practical difference in behavior.
wps_process_m2d() does not have this check, but it won't allow the
protocol to continue anyway, so I don't see any real issue with this
either. Similarly, wps_process_wsc_nack() terminates the protocol, so a
wrong nonce there is of limited significance. wps_process_wsc_ack() can
be on a success path, but nonces don't really play much in that message
either.

Similarly for WPS Registrar, the validation of Registrar nonce in
wps_process_wsc_msg() was also done in wps_process_m{3,5,7}(), so there
was no difference in behavior. wps_process_wsc_{ack,nack,done}() cases
are similar to ack/nack in Enrollee.
diff mbox

Patch

diff --git a/src/wps/wps_enrollee.c b/src/wps/wps_enrollee.c
index da0c101..389aa84 100644
--- a/src/wps/wps_enrollee.c
+++ b/src/wps/wps_enrollee.c
@@ -1150,7 +1150,7 @@  static enum wps_process_res wps_process_wsc_msg(struct wps_data *wps,
 		return WPS_FAILURE;
 
 	if (attr.enrollee_nonce == NULL ||
-	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN != 0)) {
+	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN) != 0) {
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in enrollee nonce");
 		return WPS_FAILURE;
 	}
@@ -1242,14 +1242,14 @@  static enum wps_process_res wps_process_wsc_ack(struct wps_data *wps,
 	}
 
 	if (attr.registrar_nonce == NULL ||
-	    os_memcmp(wps->nonce_r, attr.registrar_nonce, WPS_NONCE_LEN != 0))
+	    os_memcmp(wps->nonce_r, attr.registrar_nonce, WPS_NONCE_LEN) != 0)
 	{
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in registrar nonce");
 		return WPS_FAILURE;
 	}
 
 	if (attr.enrollee_nonce == NULL ||
-	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN != 0)) {
+	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN) != 0) {
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in enrollee nonce");
 		return WPS_FAILURE;
 	}
@@ -1289,7 +1289,7 @@  static enum wps_process_res wps_process_wsc_nack(struct wps_data *wps,
 	}
 
 	if (attr.registrar_nonce == NULL ||
-	    os_memcmp(wps->nonce_r, attr.registrar_nonce, WPS_NONCE_LEN != 0))
+	    os_memcmp(wps->nonce_r, attr.registrar_nonce, WPS_NONCE_LEN) != 0)
 	{
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in registrar nonce");
 		wpa_hexdump(MSG_DEBUG, "WPS: Received Registrar Nonce",
@@ -1300,7 +1300,7 @@  static enum wps_process_res wps_process_wsc_nack(struct wps_data *wps,
 	}
 
 	if (attr.enrollee_nonce == NULL ||
-	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN != 0)) {
+	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN) != 0) {
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in enrollee nonce");
 		wpa_hexdump(MSG_DEBUG, "WPS: Received Enrollee Nonce",
 			    attr.enrollee_nonce, WPS_NONCE_LEN);
diff --git a/src/wps/wps_registrar.c b/src/wps/wps_registrar.c
index 154c2b4..2d0b545 100644
--- a/src/wps/wps_registrar.c
+++ b/src/wps/wps_registrar.c
@@ -2849,7 +2849,7 @@  static enum wps_process_res wps_process_wsc_msg(struct wps_data *wps,
 	if (*attr.msg_type != WPS_M1 &&
 	    (attr.registrar_nonce == NULL ||
 	     os_memcmp(wps->nonce_r, attr.registrar_nonce,
-		       WPS_NONCE_LEN != 0))) {
+		       WPS_NONCE_LEN) != 0)) {
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in registrar nonce");
 		return WPS_FAILURE;
 	}
@@ -2945,14 +2945,14 @@  static enum wps_process_res wps_process_wsc_ack(struct wps_data *wps,
 #endif /* CONFIG_WPS_UPNP */
 
 	if (attr.registrar_nonce == NULL ||
-	    os_memcmp(wps->nonce_r, attr.registrar_nonce, WPS_NONCE_LEN != 0))
+	    os_memcmp(wps->nonce_r, attr.registrar_nonce, WPS_NONCE_LEN) != 0)
 	{
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in registrar nonce");
 		return WPS_FAILURE;
 	}
 
 	if (attr.enrollee_nonce == NULL ||
-	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN != 0)) {
+	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN) != 0) {
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in enrollee nonce");
 		return WPS_FAILURE;
 	}
@@ -3014,14 +3014,14 @@  static enum wps_process_res wps_process_wsc_nack(struct wps_data *wps,
 #endif /* CONFIG_WPS_UPNP */
 
 	if (attr.registrar_nonce == NULL ||
-	    os_memcmp(wps->nonce_r, attr.registrar_nonce, WPS_NONCE_LEN != 0))
+	    os_memcmp(wps->nonce_r, attr.registrar_nonce, WPS_NONCE_LEN) != 0)
 	{
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in registrar nonce");
 		return WPS_FAILURE;
 	}
 
 	if (attr.enrollee_nonce == NULL ||
-	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN != 0)) {
+	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN) != 0) {
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in enrollee nonce");
 		return WPS_FAILURE;
 	}
@@ -3100,14 +3100,14 @@  static enum wps_process_res wps_process_wsc_done(struct wps_data *wps,
 #endif /* CONFIG_WPS_UPNP */
 
 	if (attr.registrar_nonce == NULL ||
-	    os_memcmp(wps->nonce_r, attr.registrar_nonce, WPS_NONCE_LEN != 0))
+	    os_memcmp(wps->nonce_r, attr.registrar_nonce, WPS_NONCE_LEN) != 0)
 	{
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in registrar nonce");
 		return WPS_FAILURE;
 	}
 
 	if (attr.enrollee_nonce == NULL ||
-	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN != 0)) {
+	    os_memcmp(wps->nonce_e, attr.enrollee_nonce, WPS_NONCE_LEN) != 0) {
 		wpa_printf(MSG_DEBUG, "WPS: Mismatch in enrollee nonce");
 		return WPS_FAILURE;
 	}