Closed Bug 789217 Opened 7 years ago Closed 6 years ago

[Wifi] Support keystore protocol for wpa_supplicant to read CA from NSS

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(blocking-basecamp:-)

RESOLVED FIXED
blocking-basecamp -

People

(Reporter: briansmith, Assigned: chucklee)

References

Details

(Whiteboard: [ft:ril])

Attachments

(7 files, 28 obsolete files)

54.44 KB, image/png
Details
2.89 KB, patch
Details | Diff | Splinter Review
3.53 KB, patch
Details | Diff | Splinter Review
7.79 KB, patch
Details | Diff | Splinter Review
2.24 KB, patch
Details | Diff | Splinter Review
13.63 KB, patch
Details | Diff | Splinter Review
2.94 KB, patch
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #775499 +++

We use wpa_supplicant for 802.11 support, including WPA Enterprise.

wpa_supplicant has its own TLS implementation and apparently uses OpenSSL for certificate validation by default. This means:

1. The WPA enteprise support cannot use the same (root) certificate database that we use for the rest of B2G.

2. We must track and respond to all security bugs in the wpa_supplicant and/or OpenSSL implementations, in addition to NSS.

Basically, we should just integrate wpa_supplicant with NSS. According to the wpa_supplicant documentation, this should be straightforward to do because wpa_supplicant has a plug-in architecture for the TLS implementation.

See https://bugzilla.redhat.com/show_bug.cgi?id=348471 for Red Hat's bug tracking the integration of wpa_supplicant with NSS. Maybe we can work together on this, and/or push the NSS integration to the wpa_supplicant upstream.
Brian, should this be a basecamp blocker?
blocking-basecamp: + → ?
Whiteboard: [blocked-on-input Brian Smith]
This is a blocker to WPA Enterprise support. We should disable WPA Enterprise until this bug and bug 775499 are fixed. I do not think re-enabling WPA Enterprise needs to be a basecamp blocker.
Whiteboard: [blocked-on-input Brian Smith]
Thanks, Brian.  I filed bug 790054 to explicitly disable WPA Enterprise support and bug 790056 to enable it post-Basecamp.
blocking-basecamp: ? → -
For us it's important to ship with WPA Enterprise support. Even a broken support would be better than no support at all. In any case, we can take this bug at TEF if you wish to try and ship it in V1. 

Note that besides integrating NSS we should/would add a way to add user provided certificates since most companies (before somebody asks, that means all the companies I know about here) use internal certificates for their WPA Enteprise servers, so just integrating this with NSS would leave all those companies out of luck.
As mentioned to Brian during a past NSS conference call, there's some interest from ChromiumOS on this as well, as it also uses wpa_supplicant.

One of the things we view as blocking is the ability to have more granular trust settings. It should be possible to trust a root for WPA without having that trust transitively apply to other verification. Bug 816853 (alternatively/along with Brian's Bug 764973 ) facilitate that effort, although that relies on application-specific effort and doesn't allow the trust settings to be stored in the central NSS DB. Both Microsoft and Apple have more defined concepts of trust (Microsoft via cert attributes, Apple via trust policies), which facilitate the central storage/management of certs.
Assignee: nobody → chulee
The left green part represents current way that wpa_supplicant get certfication key in android - the key is managed by a daemon called "keystore"[1], and wap_supplicant get key from "keystore" through a local socket[2]. This flow also exists on current B2G wpa_supplicant, but we don't actually manage keys by "keystore"

Considering our partners might use wpa_supplicant from vendors, and it seems supporting "keystore" is a must in android[3]. The proposed plan is left wpa_supplicant untouched, and replace "keystore" of android with a service in gecko, and read keys from NSS.(the orange part)

The key manage part will be done in bug 867899 with an Web API, so it should be enough for the keystore in gecko support only read function.

What the partner needs to do is disable keystore in init.rc, and maybe patch the corresponding local socket protocol is they use different protocol as [2].

[1] https://android.googlesource.com/platform/system/security/+/tools_r21/keystore/keystore.cpp
[2] https://android.googlesource.com/platform/system/security/+/tools_r21/keystore/keystore_get.h
[3] http://hostap.epitest.fi/gitweb/gitweb.cgi?p=hostap.git;a=blob;f=src/crypto/tls_openssl.c;h=8600f5f0aa80a8cd34e2159140a56acfb9ae8a4d;hb=HEAD#l1368
Run Setup() in server mode of unix socket.
Attached patch WIP - 0003. Implement keystore. (obsolete) — Splinter Review
Implement keystore ipc to replace keystore daemon in android.
Attachment #768792 - Attachment filename: 0004.Run_as_system_worker.patch → WIP - 0004.Run_as_system_worker.patch
Attachment #768792 - Attachment description: 0004. Bring up keystore. → WIP - 0004. Bring up keystore.
Attachment #768792 - Attachment filename: WIP - 0004.Run_as_system_worker.patch → 0004.Run_as_system_worker.patch
Disable keystore daemon of android.
Target file is B2G/system/core/rootdir/init.rc.

This is affected on emulator but not on unagi.
Not tested on other devices.
To test test keystore function, use command:
- Get hardcoded CA :
  adb shell keystore_cli g CACERT_test_der
- Get RAW CA(transformed to DER format)
  adb shell keystore_cli g CACERT_test_raw
- GET CA in NSS(transformed to DER format)
  adb shell keystore_cli g CACERT_test_nss

To test WAP-EAP connection, do the following steps:
1. adb pull /data/misc/wifi/wpa_supplicant.conf
2. Modify wpa_supplicant.conf with WPA-EAP setting, with setting
   ca_cert="keystore://CACERT_test_der" or
   ca_cert="keystore://CACERT_test_raw" or
   ca_cert="keystore://CACERT_test_nss"
3. adb push wpa_supplicant.conf /data/misc/wifi/wpa_supplicant.conf
4. adb shell wpa_cli reconfigure
5. check connection state.
Rebase.
Remove "FORCE_STATIC_LIB = 1" in makefile.
Attachment #768787 - Attachment is obsolete: true
1. Rebase.
2. Replace MOZ_NOT_REACHED() with MOZ_CRASH().
3. Add code to read cert from NSS by name.
Attachment #768791 - Attachment is obsolete: true
Attachment #790094 - Attachment description: WIP - 0002. Setup socket after bind. → WIP - 0002. Setup socket after bind. V2
Comment on attachment 768799 [details] [diff] [review]
Test Only - Disable keystore daemon.

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

After test, replaced keystore still works without this patch.
But we should suggest partners to disable keystore from android.
Attachment #768799 - Flags: review-
Summary: WPA enterprise certificates are not validated the same way as we validate certs for TLS handshakes → [Wifi] Support keystore protocol for wpa_supplicant to read CA from gecko
Summary: [Wifi] Support keystore protocol for wpa_supplicant to read CA from gecko → [Wifi] Support keystore protocol for wpa_supplicant to read CA from NSS
Attached patch 0001. Add skeleton for keystore. (obsolete) — Splinter Review
New IPC to replace android's keystore daemon, for reading CA in NSS to wpa_supplicant.
Attachment #790093 - Attachment is obsolete: true
Attachment #805855 - Flags: review?(khuey)
Attached patch 0002. Setup socket after bind. (obsolete) — Splinter Review
Server socket will be closed after client disconnect, so we should setup socket on every accept.
Attachment #790094 - Attachment is obsolete: true
Attachment #805857 - Flags: review?(khuey)
Attached patch 0003. Implement keystore. (obsolete) — Splinter Review
Implement keystore daemon in gecko, but only support get command.

Because it uses NSS API to read CA from NSS DB, so also need bsmith's review on that part.
Attachment #790097 - Attachment is obsolete: true
Attachment #805859 - Flags: review?(khuey)
Attachment #805859 - Flags: review?(brian)
Attached patch 0004. Bring up keystore. (obsolete) — Splinter Review
Bring up keystore ipc.
Attachment #790099 - Attachment is obsolete: true
Attachment #805862 - Flags: review?(khuey)
Attached patch 0001. Add skeleton for keystore. (obsolete) — Splinter Review
Renew make scripts.
Attachment #805855 - Attachment is obsolete: true
Attachment #805855 - Flags: review?(khuey)
Attachment #805875 - Flags: review?(khuey)
Attached patch 0001. Add skeleton for keystore. (obsolete) — Splinter Review
Bad copy & paste...
Attachment #805887 - Flags: review?(khuey)
Attachment #805875 - Attachment is obsolete: true
Attachment #805875 - Flags: review?(khuey)
Comment on attachment 805857 [details] [diff] [review]
0002. Setup socket after bind.

I think it would be better if the other Kyle reviewed this.  I'm not familiar with UnixSocket.
Attachment #805857 - Flags: review?(khuey) → review?(kyle)
Comment on attachment 805857 [details] [diff] [review]
0002. Setup socket after bind.

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

::: ipc/unixsocket/UnixSocket.cpp
@@ +526,5 @@
> +      nsRefPtr<OnSocketEventTask> t =
> +        new OnSocketEventTask(this, OnSocketEventTask::CONNECT_ERROR);
> +      NS_DispatchToMainThread(t);
> +      return;
> +    }

Pretty sure this would break bluetooth. SetUp (which is a really bad name, should probably be changed to something more descriptive :( ) is only meant to be called on the final, connected socket, not on the listening bound socket. If we need to prepare a socket after a bind, this might be a good place to add yet another virtual function to the Connector class.
Attachment #805857 - Flags: review?(kyle) → review-
Comment on attachment 805859 [details] [diff] [review]
0003. Implement keystore.

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

I think the other Kyle should review this.  I don't know much about UnixSocket.

::: ipc/keystore/KeyStore.cpp
@@ +15,5 @@
> +#define LOG(args...)  printf(args);
> +#endif
> +
> +#include "jsfriendapi.h"
> +#include "nsThreadUtils.h" // For NS_IsMainThread.

NS_IsMainThread lives in MainThreadUtils.h now.

@@ +16,5 @@
> +#endif
> +
> +#include "jsfriendapi.h"
> +#include "nsThreadUtils.h" // For NS_IsMainThread.
> +#include "KeyStore.h"

Please include KeyStore.h first, before any other includes.

@@ +22,5 @@
> +#include "plbase64.h"
> +#include "cert.h"
> +#include "certdb.h"
> +
> +USING_WORKERS_NAMESPACE

Why do you need this?  I don't see any worker code being used here.

@@ +54,5 @@
> +bool
> +KeyStoreConnector::CreateAddr(bool aIsServer,
> +                         socklen_t& aAddrSize,
> +                         sockaddr_any& aAddr,
> +                         const char* aAddress)

Line up the parameters please.

::: ipc/keystore/KeyStore.h
@@ +6,5 @@
> +
> +#ifndef mozilla_ipc_KeyStore_h
> +#define mozilla_ipc_KeyStore_h 1
> +
> +#include <mozilla/dom/workers/Workers.h>

Why do you need this header?  I don't think you're doing anything with web workers here.

@@ +7,5 @@
> +#ifndef mozilla_ipc_KeyStore_h
> +#define mozilla_ipc_KeyStore_h 1
> +
> +#include <mozilla/dom/workers/Workers.h>
> +#include <mozilla/ipc/UnixSocket.h>

Usual style is "Foo.h" for mozilla headers.
Attachment #805859 - Flags: review?(khuey) → review?(kyle)
Comment on attachment 805859 [details] [diff] [review]
0003. Implement keystore.

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

::: ipc/keystore/KeyStore.cpp
@@ +275,5 @@
> +// Data response
> +void
> +KeyStore::SendData(const uint8_t *aData, int aLength)
> +{
> +  UnixSocketRawData* length = new UnixSocketRawData(2);

Super-Nit: UnixSocketRawData has a constructor that will take void*'s and do the copy for you. Might save you some lines here and elsewhere.
Attachment #805859 - Flags: review?(kyle) → review+
On second thought, build unixsocket and keystore ipc while RIL or BT enabled(which indicates firefox OS), in case it affects other version.
Attachment #805887 - Attachment is obsolete: true
Oops, wrong file..
Attachment #809088 - Attachment is obsolete: true
Attached patch 0002. Implement keystore. V2 (obsolete) — Splinter Review
Find a way which keystore works without modifying unix socket, and address reviewers' comments.
Attachment #805857 - Attachment is obsolete: true
Attachment #805859 - Attachment is obsolete: true
Attachment #805859 - Flags: review?(brian)
Attachment #809091 - Flags: review?(kyle)
Attachment #809091 - Flags: review?(brian)
Attached patch 0003. Bring up keystore. (obsolete) — Splinter Review
Add reviewer's name, and change index number.
Attachment #805862 - Attachment is obsolete: true
Comment on attachment 809091 [details] [diff] [review]
0002. Implement keystore. V2

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

r=me

::: ipc/keystore/KeyStore.cpp
@@ +232,5 @@
> +
> +// Transform base64 certification data into DER format
> +void
> +KeyStore::FormatCaData(const uint8_t *caData, int caDataLength, const char *name,
> +                       const uint8_t **formatData, int &formatDataLength)

Supernit: We prefix arguments with "a" everywhere else in the file, but not here.
Attachment #809091 - Flags: review?(kyle) → review+
Address comment 44.
Attachment #809091 - Attachment is obsolete: true
Attachment #809091 - Flags: review?(brian)
Attachment #768799 - Attachment is obsolete: true
Failed to building b2g-desktop when compiling ipc/keystore/KeyStore.cpp:
ipc/keystore/KeyStore.cpp:40:30: error: 'unlink' was not declared in this scope
ipc/keystore/KeyStore.h:37:31: error: 'NAME_MAX' was not declared in this scope
Comment on attachment 812433 [details] [diff] [review]
00001. Add skeleton for keystore. V2

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

::: ipc/moz.build
@@ +17,5 @@
>  if CONFIG['MOZ_B2G_BT']:
>      DIRS += ['dbus']
>  
>  if CONFIG['MOZ_B2G_RIL'] or CONFIG['MOZ_B2G_BT']:
> +    DIRS += ['unixsocket', 'keystore']

Don't know if there is other work-arounds, but I think moving keystore to |MOZ_WIDGET_TOOKIT == 'gonk'| section would be a better idea.
Whiteboard: [ft:ril]
You need to log in before you can comment on or make changes to this bug.