Closed Bug 813880 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

RESOLVED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: vchang, Assigned: vchang)

Details

Attachments

(1 file, 1 obsolete file)

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.
QA Contact: vchang
Assignee: nobody → vchang
QA Contact: vchang
Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Attachment #684651 - Flags: review?(mrbkap)
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
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.
Attachment #684651 - Flags: review?(mrbkap)
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P1
> > +    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 ?
Attached patch Patch v1.1Splinter Review
1. Updated the patch to address comment 2. 
2. create getCurrentNetworkId() to obtain the netId in different cases.
Attachment #684651 - Attachment is obsolete: true
Attachment #689668 - Flags: review?(mrbkap)
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.
Attachment #689668 - Flags: review?(mrbkap)
> 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 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.
Attachment #689668 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c449c10f2829
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.