Vincent Chang[:vchang][changyihsin] Reporter
	Description • 12 years ago

If user enters wrong password in WEP mode, we still can associate to AP and send the data. However, we fail in do_dhcp_request() because the data is encrypted with wrong password.
We may continue to retry without any notifications to user. We should fire events to DOM in this case.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 1 • 12 years ago

Following is the corresponding source code of wpa_supllicant handling WEP key error.

==== wpa_supplicant.c : wpa_supplicant_associate() =====

if (wpa_s->use_client_mlme)
  ret = ieee80211_sta_associate(wpa_s, &params);
else
  ret = wpa_drv_associate(wpa_s, &params);

if (ret < 0) {
  wpa_msg(wpa_s, MSG_INFO, "Association request to the driver "
          "failed");
  /* try to continue anyway; new association will be tried again
     * after timeout */
  assoc_failed = 1;
}

if (wpa_s->key_mgmt == WPA_KEY_MGMT_WPA_NONE) {
  /* Set the key after the association just in case association
   * cleared the previously configured key. */
  wpa_supplicant_set_wpa_none_key(wpa_s, ssid);
  /* No need to timeout authentication since there is no key
   * management. */
  wpa_supplicant_cancel_auth_timeout(wpa_s);
  wpa_supplicant_set_state(wpa_s, WPA_COMPLETED);
} 

====================

Based on its code, wpa_supplicant just enter COMPLETE state after association, regardless of the association result.
So no event is fired while association failed.
Without the event, there is no way for gecko to know that WEP connection is failed.

I think we need to modify wpa_supplicant to handle the condition.
After a quick survey, I think change the code
=====
  wpa_supplicant_set_state(wpa_s, WPA_COMPLETED);
=====
to
=====
  wpa_supplicant_disassociate( wpa_s, REASON_PAIRWISE_CIPHER_NOT_VALID );
  return;
=====
will do the work.

Any comments?

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 2 • 12 years ago

Fix my previous code,
The change should be from
=====
  wpa_supplicant_set_state(wpa_s, WPA_COMPLETED);
=====
to
=====
if ( assoc_failed ) {
  wpa_supplicant_disassociate( wpa_s, REASON_PAIRWISE_CIPHER_NOT_VALID );
  return;
}
wpa_supplicant_set_state(wpa_s, WPA_COMPLETED);
======

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 3 • 12 years ago

There are two cases need to be cared of:

1. Wrong password:
   wpa_supplicant will send CTRL-EVENT-ASSOC-REJECT, which need to be handled as a disconnect event.
   manager.supplicantLoopDetection() also need to be modified because state transition of WEP wrong password is ASSOCIATING(5) -> DISCONNECTED(0), different from state transition of WPA: ASSOCIATING(5) -> ASSOCIATED(6) -> FOUR_WAY_HANDSHAKE(7) -> DISCONNECTED(0)

2. Wrong password length:
   Currently only observed when AP uses WEP-128bit.
   If use password of length 5-ASCIIs or 13-ASCIIs, it goes into wrong password procedure as 1.
   But password of other length will cause a driver error and send "Association request to the driver failed" in wpa_supplicant.

I will try not to modify wpa_supplicant and handle these conditions in wifi worker.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 4 • 12 years ago

1. Handle event strings from wpa_supplicant on WEP authentication fail.
2. Rewrite disconnect detection function to support WEP mode.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 5 • 12 years ago

Handle event strings from wpa_supplicant on WEP authentication fail

	Vincent Chang[:vchang][changyihsin] Reporter
	Comment 6 • 12 years ago

Comment on attachment 671751 [details] [diff] [review]
Handle WEP Authentication Fail (v2)

Review of attachment 671751 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/wifi/WifiWorker.js
@@ +721,5 @@
>      debug("Event coming in: " + event);
>      if (event.indexOf("CTRL-EVENT-") !== 0 && event.indexOf("WPS") !== 0) {
> +      // Handle connection fail exception on WEP-128, while password length
> +      // is not 5 nor 13 bytes
> +      if ( event.indexOf("Association request to the driver failed") !== -1 ) {

Nit: no space between '(' and event.indexOf, also -1 and ')'.

@@ +723,5 @@
> +      // Handle connection fail exception on WEP-128, while password length
> +      // is not 5 nor 13 bytes
> +      if ( event.indexOf("Association request to the driver failed") !== -1 ) {
> +        notify("disconnected");
> +        notifyStateChange({ state: "DISCONNECTED", BSSID: null, id: -1 });

Do we need to call notifyStateChang in this case ?

@@ +804,5 @@
>          manager.authenticationFailuresCount = 0;
>        }
>        return true;
>      }
> +    if ( eventData.indexOf("CTRL-EVENT-ASSOC-REJECT") === 0 ) {

Nit: no space between '(' and event.indexOf, also 0 and ')'.

@@ +806,5 @@
>        return true;
>      }
> +    if ( eventData.indexOf("CTRL-EVENT-ASSOC-REJECT") === 0 ) {
> +        notify("disconnected");
> +        notifyStateChange({ state: "DISCONNECTED", BSSID: null, id: -1 });

Do we need to call notifyStateChange here ?

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 7 • 12 years ago

Handle event strings from wpa_supplicant on WEP authentication fail

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 8 • 12 years ago

Handle event strings from wpa_supplicant on WEP authentication fail.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 9 • 12 years ago

Comment on attachment 674104 [details] [diff] [review]
Handle WEP Authentication Fail (v3)

Are you sure that the only way that we can get these errors is through WEP key failure? It seems like this should filter into authentificationFailuresCount either way, so that if the problem was actually on the router's end, we don't simply try once and give up.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 10 • 12 years ago

I am not sure on what situations we might also get these errors, maybe a driver malfunction, client is blocked by AP, AP overloaded, etc. I can only observe them on WEP key failure for now.
But no matter the cause is, I think these errors must be handled, or user will get no response. And showing "connection failed" seems to be a simple way inform user.

I will add retry as authentificationFailuresCount on these two errors first, and see if anything needs to be taken into considered.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 11 • 12 years ago

While testing retry on WEP key failure, I found the scan state of reconnect is affected by Bug 796461, same as Bug 804459, because wpa_supplicant will enter disconnect state while reconnecting.

I think it's better not to add retry before Bug 804459 is fixed.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 12 • 12 years ago

Comment on attachment 674104 [details] [diff] [review]
Handle WEP Authentication Fail (v3)

Review of attachment 674104 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand the relationship to bug 804459 and having a backoff count here? If we automatically disable the network after 5 or so tries instead of after 1, what difference does it make to the user?

::: dom/wifi/WifiWorker.js
@@ +805,5 @@
>        }
>        return true;
>      }
> +    // Association reject is triggered mostly on incorrect WEP key
> +    if (eventData.indexOf("CTRL-EVENT-ASSOC-REJECT") === 0) {

Debugging an unrelated issue today, I definitely saw an EVENT-ASSOC-REJECT (I think because the driver was in the middle of a scan request) immediately followed by an association to the AP. Therefore, I don't think it's right to do this. We should let the supplicant try at least a couple of times.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 13 • 12 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #12)
> Comment on attachment 674104 [details] [diff] [review]
> Handle WEP Authentication Fail (v3)
> 
> Review of attachment 674104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand the relationship to bug 804459 and having a backoff count
> here? If we automatically disable the network after 5 or so tries instead of
> after 1, what difference does it make to the user?
> 

The cause of bug 804459 is Gaia will trigger scan and refresh its AP list on connected, disconnected, and connectingfailed states. And the connection status will be flushed by this action.
In this case, when supplicant retries, the state goes into disconnected first, and scan is triggered.
Although supplicant is retrying in the background, the "connecting..." information is flushed, also the upcoming "connection failed" information.

> ::: dom/wifi/WifiWorker.js
> @@ +805,5 @@
> >        }
> >        return true;
> >      }
> > +    // Association reject is triggered mostly on incorrect WEP key
> > +    if (eventData.indexOf("CTRL-EVENT-ASSOC-REJECT") === 0) {
> 
> Debugging an unrelated issue today, I definitely saw an EVENT-ASSOC-REJECT
> (I think because the driver was in the middle of a scan request) immediately
> followed by an association to the AP. Therefore, I don't think it's right to
> do this. We should let the supplicant try at least a couple of times. 

I will allow supplicant retry the same time as WPA mode, but I am afraid it can not be observed on Gaia with re-scan mechanism mentioned in bug 804459

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 14 • 12 years ago

Allow retry on receiving connection failed messages that might caused by WEP key failure.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 15 • 12 years ago

Comment on attachment 678634 [details] [diff] [review]
Handle WEP Authentication Fail with Retry

Review of attachment 678634 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/wifi/WifiWorker.js
@@ +775,5 @@
> +      // Handle connection fail exception on WEP-128, while password length
> +      // is not 5 nor 13 bytes.
> +      if (event.indexOf("Association request to the driver failed") !== -1) {
> +        notify("passwordmaybeincorrect");
> +        if (manager.authenticationFailuresCount > MAX_RETRIES_ON_AUTHENTICATION_FAILURE) {

When we get these notifications, are we guaranteed to go back to DISCONNECTED again? If so, you can simplify this by waiting for that notification. If not, this would be good to go.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 16 • 12 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #15)
> Comment on attachment 678634 [details] [diff] [review]
> Handle WEP Authentication Fail with Retry
> 
> Review of attachment 678634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/WifiWorker.js
> @@ +775,5 @@
> > +      // Handle connection fail exception on WEP-128, while password length
> > +      // is not 5 nor 13 bytes.
> > +      if (event.indexOf("Association request to the driver failed") !== -1) {
> > +        notify("passwordmaybeincorrect");
> > +        if (manager.authenticationFailuresCount > MAX_RETRIES_ON_AUTHENTICATION_FAILURE) {
> 
> When we get these notifications, are we guaranteed to go back to
> DISCONNECTED again? If so, you can simplify this by waiting for that
> notification. If not, this would be good to go.

Not for "Association request to the driver failed", the state transits only between INVALID and SCANNING.
For "CTRL-EVENT-ASSOC-REJECT", the state do go to DISCONNECT, but WifiWorker receives CTRL-EVENT-STATE-CHANGE, not CTRL-EVENT-DISCONNECT. So it fires "ondisconnect", while we need "onconnectingfailed" here to indicate the key failure. I think it's simpler to handle REJECT directly than doing the counting in state change because there are many conditions make state change to DISCONNECT.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 17 • 12 years ago

Comment on attachment 678634 [details] [diff] [review]
Handle WEP Authentication Fail with Retry

Great, thanks for bearing with me here. Let's get this landed.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 18 • 12 years ago

try server result : https://tbpl.mozilla.org/?tree=Try&rev=e8b7967f44fa

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c25c93664add

	Ed Morley [:emorley]
	Comment 20 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/c25c93664add

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/200e0007111a