Closed Bug 1020212 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Wifi, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

VERIFIED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: gchang, Assigned: chucklee)

References

Details

(Whiteboard: [p=13])

Attachments

(2 files, 3 obsolete files)

### 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
It seems
Flags: needinfo?(chulee)
It seems that device can't read the certificate from storage.
Assignee: nobody → chulee
Flags: needinfo?(chulee)
(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.
blocking-b2g: --- → 2.0?
triage: 2.0+
blocking-b2g: 2.0? → 2.0+
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
blocking-b2g: 2.0+ → 2.0?
typo in comment 5, "wifi certificate service" should be "keystore service"
Sandip can you provide more details on this bug as Product.
Flags: needinfo?(skamat)
Changing to Ravi for wifi
Flags: needinfo?(skamat) → needinfo?(rdandu)
Vishy mentioned in triage that the product call here is to block here.
blocking-b2g: 2.0? → 2.0+
Flags: needinfo?(rdandu)
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
Flags: needinfo?(youlong.jiang)
Francis, when can we get a image to test?
Flags: needinfo?(frlee)
hi All,

T2M should be able to release the test image to Moz today. i shall keep you posted.
Flags: needinfo?(frlee)
Attached patch Add binder support. (obsolete) — Splinter Review
Support getting certificate by binder for Android 4.3 and later.

This patch has been tested on Flame with code from mozilla-central.
Attachment #8447875 - Flags: review?(kyle)
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.
Flags: needinfo?(youlong.jiang)
(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.
(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....
(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....
Attachment #8447875 - Flags: review?(kyle) → review+
(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.
This is great job! Thanks Chuck!
Target Milestone: --- → 2.0 S5 (4july)
Attached patch Support keystore binder. V2 (obsolete) — Splinter Review
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!
Attachment #8447875 - Attachment is obsolete: true
Attachment #8448549 - Flags: review?(kyle)
(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>
hi all,

T2M has provided the test image below: https://mozilla.app.box.com/files/0/f/2152284098/Test_image
please have a try.
Attachment #8448549 - Flags: review?(kyle) → review+
Attached patch Add binder support. V3. r=qdot (obsolete) — Splinter Review
Make binder compatible with kitkat.
Attachment #8448549 - Attachment is obsolete: true
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.
https://hg.mozilla.org/mozilla-central/rev/a92d1dd24860
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Verified @
Gaia      ca022f811bcbbda0f89086094a9e92bb220fea18
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/376889ab0e02
BuildID   20140713160202
Version   32.0a2
Status: RESOLVED → VERIFIED
Blocks: 1047294
You need to log in before you can comment on or make changes to this bug.