813880 - unable to disable network when user connects to wep network with incorrect password.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Vincent Chang[:vchang][changyihsin]]

Currently, we set the netId to currentNetwork when we associate to the AP. However, if we connect to wep network and enter incorrect password, we will not move to associated state. We need netId when we try to disable or remove the network from wpa_supplicant.conf.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0

This patch tries to get the netId and use it to disable network with incorrect password. There are two cases when password is incorrect, 
1. user enters incorrect password and length of password is correct(5 or 13 characters). 
  In this case, we receive CTRL-EVENT-ASSOC-REJECT and CTRL-EVENT-STATE-CHANGE to disconnected. We can get netId from this.id when control event state change to disconnected.  
2. user enters incorrect password and length of password is incorrect. 
  In this case, we receive driver error event, this event is fired when the password length is incorrect. We can avoid this by checking the length when we are going to associate a network. And it seems not be able to get the netId in this case.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 684651 [details] [diff] [review]
Patch v1.0

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

::: dom/wifi/WifiWorker.js
@@ +1396,5 @@
> +function getNetworkEncryption(network)
> +{
> +  var encryption = "OPEN";
> +
> +  if ("capabilities" in network) {

Please use this function in getNetworkKey. Copy/pasting code like this is really not good.

@@ +1913,5 @@
>          break;
>        case "DISCONNECTED":
>          self._fireEvent("ondisconnect", {});
> +        if (WifiManager.authenticationFailuresCount > MAX_RETRIES_ON_AUTHENTICATION_FAILURE) {
> +          if (this.id && typeof this.id !== "undefined") {

No need for the second test given the first.

@@ +2650,5 @@
>      let networkKey = getNetworkKey(privnet);
>      let configured;
>  
> +    let encryption = getNetworkEncryption(privnet);
> +    if (encryption == "WEP") {

Please use === (and below, !==).

@@ +2651,5 @@
>      let configured;
>  
> +    let encryption = getNetworkEncryption(privnet);
> +    if (encryption == "WEP") {
> +      let password = privnet["wep"];

privnet.wep

@@ +2652,5 @@
>  
> +    let encryption = getNetworkEncryption(privnet);
> +    if (encryption == "WEP") {
> +      let password = privnet["wep"];
> +      if (password.length != 5 && password.length != 13) {

Can we really do this? I think some routers allow the user to specify arbitrary passphrases for WEP, and we want to support them as well. Also, this should probably match the check in isWepHexKey in some way.

Comment 3

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 4

[Vincent Chang[:vchang][changyihsin]]

> > +    let encryption = getNetworkEncryption(privnet);
> > +    if (encryption == "WEP") {
> > +      let password = privnet["wep"];
> > +      if (password.length != 5 && password.length != 13) {
> 
> Can we really do this? I think some routers allow the user to specify
> arbitrary passphrases for WEP, and we want to support them as well. Also,
> this should probably match the check in isWepHexKey in some way.

Bug 802137 is filed to limit the password length. Also, the wifi spec also limits the length to 5 and 13. 

Actually, the reason I put the check here is to prevent receiving "Association request to the driver failed" event. We have this event when password length is other than 5 or 13, and I have no idea how to get netId in this case. I tried to get the netId from list_network and search the keyword CURRENT in the lists. But I found that it's possible that there is no keyword CURRENT in the lists. So I end-up thinking about put the length check here. Any comments ?

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1

1. Updated the patch to address comment 2. 
2. create getCurrentNetworkId() to obtain the netId in different cases.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 689668 [details] [diff] [review]
Patch v1.1

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

I have one question and one suggestion. This looks good otherwise, though.

::: dom/wifi/WifiWorker.js
@@ +814,5 @@
> +        // Trying to get netId from
> +        // 1. CURRENT network.
> +        // 2. Trying to associate with SSID 'ssid' event
> +        if (network.status === "CURRENT" ||
> +            (ssid && ssid === dequote(network.ssid))) {

Do we run the risk of mistaking two networks with the same ssid but different security here? Can we possibly use configuredNetworks to help us disambiguate? Also, when will we not have a CURRENT network when we get here?

@@ +1337,5 @@
>          if (manager.stateOrdinal(state) > manager.stateOrdinal(prevState)) {
>            manager.loopDetectionCount++;
>          }
>          if (manager.loopDetectionCount > MAX_SUPPLICANT_LOOP_ITERATIONS) {
> +          notify("disconnected", {ssid: manager.connectionInfo.ssid});

For consistency, do you want to move this under setting loopDetectionCount to 0?

Actually, given the number of times we have this, it's probably worth adding a notifyDisconnected function that sets loopDetectionCount and notifies.

Comment 7

[Vincent Chang[:vchang][changyihsin]]

> Do we run the risk of mistaking two networks with the same ssid but
> different security here? Can we possibly use configuredNetworks to help us
> disambiguate? Also, when will we not have a CURRENT network when we get here?

We don't have CURRENT network in reply of list Network command when we connect to a wep 
network with invalid password length(less than 10 maybe ?). In that case, we may receive the ctrl event “Association request to the driver failed”. No idea about how to get current network in this case. We need to have both ssid and security type. But unfortunately, supplicant doesn't provide enough information. 

The other idea is that we can remember connection information such as netId, ssid and security in associate() function. But still risky unless we can 100% sure that saved networks have correct password. 

> For consistency, do you want to move this under setting loopDetectionCount
> to 0?
> 
> Actually, given the number of times we have this, it's probably worth adding
> a notifyDisconnected function that sets loopDetectionCount and notifies.
We need to set loopDetectionCount and authenticationFailuresCount. So two functions ?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 689668 [details] [diff] [review]
Patch v1.1

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

Vincent and I talked about this, and there's no good way to address my comments. We should just land this as-is.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ed Morley [:emorley]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4694ea14a8f4
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0c8d92b4a005