1020212 - [Wifi] Device keeps trying to connect to network when connect to WPA-EAP TTLS network using server certificate

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Gerry Chang [:gchang]]

Attachment: The logs of keep trying to connect to network

### Steps:
1. Tap "Manage certificates" > "Import certificate"
2. Select the certificate from sd card
3. Navigate to Settings > Wi-Fi page
4. Choose the network supporting WPA-EAP TTLS
5. Select "TTLS" in EAP method
6. Type username and password
7. Select the certificate imported in step 2
8. Tap "OK" button

### Expected:
Device should be able to connect to WPA-EAP TTLS network

### Actual:
Device failed to connect and keep trying to connect to the wireless network

### Reproduce rate
always

### Version:
Device    flame
Gaia      1d4f6f7312882e78b57971152de75d1281a26187
Gecko     https://hg.mozilla.org/mozilla-central/rev/668f29cd71b3
BuildID   20140603160202
Version   32.0a1

Comment 1

[Gerry Chang [:gchang]]

It seems

Comment 2

[Gerry Chang [:gchang]]

It seems that device can't read the certificate from storage.

Comment 3

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

(In reply to Gerry Chang [:cfchang] from comment #2)
> It seems that device can't read the certificate from storage.

It seems that flame uses built-in keystore instead of keystore in gecko.

Comment 4

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: 2.0+

Comment 5

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

It seems that Android has changed the access method to keystore from unix socket[1] to binder[2], this should be why Flame uses android keystore instead of gecko keystore.
This is a big change and I need time to study and test how to support both method binder in current wifi certificate service. So I don't think this can be fixed in gecko by 2.0 release.

A fast workaround might be replace keystore_get.h[3] to previous version[2] to use unix socket again.
This involves gonk change, and I am not sure if this real works.

[1] http://androidxref.com/4.2_r1/xref/system/security/keystore/keystore.cpp#1442
[2] http://androidxref.com/4.3_r2.1/xref/system/security/keystore/keystore.cpp#2173
[3] http://androidxref.com/4.3_r2.1/xref/system/security/keystore/include/keystore/keystore_get.h
[4] http://androidxref.com/4.2_r1/xref/system/security/keystore/keystore_get.h

Comment 6

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

typo in comment 5, "wifi certificate service" should be "keystore service"

Comment 7

[Jean Gong :jgong]

Sandip can you provide more details on this bug as Product.

Comment 8

[Sandip Kamat]

Changing to Ravi for wifi

Comment 9

[Jason Smith [:jsmith]]

Vishy mentioned in triage that the product call here is to block here.

Comment 10

[Francis Lee [:frlee]]

hi Youlong,

could you please kindly check comment 5? what we need here is: a test image for replacing keystore_get.h[3] to previous version[2] to use unix socket again.

would it be possible that T2M can help us in this case?

thank you very much in advance

Comment 11

[Ken Chang[:kenkai]]

Francis, when can we get a image to test?

Comment 12

[Francis Lee [:frlee]]

hi All,

T2M should be able to release the test image to Moz today. i shall keep you posted.

Comment 13

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

Attachment: Add binder support.

Support getting certificate by binder for Android 4.3 and later.

This patch has been tested on Flame with code from mozilla-central.

Comment 14

[youlong.jiang]

hi guys,

    I wanna to double check with you that which file should be replace.
I find that keystore_get.h[3] in comment#5 is same with our version.
if I replace keystore_get.h[4] into version, some build error happen.

====================================================================

system/security/keystore/include/keystore/keystore_get.h:42:44: error: narrowing conversion of '(length >> 8)' from 'int' to 'uint8_t {aka unsigned char}' inside { } is ill-formed in C++11 [-Werror=narrowing]
system/security/keystore/include/keystore/keystore_get.h:42:44: error: narrowing conversion of 'length' from 'int' to 'uint8_t {aka unsigned char}' inside { } is ill-formed in C++11 [-Werror=narrowing]
system/security/keystore/include/keystore/keystore_get.h: At global scope:
system/security/keystore/include/keystore/keystore_get.h:40:12: error: 'int keystore_get(const char*, int, char*)' defined but not used [-Werror=unused-function]
cc1plus: all warnings being treated as errors
make: *** [out/target/product/msm8610/obj/SHARED_LIBRARIES/libkeystore_binder_intermediates/keystore_get.o] Error 1
make: *** Waiting for unfinished jobs....

====================================================================

tks.

Comment 15

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

(In reply to youlong.jiang from comment #14)
> hi guys,
> 
>     I wanna to double check with you that which file should be replace.
> I find that keystore_get.h[3] in comment#5 is same with our version.
> if I replace keystore_get.h[4] into version, some build error happen.
> 
> ====================================================================
> 
> system/security/keystore/include/keystore/keystore_get.h:42:44: error:
> narrowing conversion of '(length >> 8)' from 'int' to 'uint8_t {aka unsigned
> char}' inside { } is ill-formed in C++11 [-Werror=narrowing]

I think you can solve compile error by casting like
uint8_t bytes[2] = {(uint8_t)(length >> 8), (uint8_t)length};

> system/security/keystore/include/keystore/keystore_get.h:42:44: error:
> narrowing conversion of 'length' from 'int' to 'uint8_t {aka unsigned char}'
> inside { } is ill-formed in C++11 [-Werror=narrowing]
> system/security/keystore/include/keystore/keystore_get.h: At global scope:
> system/security/keystore/include/keystore/keystore_get.h:40:12: error: 'int
> keystore_get(const char*, int, char*)' defined but not used
> [-Werror=unused-function]
> cc1plus: all warnings being treated as errors
> make: ***
> [out/target/product/msm8610/obj/SHARED_LIBRARIES/
> libkeystore_binder_intermediates/keystore_get.o] Error 1
> make: *** Waiting for unfinished jobs....
> 
> ====================================================================
> 
> tks.

Comment 16

[youlong.jiang]

(In reply to Chuck Lee [:chucklee] from comment #15)
> (In reply to youlong.jiang from comment #14)
> > hi guys,
> > 
> >     I wanna to double check with you that which file should be replace.
> > I find that keystore_get.h[3] in comment#5 is same with our version.
> > if I replace keystore_get.h[4] into version, some build error happen.
> > 
> > ====================================================================
> > 
> > system/security/keystore/include/keystore/keystore_get.h:42:44: error:
> > narrowing conversion of '(length >> 8)' from 'int' to 'uint8_t {aka unsigned
> > char}' inside { } is ill-formed in C++11 [-Werror=narrowing]
> 
> I think you can solve compile error by casting like
> uint8_t bytes[2] = {(uint8_t)(length >> 8), (uint8_t)length};
> 
> > system/security/keystore/include/keystore/keystore_get.h:42:44: error:
> > narrowing conversion of 'length' from 'int' to 'uint8_t {aka unsigned char}'
> > inside { } is ill-formed in C++11 [-Werror=narrowing]
> > system/security/keystore/include/keystore/keystore_get.h: At global scope:
> > system/security/keystore/include/keystore/keystore_get.h:40:12: error: 'int
> > keystore_get(const char*, int, char*)' defined but not used
> > [-Werror=unused-function]
> > cc1plus: all warnings being treated as errors
> > make: ***
> > [out/target/product/msm8610/obj/SHARED_LIBRARIES/
> > libkeystore_binder_intermediates/keystore_get.o] Error 1
> > make: *** Waiting for unfinished jobs....
> > 
> > ====================================================================
> > 
> > tks.

hi chunck,

    also new issue happen, i've try to correct reference of interface, but no work. pls check it.

=========================================================

target SharedLib: libkeystore_binder (out/target/product/msm8610/obj/SHARED_LIBRARIES/libkeystore_binder_intermediates/LINKED/libkeystore_binder.so)
make[3]: warning: -jN forced in submake: disabling jobserver mode.
system/security/keystore/include/keystore/keystore_get.h:50: error: undefined reference to 'socket_local_client'
collect2: error: ld returned 1 exit status
make: *** [out/target/product/msm8610/obj/SHARED_LIBRARIES/libkeystore_binder_intermediates/LINKED/libkeystore_binder.so] Error 1
make: *** Waiting for unfinished jobs....

Comment 17

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

(In reply to youlong.jiang from comment #16)
> hi chunck,
> 
>     also new issue happen, i've try to correct reference of interface, but
> no work. pls check it.
> 
> =========================================================
> 
> target SharedLib: libkeystore_binder
> (out/target/product/msm8610/obj/SHARED_LIBRARIES/
> libkeystore_binder_intermediates/LINKED/libkeystore_binder.so)
> make[3]: warning: -jN forced in submake: disabling jobserver mode.
> system/security/keystore/include/keystore/keystore_get.h:50: error:
> undefined reference to 'socket_local_client'

I think comment out the include line[1] might do the work.

[1] http://androidxref.com/4.3_r2.1/xref/system/security/keystore/keystore_get.cpp#20

> collect2: error: ld returned 1 exit status
> make: ***
> [out/target/product/msm8610/obj/SHARED_LIBRARIES/
> libkeystore_binder_intermediates/LINKED/libkeystore_binder.so] Error 1
> make: *** Waiting for unfinished jobs....

Comment 18

[youlong.jiang]

(In reply to Chuck Lee [:chucklee] from comment #17)
> (In reply to youlong.jiang from comment #16)
> > hi chunck,
> > 
> >     also new issue happen, i've try to correct reference of interface, but
> > no work. pls check it.
> > 
> > =========================================================
> > 
> > target SharedLib: libkeystore_binder
> > (out/target/product/msm8610/obj/SHARED_LIBRARIES/
> > libkeystore_binder_intermediates/LINKED/libkeystore_binder.so)
> > make[3]: warning: -jN forced in submake: disabling jobserver mode.
> > system/security/keystore/include/keystore/keystore_get.h:50: error:
> > undefined reference to 'socket_local_client'
> 
> I think comment out the include line[1] might do the work.
> 
> [1]
> http://androidxref.com/4.3_r2.1/xref/system/security/keystore/keystore_get.
> cpp#20
> 
> > collect2: error: ld returned 1 exit status
> > make: ***
> > [out/target/product/msm8610/obj/SHARED_LIBRARIES/
> > libkeystore_binder_intermediates/LINKED/libkeystore_binder.so] Error 1
> > make: *** Waiting for unfinished jobs....

keystore.cpp of line[1] is same with our code version.

Comment 19

[howie [:howie]]

This is great job! Thanks Chuck!

Comment 20

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

Attachment: Support keystore binder. V2

Hi Kyle,
  I added permission check for binder part, I think it's better to review again.
  And thanks for your fast review on last patch!

Comment 21

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

(In reply to youlong.jiang from comment #18)
> (In reply to Chuck Lee [:chucklee] from comment #17)
> > (In reply to youlong.jiang from comment #16)
> > > hi chunck,
> > > 
> > >     also new issue happen, i've try to correct reference of interface, but
> > > no work. pls check it.
> > > 
> > > =========================================================
> > > 
> > > target SharedLib: libkeystore_binder
> > > (out/target/product/msm8610/obj/SHARED_LIBRARIES/
> > > libkeystore_binder_intermediates/LINKED/libkeystore_binder.so)
> > > make[3]: warning: -jN forced in submake: disabling jobserver mode.
> > > system/security/keystore/include/keystore/keystore_get.h:50: error:
> > > undefined reference to 'socket_local_client'
> > 
> > I think comment out the include line[1] might do the work.
> > 
> > [1]
> > http://androidxref.com/4.3_r2.1/xref/system/security/keystore/keystore_get.
> > cpp#20
> > 
> > > collect2: error: ld returned 1 exit status
> > > make: ***
> > > [out/target/product/msm8610/obj/SHARED_LIBRARIES/
> > > libkeystore_binder_intermediates/LINKED/libkeystore_binder.so] Error 1
> > > make: *** Waiting for unfinished jobs....
> 
> keystore.cpp of line[1] is same with our code version.

I mean, add "//" at head of line 20 to make it becoming a comment, like
//#include <keystore/keystore_get.h>

Comment 22

[Francis Lee [:frlee]]

hi all,

T2M has provided the test image below: https://mozilla.app.box.com/files/0/f/2152284098/Test_image
please have a try.

Comment 23

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

Attachment: Add binder support. V3. r=qdot

Make binder compatible with kitkat.

Comment 24

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

Attachment: Add binder support. V3.1, r=qdot

Fix nits.

Comment 25

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

Side note:
Although keystore_cli[1] and keystore_get[2](which is used by wpa_supplicant) both use |::NO_ERROR| to determine if get() is succeeded, the value of |::NO_ERROR| is different for keystore_cli(::NO_ERROR is 1) and keystore_get(::NO_ERROR is 0).
I don't know how this difference happens, and it seems that Android decides to return 0 in get function to make wpa_supplicant work. This also means keystore_cli can no longer be used to test get function of keystore after Android 4.3.

Comment 26

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

Reference link for comment 25:
[1] http://androidxref.com/4.4.3_r1.1/xref/system/security/keystore/keystore_cli.cpp#136
[2] http://androidxref.com/4.4.3_r1.1/xref/system/security/keystore/keystore_get.cpp#37

Comment 27

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

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=5745b74e0143

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a92d1dd24860

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a92d1dd24860

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b3b7dd57218

Comment 31

[Gerry Chang [:gchang]]

Verified @
Gaia      ca022f811bcbbda0f89086094a9e92bb220fea18
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/376889ab0e02
BuildID   20140713160202
Version   32.0a2