Add method GetKeyFromSystem to WiFiService on Windows and Mac.

Add method GetKeyFromSystem to WiFiService on Windows and Mac.

BUG=328960
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251131

[mef, 2014-02-06 20:35:40]

Hi,

please take a look. I've added platform-specific code to slurp WiFi password
from the system. Overall plumbing to use it from Networking Private API will be
landed separately.

tbarzic - overall
arw - wifi_service_mac.mm
afontan - wifi_service_win.cc

thanks,
-m

[arw, 2014-02-06 20:51:29]

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
File components/wifi/wifi_service_mac.mm (right):

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
components/wifi/wifi_service_mac.mm:379: const char kAirPortServiceName[] =
"AirPort";
Probably could be static?

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
components/wifi/wifi_service_mac.mm:382: void *password_data = nil;
Not super familiar with ObjC++... but should password_data be initialized to
NULL? Was under the impression that nil was reserved for using with ids?

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
components/wifi/wifi_service_mac.mm:401: NULL);
Do you mean to pass in an itemRef here and derive key_data from that?
password_data is not set here, so key_data will not get set below.

[mef, 2014-02-06 21:53:43]

Thanks Amy, PTAL!

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
File components/wifi/wifi_service_mac.mm (right):

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
components/wifi/wifi_service_mac.mm:379: const char kAirPortServiceName[] =
"AirPort";
On 2014/02/06 20:51:30, arw wrote:
> Probably could be static?

Done.

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
components/wifi/wifi_service_mac.mm:382: void *password_data = nil;
On 2014/02/06 20:51:30, arw wrote:
> Not super familiar with ObjC++... but should password_data be initialized to
> NULL? Was under the impression that nil was reserved for using with ids?

Done.

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
components/wifi/wifi_service_mac.mm:401: NULL);
On 2014/02/06 20:51:30, arw wrote:
> Do you mean to pass in an itemRef here and derive key_data from that?
> password_data is not set here, so key_data will not get set below.
Good point. My idea was to have a method of checking whether there is a key
stored in the system without bringing up the privilege elevation prompt. Windows
API can return the key protected by system key, do you know whether there is
something similar on Mac? In my test if I pass an itemRef, then I get the
keychain unlock prompt.

[tbarzic, 2014-02-07 22:50:23]

lgtm (but wait for arw and afontan)

[arw, 2014-02-07 23:25:29]

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
File components/wifi/wifi_service_mac.mm (right):

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
components/wifi/wifi_service_mac.mm:401: NULL);
Yeah, it is possible to see if the password exists. Try this snippet of code:

+ (BOOL)isPasswordAvailable:(NSString *)network {
  if (network) {
    NSDictionary *query;
    query = [NSDictionary dictionaryWithObjectsAndKeys:
             kSecClassGenericPassword, kSecClass,
             network, kSecAttrAccount,
             kCFBooleanTrue, kSecReturnRef,
             nil];

    CFDataRef data;
    OSStatus status = SecItemCopyMatching((CFDictionaryRef)query,
                                          (void *)&data);
    [(id) data autorelease];
    return status == errSecSuccess ? YES : NO;
  }
  return NO;
}

On 2014/02/06 21:53:43, mef wrote:
> On 2014/02/06 20:51:30, arw wrote:
> > Do you mean to pass in an itemRef here and derive key_data from that?
> > password_data is not set here, so key_data will not get set below.
> Good point. My idea was to have a method of checking whether there is a key
> stored in the system without bringing up the privilege elevation prompt.
Windows
> API can return the key protected by system key, do you know whether there is
> something similar on Mac? In my test if I pass an itemRef, then I get the
> keychain unlock prompt.

[afontan, 2014-02-12 09:17:28]

lgtm

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
File components/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
components/wifi/wifi_service_win.cc:654: std::string* error) {
I assume that the caller will have a way to pass the plaintext key encrypted
from the elevated process to the non-elevated process?

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
components/wifi/wifi_service_win.cc:684: if (reader.NodeName() ==
kProtectedElement) {
else if?

[afontan, 2014-02-12 09:29:27]

lgtm

A couple more minor comments with second thoughts.

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
File components/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
components/wifi/wifi_service_win.cc:682: reader.ReadElementContent(key_data);
if !get_plaintext_key we are done, we could break.

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
components/wifi/wifi_service_win.cc:688: *error = kWiFiServiceError;
Can this ever happen? Do we need to protect against it? It would mean the call
to GetProfile has not properly succeeded, right?

[mef, 2014-02-12 16:29:37]

Thanks, Amy, PTAL!

Thanks, Antonio!

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
File components/wifi/wifi_service_mac.mm (right):

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
components/wifi/wifi_service_mac.mm:401: NULL);
On 2014/02/07 23:25:29, arw wrote:
> Yeah, it is possible to see if the password exists. Try this snippet of code:
> 
> + (BOOL)isPasswordAvailable:(NSString *)network {
>   if (network) {
>     NSDictionary *query;
>     query = [NSDictionary dictionaryWithObjectsAndKeys:
>              kSecClassGenericPassword, kSecClass,
>              network, kSecAttrAccount,
>              kCFBooleanTrue, kSecReturnRef,
>              nil];
> 
>     CFDataRef data;
>     OSStatus status = SecItemCopyMatching((CFDictionaryRef)query,
>                                           (void *)&data);
>     [(id) data autorelease];
>     return status == errSecSuccess ? YES : NO;
>   }
>   return NO;
> }
> 
> On 2014/02/06 21:53:43, mef wrote:
> > On 2014/02/06 20:51:30, arw wrote:
> > > Do you mean to pass in an itemRef here and derive key_data from that?
> > > password_data is not set here, so key_data will not get set below.
> > Good point. My idea was to have a method of checking whether there is a key
> > stored in the system without bringing up the privilege elevation prompt.
> Windows
> > API can return the key protected by system key, do you know whether there is
> > something similar on Mac? In my test if I pass an itemRef, then I get the
> > keychain unlock prompt.
> 

Done. SecKeychainFindGenericPassword with NULL output parameters seemed to work
for detection as well.

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
File components/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
components/wifi/wifi_service_win.cc:654: std::string* error) {
On 2014/02/12 09:17:28, afontan wrote:
> I assume that the caller will have a way to pass the plaintext key encrypted
> from the elevated process to the non-elevated process?
Yes, see https://codereview.chromium.org/102993002/ for the framework around it.

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
components/wifi/wifi_service_win.cc:682: reader.ReadElementContent(key_data);
On 2014/02/12 09:29:28, afontan wrote:
> if !get_plaintext_key we are done, we could break.

Done.

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
components/wifi/wifi_service_win.cc:684: if (reader.NodeName() ==
kProtectedElement) {
On 2014/02/12 09:17:28, afontan wrote:
> else if?

Done.

https://codereview.chromium.org/156943002/diff/80001/components/wifi/wifi_ser...
components/wifi/wifi_service_win.cc:688: *error = kWiFiServiceError;
On 2014/02/12 09:29:28, afontan wrote:
> Can this ever happen? Do we need to protect against it? It would mean the call
> to GetProfile has not properly succeeded, right?
In my testing without UAC privilege escalation call to GetProfile with
WLAN_PROFILE_GET_PLAINTEXT_KEY flag returns success, but has protected
keyMaterial. This shouldn't happen as we always call it from elevated process,
but seems like a good safety measure.

[arw, 2014-02-12 17:41:50]

lgtm

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
File components/wifi/wifi_service_mac.mm (right):

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
components/wifi/wifi_service_mac.mm:401: NULL);
Okay, I think I understand now. My final suggestion is to make this method a
little more explicit about what it does (an "exists" parameter, or even perhaps
breaking this out into 2 methods). Also, it's also not quite clear from the
comment on this function that it is checking for the presence of a password if
get_plaintext_key is false. From reading the method description - it sounds like
the method would attempt to retrieve the password without privilege elevation if
get_plaintext_key is not set to true.

On 2014/02/12 16:29:37, mef wrote:
> On 2014/02/07 23:25:29, arw wrote:
> > Yeah, it is possible to see if the password exists. Try this snippet of
code:
> > 
> > + (BOOL)isPasswordAvailable:(NSString *)network {
> >   if (network) {
> >     NSDictionary *query;
> >     query = [NSDictionary dictionaryWithObjectsAndKeys:
> >              kSecClassGenericPassword, kSecClass,
> >              network, kSecAttrAccount,
> >              kCFBooleanTrue, kSecReturnRef,
> >              nil];
> > 
> >     CFDataRef data;
> >     OSStatus status = SecItemCopyMatching((CFDictionaryRef)query,
> >                                           (void *)&data);
> >     [(id) data autorelease];
> >     return status == errSecSuccess ? YES : NO;
> >   }
> >   return NO;
> > }
> > 
> > On 2014/02/06 21:53:43, mef wrote:
> > > On 2014/02/06 20:51:30, arw wrote:
> > > > Do you mean to pass in an itemRef here and derive key_data from that?
> > > > password_data is not set here, so key_data will not get set below.
> > > Good point. My idea was to have a method of checking whether there is a
key
> > > stored in the system without bringing up the privilege elevation prompt.
> > Windows
> > > API can return the key protected by system key, do you know whether there
is
> > > something similar on Mac? In my test if I pass an itemRef, then I get the
> > > keychain unlock prompt.
> > 
> 
> Done. SecKeychainFindGenericPassword with NULL output parameters seemed to
work
> for detection as well.

[afontan, 2014-02-12 17:57:17]

lgtm

https://codereview.chromium.org/156943002/diff/180001/components/wifi/wifi_se...
File components/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/156943002/diff/180001/components/wifi/wifi_se...
components/wifi/wifi_service_win.cc:690: break;
Interesting that the call can partially succeed depending on UAC. I would
suggest adding a comment in the code explaining the case where this error can
happen so it is more clear to the reader, I do not think it is too obvious.

[mef, 2014-02-12 22:26:21]

Thanks!

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
File components/wifi/wifi_service_mac.mm (right):

https://codereview.chromium.org/156943002/diff/1/components/wifi/wifi_service...
components/wifi/wifi_service_mac.mm:401: NULL);
On 2014/02/12 17:41:50, arw wrote:
> Okay, I think I understand now. My final suggestion is to make this method a
> little more explicit about what it does (an "exists" parameter, or even
perhaps
> breaking this out into 2 methods). Also, it's also not quite clear from the
> comment on this function that it is checking for the presence of a password if
> get_plaintext_key is false. From reading the method description - it sounds
like
> the method would attempt to retrieve the password without privilege elevation
if
> get_plaintext_key is not set to true.

Done. There is no immediate need for key existence checking method, so I've just
removed 'get_plaintext_key' parameter and will consider adding a separate method
later if necessary.

https://codereview.chromium.org/156943002/diff/180001/components/wifi/wifi_se...
File components/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/156943002/diff/180001/components/wifi/wifi_se...
components/wifi/wifi_service_win.cc:690: break;
On 2014/02/12 17:57:18, afontan wrote:
> Interesting that the call can partially succeed depending on UAC. I would
> suggest adding a comment in the code explaining the case where this error can
> happen so it is more clear to the reader, I do not think it is too obvious.

Done.

[mef, 2014-02-13 17:44:29]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-02-13 17:44:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/156943002/300001

[commit-bot: I haz the power, 2014-02-13 19:52:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/156943002/300001

[commit-bot: I haz the power, 2014-02-13 21:37:36]

Change committed as 251131