shill: implement heuristic for WPA/WPA2 bad-passphrase detection
BUG=chromium-os:23211
TEST=new unit tests, manual (see below)
Collateral changes:
- alphabetize properties in Service::Load, Service::Save
- clean up some comments (remove stale ones, fix typos)
- minor refactoring of WiFi::HandleDisconnect, so that we
don't bail early if we can't find the network to disable
Manual testing:
- Try connecting to a WPA network using the wrong password.
Note that the password must be syntactically valid.
(E.g. try "abcdefgh".)
- Observe that Chrome shows the "Network Connection Error" dialog.
- Dismiss dialog.
/home/chronos/user/shill/shill.profile
- Try connecting to a WPA network using the wrong password.
Note that the password must be syntactically valid.
(E.g. try "abcdefgh".)
- Observe that Chrome does NOT show the "Network Connection Error" dialog.
Change-Id: If8f223432c324862965729e05b20b455177ca1f5
Reviewed-on: https://gerrit.chromium.org/gerrit/23290
Reviewed-by: Gary Morain <gmorain@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
Commit-Ready: mukesh agrawal <quiche@chromium.org>
diff --git a/mock_wifi_service.h b/mock_wifi_service.h
index f39492d..f9e3bf9 100644
--- a/mock_wifi_service.h
+++ b/mock_wifi_service.h
@@ -27,6 +27,7 @@
bool hidden_ssid);
virtual ~MockWiFiService();
+ MOCK_METHOD1(SetFailure, void(ConnectFailure failure));
MOCK_METHOD1(SetState, void(ConnectState state));
private:
diff --git a/service.cc b/service.cc
index c1ad05a..26704ae 100644
--- a/service.cc
+++ b/service.cc
@@ -80,6 +80,7 @@
const char Service::kStorageError[] = "Error";
const char Service::kStorageFavorite[] = "Favorite";
const char Service::kStorageGUID[] = "GUID";
+const char Service::kStorageHasEverConnected[] = "HasEverConnected";
const char Service::kStorageName[] = "Name";
const char Service::kStoragePriority[] = "Priority";
const char Service::kStorageProxyConfig[] = "ProxyConfig";
@@ -111,6 +112,7 @@
save_credentials_(true),
technology_(technology),
failed_time_(0),
+ has_ever_connected_(false),
dispatcher_(dispatcher),
unique_name_(base::UintToString(serial_number_++)),
friendly_name_(unique_name_),
@@ -278,6 +280,10 @@
failure_ = kFailureUnknown;
failed_time_ = 0;
}
+ if (state == kStateConnected) {
+ has_ever_connected_ = true;
+ SaveToProfile();
+ }
manager_->UpdateService(this);
metrics_->NotifyServiceStateChanged(this, state);
Error error;
@@ -315,11 +321,12 @@
storage->GetBool(id, kStorageAutoConnect, &auto_connect_);
storage->GetString(id, kStorageCheckPortal, &check_portal_);
storage->GetBool(id, kStorageFavorite, &favorite_);
+ storage->GetString(id, kStorageGUID, &guid_);
+ storage->GetBool(id, kStorageHasEverConnected, &has_ever_connected_);
storage->GetInt(id, kStoragePriority, &priority_);
storage->GetString(id, kStorageProxyConfig, &proxy_config_);
storage->GetBool(id, kStorageSaveCredentials, &save_credentials_);
storage->GetString(id, kStorageUIData, &ui_data_);
- storage->GetString(id, kStorageGUID, &guid_);
LoadEapCredentials(storage, id);
static_ip_parameters_.Load(storage, id);
@@ -328,8 +335,6 @@
// "Failure"
// "Modified"
// "LastAttempt"
- // "APN"
- // "LastGoodAPN"
explicitly_disconnected_ = false;
favorite_ = true;
@@ -366,6 +371,8 @@
storage->SetString(id, kStorageCheckPortal, check_portal_);
}
storage->SetBool(id, kStorageFavorite, favorite_);
+ SaveString(storage, id, kStorageGUID, guid_, false, true);
+ storage->SetBool(id, kStorageHasEverConnected, has_ever_connected_);
storage->SetString(id, kStorageName, friendly_name_);
if (priority_ != kPriorityNone) {
storage->SetInt(id, kStoragePriority, priority_);
@@ -379,19 +386,15 @@
storage->SetBool(id, kStorageSaveCredentials, false);
}
SaveString(storage, id, kStorageUIData, ui_data_, false, true);
- SaveString(storage, id, kStorageGUID, guid_, false, true);
SaveEapCredentials(storage, id);
static_ip_parameters_.Save(storage, id);
// TODO(petkov): Save these:
- // "WiFi.HiddenSSID"
- // "SSID"
// "Failure"
// "Modified"
// "LastAttempt"
- // WiFiService: "Passphrase"
return true;
}
@@ -688,9 +691,7 @@
// This notifies subclassess that EAP parameters have been changed.
set_eap(eap_);
}
- if (profile_.get() && profile_->GetConstStorage()) {
- profile_->UpdateService(this);
- }
+ SaveToProfile();
if ((property == flimflam::kCheckPortalProperty ||
property == flimflam::kProxyConfigProperty) &&
(state_ == kStateConnected ||
@@ -1004,6 +1005,12 @@
return 0;
}
+void Service::SaveToProfile() {
+ if (profile_.get() && profile_->GetConstStorage()) {
+ profile_->UpdateService(this);
+ }
+}
+
void Service::SetStrength(uint8 strength) {
if (strength == strength_) {
return;
diff --git a/service.h b/service.h
index 2acfa07..40ff7a6 100644
--- a/service.h
+++ b/service.h
@@ -78,6 +78,7 @@
static const char kStorageError[];
static const char kStorageFavorite[];
static const char kStorageGUID[];
+ static const char kStorageHasEverConnected[];
static const char kStorageName[];
static const char kStoragePriority[];
static const char kStorageProxyConfig[];
@@ -289,6 +290,8 @@
const std::string &guid() const { return guid_; }
void set_guid(const std::string &guid) { guid_ = guid; }
+ bool has_ever_connected() const { return has_ever_connected_; }
+
int32 priority() const { return priority_; }
void set_priority(int32 priority) { priority_ = priority; }
@@ -455,6 +458,7 @@
FRIEND_TEST(ServiceTest, SetCheckPortal);
FRIEND_TEST(ServiceTest, State);
FRIEND_TEST(ServiceTest, Unload);
+ FRIEND_TEST(WiFiMainTest, SuspectCredentialsWPAPreviouslyConnected);
static const char kAutoConnConnected[];
static const char kAutoConnConnecting[];
@@ -491,6 +495,11 @@
// Returns TCP port of service's HTTP proxy in host order.
uint16 GetHTTPProxyPort(Error *error);
+ // Saves settings to profile, if we have one. Unlike
+ // SaveServiceToProfile, SaveToProfile never assigns this service
+ // into a profile.
+ void SaveToProfile();
+
// Utility function that returns true if a is different from b. When they
// are, "decision" is populated with the boolean value of "a > b".
static bool DecideBetween(int a, int b, bool *decision);
@@ -515,6 +524,8 @@
// The time of the most recent failure. Value is 0 if the service is
// not currently failed.
time_t failed_time_;
+ // Whether or not this service has ever reached kStateConnected.
+ bool has_ever_connected_;
ProfileRefPtr profile_;
PropertyStore store_;
diff --git a/service_under_test.h b/service_under_test.h
index 0e32223..11bedd8 100644
--- a/service_under_test.h
+++ b/service_under_test.h
@@ -14,7 +14,7 @@
class Manager;
class Metrics;
-// This is a simple Service subclass with all the pure-virutal methods stubbed
+// This is a simple Service subclass with all the pure-virutal methods stubbed.
class ServiceUnderTest : public Service {
public:
static const char kRpcId[];
diff --git a/service_unittest.cc b/service_unittest.cc
index 32e528b..3fb5d26 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -35,6 +35,7 @@
using std::string;
using std::vector;
using testing::_;
+using testing::AnyNumber;
using testing::AtLeast;
using testing::DoAll;
using testing::Mock;
@@ -64,6 +65,7 @@
MOCK_METHOD1(TestCallback, void(const Error &error));
protected:
+ typedef scoped_refptr<MockProfile> MockProfileRefPtr;
MockManager mock_manager_;
scoped_refptr<ServiceUnderTest> service_;
@@ -74,6 +76,7 @@
EXPECT_TRUE(service_->save_credentials_);
EXPECT_EQ(Service::kCheckPortalAuto, service_->check_portal_);
EXPECT_EQ(Service::kStateIdle, service_->state());
+ EXPECT_FALSE(service_->has_ever_connected());
}
TEST_F(ServiceTest, GetProperties) {
@@ -265,7 +268,7 @@
.WillRepeatedly(DoAll(SetArgumentPointee<2>(string_value), Return(true)));
ASSERT_TRUE(service_->Load(&storage));
// TODO(pstew): A single string property in the service is tested as
- // a sentinel that properties are being set and reset at the rit times.
+ // a sentinel that properties are being set and reset at the right times.
// However, since property load/store is essentially a manual process,
// it is error prone and should either be exhaustively unit-tested or
// a generic framework for registering loaded/stored properties should
@@ -282,13 +285,14 @@
ServiceRefPtr service_ref(service_);
EXPECT_CALL(*dynamic_cast<ServiceMockAdaptor *>(service_->adaptor_.get()),
- EmitStringChanged(flimflam::kStateProperty, _)).Times(5);
+ EmitStringChanged(flimflam::kStateProperty, _)).Times(7);
EXPECT_CALL(mock_manager_, UpdateService(service_ref));
service_->SetState(Service::kStateConnected);
// A second state change shouldn't cause another update
service_->SetState(Service::kStateConnected);
EXPECT_EQ(Service::kStateConnected, service_->state());
EXPECT_EQ(Service::kFailureUnknown, service_->failure());
+ EXPECT_TRUE(service_->has_ever_connected_);
EXPECT_CALL(mock_manager_, UpdateService(service_ref));
service_->SetState(Service::kStateDisconnected);
@@ -311,6 +315,35 @@
EXPECT_GT(service_->failed_time_, 0);
EXPECT_EQ(Service::kStateIdle, service_->state());
EXPECT_EQ(Service::kFailurePinMissing, service_->failure());
+
+ // If the Service has a Profile, the profile should be saved when
+ // the service enters kStateConnected. (The case where the service
+ // doesn't have a profile is tested above.)
+ MockProfileRefPtr mock_profile(
+ new MockProfile(control_interface(), &mock_manager_));
+ NiceMock<MockStore> storage;
+ service_->set_profile(mock_profile);
+ service_->has_ever_connected_ = false;
+ EXPECT_CALL(mock_manager_, UpdateService(service_ref));
+ EXPECT_CALL(*mock_profile, GetConstStorage())
+ .WillOnce(Return(&storage));
+ EXPECT_CALL(*mock_profile, UpdateService(service_ref));
+ service_->SetState(Service::kStateConnected);
+ EXPECT_TRUE(service_->has_ever_connected_);
+ service_->set_profile(NULL); // Break reference cycle.
+
+ // Similar to the above, but emulate an emphemeral profile, which
+ // has no storage. We can't update the service in the profile, but
+ // we should not crash.
+ service_->state_ = Service::kStateIdle; // Skips state change logic.
+ service_->set_profile(mock_profile);
+ service_->has_ever_connected_ = false;
+ EXPECT_CALL(mock_manager_, UpdateService(service_ref));
+ EXPECT_CALL(*mock_profile, GetConstStorage()).
+ WillOnce(Return(static_cast<StoreInterface *>(NULL)));
+ service_->SetState(Service::kStateConnected);
+ EXPECT_TRUE(service_->has_ever_connected_);
+ service_->set_profile(NULL); // Break reference cycle.
}
TEST_F(ServiceTest, ActivateCellularModem) {
diff --git a/wifi.cc b/wifi.cc
index 0845d36..10d20d2 100644
--- a/wifi.cc
+++ b/wifi.cc
@@ -566,20 +566,22 @@
<< " (or failed to connect to) "
<< affected_service->friendly_name() << ", "
<< "but could not find supplicant network to disable.";
- return;
+ } else {
+ // TODO(quiche): Reconsider giving up immediately. Maybe give
+ // wpa_supplicant some time to retry, first.
+ supplicant_interface_proxy_->RemoveNetwork(rpcid_it->second);
}
SLOG(WiFi, 2) << "WiFi " << link_name() << " disconnected from "
<< " (or failed to connect to) "
<< affected_service->friendly_name();
- // TODO(quiche): Reconsider giving up immediately. Maybe give
- // wpa_supplicant some time to retry, first.
- supplicant_interface_proxy_->RemoveNetwork(rpcid_it->second);
- // TOOD(quiche): In the event that the disconnect was deliberate, we
- // might want to go to SetState(kStateIdle), rather than reporting a
- // failure. crosbug.com/24700.
- // TODO(quiche): In the event that we suspect a password failure,
- // we should not be silent. crosbug.com/23211.
+ if (SuspectCredentials(*affected_service)) {
+ // If we suspect bad credentials, set failure, to trigger an error
+ // mole in Chrome. Failure is a transient state, and we'll
+ // transition out of it immediately (after this if block).
+ affected_service->SetFailure(Service::kFailureBadCredentials);
+ LOG(ERROR) << "Connection failure during 4-Way Handshake. Bad passphrase?";
+ }
affected_service->SetFailureSilent(Service::kFailureUnknown);
affected_service->NotifyCurrentEndpoint(NULL);
metrics()->NotifyServiceDisconnect(affected_service);
@@ -1081,6 +1083,17 @@
}
}
+bool WiFi::SuspectCredentials(const WiFiService &service) const {
+ if (!service.IsSecurityMatch(flimflam::kSecurityPsk)) {
+ // We can only diagnose credentials for WPA/RSN networks. For
+ // others, assume the failure was not credential related.
+ return false;
+ }
+
+ return supplicant_state_ == wpa_supplicant::kInterfaceState4WayHandshake &&
+ !service.has_ever_connected();
+}
+
// Used by Manager.
WiFiServiceRefPtr WiFi::GetService(const KeyValueStore &args, Error *error) {
CHECK_EQ(args.GetString(flimflam::kTypeProperty), flimflam::kTypeWifi);
diff --git a/wifi.h b/wifi.h
index 53e2def..133bf00 100644
--- a/wifi.h
+++ b/wifi.h
@@ -151,6 +151,9 @@
private:
friend class WiFiMainTest; // access to supplicant_*_proxy_, link_up_
+ FRIEND_TEST(WiFiMainTest, SuspectCredentialsOpen); // SuspectCredentials
+ FRIEND_TEST(WiFiMainTest, SuspectCredentialsWPANeverConnected); // as above
+ FRIEND_TEST(WiFiMainTest, SuspectCredentialsWPAPreviouslyConnected); // ""
FRIEND_TEST(WiFiMainTest, InitialSupplicantState); // kInterfaceStateUnknown
FRIEND_TEST(WiFiMainTest, ScanResults); // EndpointMap
FRIEND_TEST(WiFiMainTest, ScanResultsWithUpdates); // EndpointMap
@@ -210,7 +213,8 @@
void ScanDoneTask();
void ScanTask();
void StateChanged(const std::string &new_state);
-
+ // Heuristic check if a connection failure was due to bad credentials.
+ bool SuspectCredentials(const WiFiService &service) const;
void HelpRegisterDerivedInt32(
PropertyStore *store,
const std::string &name,
diff --git a/wifi_unittest.cc b/wifi_unittest.cc
index a6f20f7..846884e 100644
--- a/wifi_unittest.cc
+++ b/wifi_unittest.cc
@@ -33,6 +33,7 @@
#include "shill/mock_device_info.h"
#include "shill/mock_dhcp_config.h"
#include "shill/mock_dhcp_provider.h"
+#include "shill/mock_log.h"
#include "shill/mock_manager.h"
#include "shill/mock_metrics.h"
#include "shill/mock_power_manager.h"
@@ -61,6 +62,7 @@
using ::testing::AnyNumber;
using ::testing::DefaultValue;
using ::testing::DoAll;
+using ::testing::EndsWith;
using ::testing::InSequence;
using ::testing::Invoke;
using ::testing::Mock;
@@ -314,7 +316,7 @@
return WiFiEndpoint::MakeOpenEndpoint(
&proxy_factory_, NULL, ssid, bssid, 0, 0);
}
- MockWiFiServiceRefPtr MakeMockService() {
+ MockWiFiServiceRefPtr MakeMockService(const std::string &security) {
vector<uint8_t> ssid(1, 'a');
return new MockWiFiService(
&control_interface_,
@@ -324,7 +326,7 @@
wifi_,
ssid,
flimflam::kModeManaged,
- flimflam::kSecurityNone,
+ security,
false);
}
void RemoveBSS(const ::DBus::Path &bss_path);
@@ -352,6 +354,9 @@
void ReportStateChanged(const string &new_state) {
wifi_->StateChanged(new_state);
}
+ void SetPendingService(const WiFiServiceRefPtr &service) {
+ wifi_->pending_service_ = service;
+ }
void SetScanInterval(uint16_t interval_seconds) {
wifi_->SetScanInterval(interval_seconds, NULL);
}
@@ -365,20 +370,20 @@
RemoveStateChangeCallback(wifi_->UniqueName()));
wifi_->SetEnabled(false); // Stop(NULL, ResultCallback());
}
- void GetOpenService(const char *service_type,
- const char *ssid,
- const char *mode,
- Error *result) {
- GetServiceInner(service_type, ssid, mode, NULL, NULL, false, result);
+ WiFiServiceRefPtr GetOpenService(const char *service_type,
+ const char *ssid,
+ const char *mode,
+ Error *result) {
+ return GetServiceInner(service_type, ssid, mode, NULL, NULL, false, result);
}
- void GetService(const char *service_type,
- const char *ssid,
- const char *mode,
- const char *security,
- const char *passphrase,
- Error *result) {
- GetServiceInner(service_type, ssid, mode, security, passphrase, false,
- result);
+ WiFiServiceRefPtr GetService(const char *service_type,
+ const char *ssid,
+ const char *mode,
+ const char *security,
+ const char *passphrase,
+ Error *result) {
+ return GetServiceInner(service_type, ssid, mode, security, passphrase,
+ false, result);
}
WiFiServiceRefPtr GetServiceInner(const char *service_type,
const char *ssid,
@@ -1369,7 +1374,7 @@
// Forward transition should trigger a Service state change.
StartWiFi();
dispatcher_.DispatchPendingEvents();
- MockWiFiServiceRefPtr service = MakeMockService();
+ MockWiFiServiceRefPtr service = MakeMockService(flimflam::kSecurityNone);
InitiateConnect(service);
EXPECT_CALL(*service.get(), SetState(Service::kStateAssociating));
ReportStateChanged(wpa_supplicant::kInterfaceStateAssociated);
@@ -1386,7 +1391,7 @@
EXPECT_CALL(*dhcp_config_.get(), RequestIP()).Times(AnyNumber());
StartWiFi();
dispatcher_.DispatchPendingEvents();
- MockWiFiServiceRefPtr service = MakeMockService();
+ MockWiFiServiceRefPtr service = MakeMockService(flimflam::kSecurityNone);
EXPECT_CALL(*service.get(), SetState(Service::kStateConfiguring));
InitiateConnect(service);
ReportStateChanged(wpa_supplicant::kInterfaceStateCompleted);
@@ -1633,7 +1638,7 @@
TEST_F(WiFiMainTest, StateAndIPIgnoreLinkEvent) {
StartWiFi();
- MockWiFiServiceRefPtr service = MakeMockService();
+ MockWiFiServiceRefPtr service = MakeMockService(flimflam::kSecurityNone);
InitiateConnect(service);
EXPECT_CALL(*service.get(), SetState(_)).Times(0);
EXPECT_CALL(*dhcp_config_.get(), RequestIP()).Times(0);
@@ -1874,4 +1879,49 @@
dispatcher_.DispatchPendingEvents();
}
+TEST_F(WiFiMainTest, SuspectCredentialsOpen) {
+ Error e;
+ WiFiServiceRefPtr service = GetOpenService(
+ flimflam::kTypeWifi, "an_ssid", flimflam::kModeManaged, &e);
+ ReportStateChanged(wpa_supplicant::kInterfaceState4WayHandshake);
+ EXPECT_FALSE(service->has_ever_connected());
+ EXPECT_FALSE(wifi()->SuspectCredentials(*service));
+}
+
+TEST_F(WiFiMainTest, SuspectCredentialsWPANeverConnected) {
+ Error e;
+ WiFiServiceRefPtr service =
+ GetService(flimflam::kTypeWifi, "an_ssid", flimflam::kModeManaged,
+ flimflam::kSecurityWpa, "abcdefgh", &e);
+ ReportStateChanged(wpa_supplicant::kInterfaceState4WayHandshake);
+ EXPECT_FALSE(service->has_ever_connected());
+ EXPECT_TRUE(wifi()->SuspectCredentials(*service));
+}
+
+TEST_F(WiFiMainTest, SuspectCredentialsWPAPreviouslyConnected) {
+ Error e;
+ WiFiServiceRefPtr service =
+ GetService(flimflam::kTypeWifi, "an_ssid", flimflam::kModeManaged,
+ flimflam::kSecurityWpa, "abcdefgh", &e);
+ ReportStateChanged(wpa_supplicant::kInterfaceState4WayHandshake);
+ service->has_ever_connected_ = true;
+ EXPECT_FALSE(wifi()->SuspectCredentials(*service));
+}
+
+TEST_F(WiFiMainTest, SuspectCredentialsYieldFailure) {
+ ScopedMockLog log;
+ Error e;
+ MockWiFiServiceRefPtr service = MakeMockService(flimflam::kSecurityWpa);
+ SetPendingService(service);
+ ReportStateChanged(wpa_supplicant::kInterfaceState4WayHandshake);
+ EXPECT_FALSE(service->has_ever_connected());
+
+ EXPECT_CALL(*service, SetFailure(Service::kFailureBadCredentials));
+ EXPECT_CALL(log, Log(_, _, _)).Times(AnyNumber());
+ EXPECT_CALL(log, Log(logging::LOG_ERROR, _, EndsWith("Bad passphrase?")));
+ ReportCurrentBSSChanged(wpa_supplicant::kCurrentBSSNull);
+ EXPECT_EQ(Service::kStateIdle, service->state());
+ EXPECT_TRUE(service->IsFailed());
+}
+
} // namespace shill