Closed Bug 786438 Opened 8 years ago Closed 7 years ago

Wifi: fire events to DOM when entering wrong password for WEP

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: vchang, Assigned: chucklee)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → chulee
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?
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);
======
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.
Attached patch Handle WEP Authentication Fail (obsolete) — Splinter Review
1. Handle event strings from wpa_supplicant on WEP authentication fail.
2. Rewrite disconnect detection function to support WEP mode.
Attachment #671339 - Flags: review?(mrbkap)
Handle event strings from wpa_supplicant on WEP authentication fail
Attachment #671339 - Attachment is obsolete: true
Attachment #671339 - Flags: review?(mrbkap)
Attachment #671751 - Flags: review?(mrbkap)
Attachment #671751 - Flags: review?(mrbkap) → review?(vchang)
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 ?
Handle event strings from wpa_supplicant on WEP authentication fail
Attachment #671751 - Attachment is obsolete: true
Attachment #671751 - Flags: review?(vchang)
Attachment #674103 - Flags: review?(mrbkap)
Handle event strings from wpa_supplicant on WEP authentication fail.
Attachment #674103 - Attachment is obsolete: true
Attachment #674103 - Flags: review?(mrbkap)
Attachment #674104 - Flags: review?(mrbkap)
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.
Attachment #674104 - Flags: review?(mrbkap)
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.
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.
Flags: needinfo?(mrbkap)
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.
Attachment #674104 - Flags: review-
Flags: needinfo?(mrbkap)
(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
Allow retry on receiving connection failed messages that might caused by WEP key failure.
Attachment #674104 - Attachment is obsolete: true
Attachment #678634 - Flags: review?(mrbkap)
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.
Attachment #678634 - Flags: review?(mrbkap)
(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.
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.
Attachment #678634 - Flags: review+
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
https://hg.mozilla.org/mozilla-central/rev/c25c93664add
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.