anyboo / seek-for-android

Automatically exported from code.google.com/p/seek-for-android
0 stars 0 forks source link

Incorrect behavior (SecurityException) after Open Session Failed on Out of Channel situation #83

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
On device utilizing GPAC control.
When an openSession is being called on out of channel situation, while the API 
call failed, it is also creating negative impact on another opened Session 
belong to another SEService (same or different client).  
In fact, the impact is so big that the client owning the no longer working 
opened Session must be destroy in order to gain access to SecureElement again.  
Creating another SEService without destroying the client would not work.

This behavior is highly undesired, because:

1) it is impossible for a client that has an opened Session to detect the 
session has rendered un-usable because of action done by someone else
2) once the issue occurs, client must be destroyed/fully exit and re-launch in 
order to gain access again.

The problem likely has to do with the implementation on handling Access Control 
refresh tag checking.  Where if any error occur on checking refresh tag by a 
different SEService instance, would render some kind of Access Control cache 
invalid.

Here is the step to reproduce it with one single Client.
1) create a SEService, service1
2) openSession against this service1's UICC reader
3) test openLogicalChannel against some applet on the UICC, make sure it works, 
then release the resource by closing this channel ONLY, but keep the session 
and SEService intact.
4) create another SEService, service2
5) openSession against this service2's UICC reader
6) occupy all the available channels, probably by utilizing a multiselectable 
applet in UICC
7) openSession again against this service2's UICC reader, this openSession is 
expected to fail
8) close service2
9) use the session on service1, openLogicalChannel against the applet previous 
success in step 3)

expected result:
a) step 9) would succeed.
b) another Service after release resource on Service1 would also have access to 
UICC without issue.

actual result:
a) step 9) failed with SecurityException
b) after releasing Service1 and its resources, access to UICC from the same 
client would failed without first fully destroy/exit the client.

Propose approach on this issue:
While GPAC asked for Access Control to invalid rule in case GPAC applet cannot 
be accessed or refresh tag is changed, it never really consider out of channel 
situation.  The current implementation likely invalid the Access Control cache 
whenever any error occurs.  but in reality, out of channel by client A should 
not cause another client B's working Session to become un-usable.

So, one possible approach could be handle out of channel situation differently 
during check refresh tag.  Consider in the situation where it is out of 
channel, openSession is already failing (IOException, null Session return), so 
the client asked for new Session would not be able to access SecureElement.  
There is no need to invalid the Access Control cache which would create 
negative impact for all other clients.

Original issue reported on code.google.com by etpcc...@gmail.com on 5 Sep 2014 at 10:01

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
IMPORTANT UPDATE
================

Upon further study, the root cause actually might have nothing to do with 
Access Control cache on the exact sequence to reproduce this error.  Instead, 
it has to do with step 8), where Shutdown of Service2 is not resulting in its 
first Session owning all the occupied channels to be released properly.

On the client side, the workaround on this issue is to keep reference to the 
session and explicitly call Session.closeChannels() or Session.close().

So, instead of this being a access control issue, it is instead issue on not 
cleaning up properly problem with SEService shutdown().

Original comment by etpcc...@gmail.com on 10 Sep 2014 at 3:52

GoogleCodeExporter commented 9 years ago
UPDATE
======

it has been identified that the following would close all the occupied channels 
probably

session.close();
session.closeChannels();
session.getReader().closeSessions();

but the following would not

session.getReader().getSEService().shutdown();

Based upon this knowledge, it has been identified that the root cause is in 
SEService getReaders() function, where getReaders would dereference and create 
new Readers object everytime it is called, causing any opened Session with 
opened Channel not to be closed properly with SEService.shutdown();

Looking at the logic, it seems like implementation of getReaders is trying to 
detect newly added readers while removing those that no longer exist (unlikely 
during actual run time in reality).  Though, the following code could be 
dealing with the need and fixed this issue:

        //mReaders.clear();   // this is the issue <<<<<

        // new logic to add and remove the list of readers.

        Set<String> previousReaderNames = mReaders.keySet();
        for (String readerName : readerNames)
        {
            //Add new plug-in readers
            if (!mReaders.containsKey(readerName))
                mReaders.put(readerName, new Reader(this, readerName));
        }

        //Remove not-present readers
        List<String> readerNameList = Arrays.asList(readerNames);
        for (String preReaderName : preReaderNames)
        {
            if (!readerNameList.contains(preReaderName))
                mReaders.remove(preReaderName);
        }

Original comment by etpcc...@gmail.com on 15 Oct 2014 at 2:39