953237 - [FUGU][wifi]Support WAPI-CERT mode

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Wifi, defect)

Description

[Pin Zhang [:pzhang] (inactive)]

+++ This bug was initially created as a clone of Bug #833236 +++
Bug 833236 is for WAPI-PSK mode, we still need to support WAPI certificate mode.

Comment 1

[DongShengXue]

Attachment: gaia-support-WAPI-certificate.patch

Comment 2

[DongShengXue]

Attachment: gecko-support-WAPI-certificate.patch

Comment 3

[DongShengXue]

Because we can not import private key to keystore now, the patch will be failed when connect to the WAPI_CERT AP. If you want to test it, you can modify gecko/dom/wifi/WifiWorker.js:1729, from net.wapi_user_cert = quote("keystore://WIFI_USERCERT_" + net.wapiUserCertificate); to net.wapi_user_cert = quote("path/user.cer");

Comment 4

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8360159 [details] [diff] [review]
gecko-support-WAPI-certificate.patch

I don't think I'm qualified, ask Vincent for review here.

Comment 5

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8360157 [details] [diff] [review]
gaia-support-WAPI-certificate.patch

We usually send a PR for Gaia and post the link as attachment for review.

Comment 6

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8360159 [details] [diff] [review]
gecko-support-WAPI-certificate.patch

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

Chuck, can you help to review the patches ?

Comment 7

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try seems better: https://tbpl.mozilla.org/?tree=Try&rev=aee163b7e6c3

Comment 8

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Sorry, post on wrong bug. /_\

Comment 9

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8360159 [details] [diff] [review]
gecko-support-WAPI-certificate.patch

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

Please change patch to eight-line-context(with argument "-U 8") [1], transform to hg style, and change subject to "Bug XXXXXX - Patch Descriptioin". [2]

[1] https://developer.mozilla.org/en/docs/Mercurial_FAQ#How_can_I_diff_and_patch_files.3F
[2] https://developer.mozilla.org/en/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: dom/wifi/NssUtils.cpp
@@ +126,4 @@
>      fullNickname += userNickname;
>      *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER;
>    } else {
> +    // User certificate

I think we need to a rule to identify user certificate, not in else.

About the user certificate, is it a must for you?
And what types of user certificate do you expect can be imported?

::: dom/wifi/WifiWorker.js
@@ +1004,4 @@
>      "ssid", "bssid", "psk", "wep_key0", "wep_key1", "wep_key2", "wep_key3",
>      "wep_tx_keyidx", "priority", "key_mgmt", "scan_ssid", "disabled",
>      "identity", "password", "auth_alg", "phase1", "phase2", "eap", "pin",
> +    "pcsc", "ca_cert", "proto", "wapi_as_cert","wapi_user_cert"

Is there any reason to use "wapi_as_cert" and "wapi_user_cert" instead of existing "ca_cert" and "user_cert"?

Based on the source code I found on github [1], these two parameters are used to indicate certificate files to be merged into a WAPI specific format file [2], for read into memory right after [3].

I think it can be replaced by using "ca_cert" and "user_cert" while reading config [4], and use |BIO_from_keystore| [5] to read corresponding certificates in DER format from NSS directly.
Then we can get rid of the merge-read process, and prevent creating template certificate file for better security.

[1] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wpa_supplicant.c#L1539
[2] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wpa_supplicant.c#L1464
[3] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wapi_config.c#L388
[4] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wpa_supplicant.c#L1619
[5] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/tls_openssl.c#L44

@@ +1726,5 @@
> +      if (hasValidProperty("wapiAsCertificate"))
> +        net.wapi_as_cert = quote("keystore://WIFI_SERVERCERT_" + net.wapiAsCertificate);
> +      if (hasValidProperty("wapiUserCertificate"))
> +        net.wapi_user_cert = quote("keystore://WIFI_USERCERT_" + net.wapiUserCertificate);
> +    }

"keystore://" is the keyword to read certificate from keystore daemon through unix socket, but the source code [1] shows wapi_*_cert doesn't support such method.
Or you have patched your own supplicant to support it?

[1] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wpa_supplicant.c#L1477

Comment 10

[DongShengXue]

(In reply to Chuck Lee [:chucklee] from comment #9)
> Comment on attachment 8360159 [details] [diff] [review]
> gecko-support-WAPI-certificate.patch
> 
> Review of attachment 8360159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please change patch to eight-line-context(with argument "-U 8") [1],
> transform to hg style, and change subject to "Bug XXXXXX - Patch
> Descriptioin". [2]
> 
> [1]
> https://developer.mozilla.org/en/docs/
> Mercurial_FAQ#How_can_I_diff_and_patch_files.3F
> [2]
> https://developer.mozilla.org/en/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
> 

OK, I will update my patch.

> ::: dom/wifi/NssUtils.cpp
> @@ +126,4 @@
> >      fullNickname += userNickname;
> >      *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER;
> >    } else {
> > +    // User certificate
> 
> I think we need to a rule to identify user certificate, not in else.

If the certificate is not root, it should be user certificate, right?

> 
> About the user certificate, is it a must for you?
Yes.

> And what types of user certificate do you expect can be imported?
p7.

> 
> ::: dom/wifi/WifiWorker.js
> @@ +1004,4 @@
> >      "ssid", "bssid", "psk", "wep_key0", "wep_key1", "wep_key2", "wep_key3",
> >      "wep_tx_keyidx", "priority", "key_mgmt", "scan_ssid", "disabled",
> >      "identity", "password", "auth_alg", "phase1", "phase2", "eap", "pin",
> > +    "pcsc", "ca_cert", "proto", "wapi_as_cert","wapi_user_cert"
> 
> Is there any reason to use "wapi_as_cert" and "wapi_user_cert" instead of
> existing "ca_cert" and "user_cert"?

They are defined in FUGU's revised supplicant, you can find them under |$B2G_FUGU/external/wpa_supplicant_8/|.

> 
> Based on the source code I found on github [1], these two parameters are
> used to indicate certificate files to be merged into a WAPI specific format
> file [2], for read into memory right after [3].
> 
> I think it can be replaced by using "ca_cert" and "user_cert" while reading
> config [4], and use |BIO_from_keystore| [5] to read corresponding
> certificates in DER format from NSS directly.
> Then we can get rid of the merge-read process, and prevent creating template
> certificate file for better security.

Same here, please check the codes under wpa_supplicant_8.

> 
> [1]
> https://github.com/Dopi/JetPlatform/blob/master/delta/external/
> wpa_supplicant/wpa_supplicant.c#L1539
> [2]
> https://github.com/Dopi/JetPlatform/blob/master/delta/external/
> wpa_supplicant/wpa_supplicant.c#L1464
> [3]
> https://github.com/Dopi/JetPlatform/blob/master/delta/external/
> wpa_supplicant/wapi_config.c#L388
> [4]
> https://github.com/Dopi/JetPlatform/blob/master/delta/external/
> wpa_supplicant/wpa_supplicant.c#L1619
> [5]
> https://github.com/Dopi/JetPlatform/blob/master/delta/external/
> wpa_supplicant/tls_openssl.c#L44
> 
> @@ +1726,5 @@
> > +      if (hasValidProperty("wapiAsCertificate"))
> > +        net.wapi_as_cert = quote("keystore://WIFI_SERVERCERT_" + net.wapiAsCertificate);
> > +      if (hasValidProperty("wapiUserCertificate"))
> > +        net.wapi_user_cert = quote("keystore://WIFI_USERCERT_" + net.wapiUserCertificate);
> > +    }
> 
> "keystore://" is the keyword to read certificate from keystore daemon
> through unix socket, but the source code [1] shows wapi_*_cert doesn't
> support such method.
> Or you have patched your own supplicant to support it?
> 
> [1]
> https://github.com/Dopi/JetPlatform/blob/master/delta/external/
> wpa_supplicant/wpa_supplicant.c#L1477

Same here, please check the codes under wpa_supplicant_8.

Comment 11

[DongShengXue]

Attachment: Gecko Patch

Comment 12

[DongShengXue]

Attachment: Gaia pull request

Comment 13

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to DongShengXue from comment #10)
> > ::: dom/wifi/NssUtils.cpp
> > @@ +126,4 @@
> > >      fullNickname += userNickname;
> > >      *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER;
> > >    } else {
> > > +    // User certificate
> > 
> > I think we need to a rule to identify user certificate, not in else.
> 
> If the certificate is not root, it should be user certificate, right?
>

AFAIK, The isRoot indicates the certificate is a valid root certificate, so an invalid root certificate, maybe missing some item, is also presented as |isRoot == false|.
So |isRoot == false| doesn't imply it is a user certificate.

> > 
> > About the user certificate, is it a must for you?
> Yes.
> 
> > And what types of user certificate do you expect can be imported?
> p7.
> 

Importing user certificate is not in current scope, there might be some security/privacy issue.
I think we need opinion from others.

> > 
> > ::: dom/wifi/WifiWorker.js
> > @@ +1004,4 @@
> > >      "ssid", "bssid", "psk", "wep_key0", "wep_key1", "wep_key2", "wep_key3",
> > >      "wep_tx_keyidx", "priority", "key_mgmt", "scan_ssid", "disabled",
> > >      "identity", "password", "auth_alg", "phase1", "phase2", "eap", "pin",
> > > +    "pcsc", "ca_cert", "proto", "wapi_as_cert","wapi_user_cert"
> > 
> > Is there any reason to use "wapi_as_cert" and "wapi_user_cert" instead of
> > existing "ca_cert" and "user_cert"?
> 
> They are defined in FUGU's revised supplicant, you can find them under
> |$B2G_FUGU/external/wpa_supplicant_8/|.
> 

I can't find wapi-related code in fugu code base you mentioned, can you provide a git link?

Comment 14

[DongShengXue]

(In reply to Chuck Lee [:chucklee] from comment #13)
> (In reply to DongShengXue from comment #10)
> > > ::: dom/wifi/NssUtils.cpp
> > > @@ +126,4 @@
> > > >      fullNickname += userNickname;
> > > >      *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER;
> > > >    } else {
> > > > +    // User certificate
> > > 
> > > I think we need to a rule to identify user certificate, not in else.
> > 
> > If the certificate is not root, it should be user certificate, right?
> >
> 
> AFAIK, The isRoot indicates the certificate is a valid root certificate, so
> an invalid root certificate, maybe missing some item, is also presented as
> |isRoot == false|.
> So |isRoot == false| doesn't imply it is a user certificate.
> 

Ok, I will check the definition of the isRoot, and update the patch. 

> > > 
> > > About the user certificate, is it a must for you?
> > Yes.
> > 
> > > And what types of user certificate do you expect can be imported?
> > p7.
> > 
> 
> Importing user certificate is not in current scope, there might be some
> security/privacy issue.
> I think we need opinion from others.
> 

Sure, I think we can refer to Bug 868373.

> > > 
> > > ::: dom/wifi/WifiWorker.js
> > > @@ +1004,4 @@
> > > >      "ssid", "bssid", "psk", "wep_key0", "wep_key1", "wep_key2", "wep_key3",
> > > >      "wep_tx_keyidx", "priority", "key_mgmt", "scan_ssid", "disabled",
> > > >      "identity", "password", "auth_alg", "phase1", "phase2", "eap", "pin",
> > > > +    "pcsc", "ca_cert", "proto", "wapi_as_cert","wapi_user_cert"
> > > 
> > > Is there any reason to use "wapi_as_cert" and "wapi_user_cert" instead of
> > > existing "ca_cert" and "user_cert"?
> > 
> > They are defined in FUGU's revised supplicant, you can find them under
> > |$B2G_FUGU/external/wpa_supplicant_8/|.
> > 
> 
> I can't find wapi-related code in fugu code base you mentioned, can you
> provide a git link?

git clone https://github.com/sprd-ffos/B2G.git -b sprd
./config.sh sp7710gaplus_gonk4.0_master

Comment 15

[DongShengXue]

Attachment: Gecko Path V2

Comment 16

[Paul Theriault [:pauljt] (no longer reading bugmail)]

What is the needinfo for here? Just an FYI that this is happening or do you need a review? I can't see any issues with this, but then I am not that familiar with this code, so Brian is definitely more qualified to review here.

Comment 17

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8361510 [details] [diff] [review]
Gecko Path V2

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

::: dom/wifi/NssUtils.cpp
@@ +127,5 @@
>      *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER;
> +  } else if (aCert->isUser) {
> +    // User certificate
> +    fullNickname.AssignLiteral("WIFI_USERCERT_");
> +    fullNickname += userNickname;

Not sure if this is a concern because I can't find the rest of this file, but do we need to do some validation (length, metacharacters etc) on aNickname here (above).  I assume that this comes from content?

Comment 18

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Hi Paul,
  I like to know if you have any security concerns about importing user certificates to NSS.

(In reply to Paul Theriault [:pauljt] from comment #17)
> Comment on attachment 8361510 [details] [diff] [review]
> Gecko Path V2
> 
> Review of attachment 8361510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/NssUtils.cpp
> @@ +127,5 @@
> >      *aUsageFlag |= WIFI_CA_USAGE_FLAG_SERVER;
> > +  } else if (aCert->isUser) {
> > +    // User certificate
> > +    fullNickname.AssignLiteral("WIFI_USERCERT_");
> > +    fullNickname += userNickname;
> 
> Not sure if this is a concern because I can't find the rest of this file,
> but do we need to do some validation (length, metacharacters etc) on
> aNickname here (above).  I assume that this comes from content?

The patch is based on uncheckin-ed bug 917102, and Nickname do come from content page, the meta-characters is already handled by gaia, or should we double check in gecko?

Comment 19

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8361510 [details] [diff] [review]
Gecko Path V2

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

The code looks good to me, but I think we should use "ca_cert"/"user_cert" instead of "wapi_as_cert"/"wapi_user_cert" in central.
if that can't be done, I think this patch is better go to fugu-specific branch instead of central.

And the modification involving NSS has to be reviewed by brian.

::: dom/wifi/WifiWorker.js
@@ +1003,5 @@
>    var networkConfigurationFields = [
>      "ssid", "bssid", "psk", "wep_key0", "wep_key1", "wep_key2", "wep_key3",
>      "wep_tx_keyidx", "priority", "key_mgmt", "scan_ssid", "disabled",
>      "identity", "password", "auth_alg", "phase1", "phase2", "eap", "pin",
> +    "pcsc", "ca_cert", "proto", "wapi_as_cert", "wapi_user_cert"

I have read the source code about WAPI config, it's a great work, but I didn't find it's necessary to use "wapi_as_cert"/"wapi_user_cert" instead of "ca_cert"/"user_cert".
Leveraging "ca_cert"/"user_cert" can reduce the changes in Gaia, also provide better compatibility when porting to other platform(and future releases of Firefox OS).
Also, it's seems that there's no standard or well-accepted way of implementing WAPI in wpa_supplicant. If we accept this implementation into central, we might have to support various implementations in the future. This leads to a compatibility issue.
So I think this patch is better go to fugu-specific branch instead of central.

Also, I like to suggest to read certificates into memory directly instead of read into a temporary cert file the read back right away[1].

[1] https://github.com/Dopi/JetPlatform/blob/master/delta/external/wpa_supplicant/wpa_supplicant.c#L1619  The referenced code is identical to fugu's code.

Comment 20

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8360962 [details] [review]
Gaia pull request

Cancel the review at first as the gecko part seems not ready currently.

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8361510 [details] [diff] [review]
Gecko Path V2

Chuck, could you please contact Li Gong and/or Andreas Gal about this? I had a discussion with them about WAPI and they indicated to me that we should not implement WAPI stuff directly, but instead we should have the partner that is doing China-specific customizations handle this. Please ask them about the email thread "WAPI in B2G" started by Brian Smith, January 20-27.

Comment 22

[Kai-Chih Hu [:khu]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #21)
> Comment on attachment 8361510 [details] [diff] [review]
> Gecko Path V2
> 
> Chuck, could you please contact Li Gong and/or Andreas Gal about this? I had
> a discussion with them about WAPI and they indicated to me that we should
> not implement WAPI stuff directly, but instead we should have the partner
> that is doing China-specific customizations handle this. Please ask them
> about the email thread "WAPI in B2G" started by Brian Smith, January 20-27.

IMO, we may need to discuss among TAMs first. Thank you for your information, Brian.

Comment 23

[Kai-Chih Hu [:khu]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #21)
> Comment on attachment 8361510 [details] [diff] [review]
> Gecko Path V2
> 
> Chuck, could you please contact Li Gong and/or Andreas Gal about this? I had
> a discussion with them about WAPI and they indicated to me that we should
> not implement WAPI stuff directly, but instead we should have the partner
> that is doing China-specific customizations handle this. Please ask them
> about the email thread "WAPI in B2G" started by Brian Smith, January 20-27.

Brian, can you loop me into this mail thread? I can't find this email from my INBOX. Thank you.

Comment 24

[BMO Automation]

Firefox OS is not being worked on