ge0rg / MemorizingTrustManager

A "plugin" for Android Java to allow asking the user about SSL certificates
MIT License
182 stars 91 forks source link

interactResult: aborting due to stale decision reference! #52

Open rfc2822 opened 8 years ago

rfc2822 commented 8 years ago

Hello,

We have seen increasing problems with MTM in DAVdroid. Honestly, I don't know how to start debugging (except from dissecting the whole code line for line).

The problem is that MTM doesn't remember the "Always" click in the "Accept certificate?" dialog. As far as I have found out, it's not a key store problem, but related to how MTM GUI works (maybe some kind of thread synchronization problem?). DAVdroid uses a global (process-wide) MTM singleton in various components (Activities, Services etc.) and various threads. There are also up to two processes (a normal process and a :sync process), so there might be two singletons sharing the same key store.

When MTM is called by the customized okhttp HttpClient, it correctly shows the "Accept certificate?" dialog. However, as soon as I click the "Always", button, I see this log message:

E/MemorizingTrustManager: interactResult: aborting due to stale decision reference!

and MTM ignores the click.

Do you have any idea what this could be or how I could start debugging it?

Flowdalic commented 8 years ago

Just for the record, the relevant log line is https://github.com/ge0rg/MemorizingTrustManager/blob/d05a9a53a61d3ca028d7ba201ce287b8ab04c8d5/src/de/duenndns/ssl/MemorizingTrustManager.java#L714

After a first glance, this could very well be caused by using multiple MTM instances in different processes that do not share the same openDecisions data structure.

ge0rg commented 8 years ago

This really looks like a multi-process issue to me. Initially I thought it might be a problem with multi-threaded access to openDecisions, but reading and writing on that structure is synchronized.

A regular SSLSocket must be blocked until the user has decided, and the openDecisions entries are used as semaphores for that. If you have two processes, you should give each of them its own MTM and let the according instance handle the UI. Unfortunately, it won't synchronize the trust stores then.

rfc2822 commented 8 years ago

If you have two processes, you should give each of them its own MTM and let the according instance handle the UI. Unfortunately, it won't synchronize the trust stores then.

This is of course the case in DAVdroid. Every Application is of course per process. How could it even be possible to share a MTM between processes without IPC/RPC?

Unfortunately, it won't synchronize the trust stores then.

Yes, that would require file-based locking. However, that would not be that important. The problem is that the user-decision is just ignored.

Maybe the :sync process calls the MTMActivity of the other process, which doesn't have access to the MTMdecisions (because it resides in another process)? I guess that the Activity, which is an Android component and thus can be in any process, is (unwantedly) used as IPC and connects the two MTMs of the two processes somehow. (Any Android component can be in any process, there's no guarantee that all activities, services etc. of an app are in the same process.)

rfc2822 commented 8 years ago

In the second process, MTM seems to wait for a response forever.

This is the problem:

  1. Given that there are two processes (let's call them davdroid and davdroid:sync),
  2. one process (davdroid) registers an MTM, does some network and some GUI and then starts (better: lets Android start) a (synchronization) worker thread which runs in a second process davdroid:sync.
  3. The second process gets its own MTM. When a decision is required (because the cert has changed), it starts an MemorizingActivity, but the activity is started in the first process (because it doesn't have a process attribute in the AndroidManifest).
  4. So, the MemorizingActivity is shown in the context of the first process. When the user clicks on a button, the response can't be processed (see log message above) because this decision ID is unknown in the first process.
  5. The user decision isn't delivered to the second process (because it's processed in the first process). This is why the MTM of the second process waits forever.

Possible solution:

ge0rg commented 8 years ago

MTM starts its UI by means of a startActivity call that is posted to the original Context's getMainLooper():

https://github.com/ge0rg/MemorizingTrustManager/blob/master/src/de/duenndns/ssl/MemorizingTrustManager.java#L652-L670

I could imagine that for the davdroid:sync process, this leads to the startActivity actually being called on the original process and not on the sync process. However, I have no idea why or how that might actually be happening if you have a second MTM instance.

rfc2822 commented 8 years ago

I could imagine that for the davdroid:sync process, this leads to the startActivity actually being called on the original process and not on the sync process.

Indeed, it does.

However, I have no idea why or how that might actually be happening if you have a second MTM instance.

startActivity starts the activity in the process for which the activity is declared in the AndroidManifest <activity>. If there's no process attribute in the <activity> declaration, the activity will run in the default process.

Adding process=':sync' to the <activity> declaration would cause the activity to run in the :sync process, but then the same problem would happen in the other direction (when the main process MTM calls for a decision).

I have verified that this is the problem by temporarily removing the process=':sync' attributes for the sync threads, and then it works.

In my opinion, this can only be handled by either not using multiple processes (:-1:) or using IPC/services (:+1:).

rfc2822 commented 8 years ago

Adding android:multiprocess="true" to the <activity> declaration in the manifest seems to fix the problem:

<activity
            android:name="de.duenndns.ssl.MemorizingActivity"
            android:theme="@android:style/Theme.Holo.Light.Dialog.NoActionBar"
            android:multiprocess="true"/>

allows MemorizingActivity to be launched the the current process instead of the process where it was declared.

rfc2822 commented 8 years ago

Would you accept a patch that defines a new service (which always runs in the defined process), and changes the MTM UI so that it accesses this service? This should solve problems for multi-process environments.