Remove Launchmode to create editNoteActivity in same task stack

[JonasMayerDev]

Bug described in issue #2124 was introduced by #2018 and can be seen in this Video:

master.mp4

When removing launchmode this specific bug is fixed (can be seen in this video) but it may introduce new bugs that should be collected here:

pr.mp4

For the reviewers

Please test carefully behaviours where new activities are started or switched!

[JonasMayerDev]

Still creates a new task stack when creating a note by selecting text, click "Share" and select "New Note"

[larena1]

Still creates a new task stack when creating a note by selecting text, click "Share" and select "New Note"

But I think that's absolutely how it should be so other apps can't hijack the stack and that's also what your initial fix intended to mitigate, right? I'm using it like this for the past two weeks and it's all good.

[alperozturk96]

This change makes EditNoteActivity vulnerable against the task hijacking. We should use singleInstance and set taskAffinity to empty string. I propose to do it in other PR and close this PR without merging.

Please read documentation

Launch Modes Explanation

Task Hijacking In Android

Task Hijacking Demo

Why we need to use singleInstance and empty taskAffinity?

Link 1

Link 2

[larena1]

@alperozturk96 I don't think it is necessary to have both singleInstace + taskAffinity.

Recommendation

Different forms of Task Hijacking vulnerabilities require different fixes:

    Set the task affinity of the application activities to ""(empty string) in the <activity> tag of the AndroidManifest.xml to force the activities to use a randomly generated task affinity, or set it at the<application> tag to enforce on all activities in the application.

OR

    Set the android:launchMode to singleInstance. singleInstance ensure that no other activities will be created in the same task.

    Do not specify launch mode set to singleTask or add support for a monitoring service to detect the presence of malicious foreground tasks.

    Do not set the flag FLAG_ACTIVITY_NEW_TASK in activity launch intents, or use with the FLAG_ACTIVITY_CLEAR_TASK:

The docs https://docs.ostorlab.co/kb/APK_TASK_HIJACKING/index.html you linked in the original PR also clearly state OR and not AND. Do you have any actual experience with this or you just repeat what others write? Only on your "Link 1" it is stated that both is needed but who knows if that is correct.

I fail to see how another app could sneak into this app's back stack when it does not have the same taskAffinity but you can probably explain that to us, @alperozturk96?

@tobiasKaminsky @AndyScherzinger do you maybe have an opinion on this?

[alperozturk96]

@alperozturk96 I don't think it is necessary to have both singleInstace + taskAffinity.

Recommendation

Different forms of Task Hijacking vulnerabilities require different fixes:

    Set the task affinity of the application activities to ""(empty string) in the <activity> tag of the AndroidManifest.xml to force the activities to use a randomly generated task affinity, or set it at the<application> tag to enforce on all activities in the application.

OR

    Set the android:launchMode to singleInstance. singleInstance ensure that no other activities will be created in the same task.

    Do not specify launch mode set to singleTask or add support for a monitoring service to detect the presence of malicious foreground tasks.

    Do not set the flag FLAG_ACTIVITY_NEW_TASK in activity launch intents, or use with the FLAG_ACTIVITY_CLEAR_TASK:

The docs https://docs.ostorlab.co/kb/APK_TASK_HIJACKING/index.html you linked in the original PR also clearly state OR and not AND. Do you have any actual experience with this or you just repeat what others write? Only on your "Link 1" it is stated that both is needed but who knows if that is correct.

I fail to see how another app could sneak into this app's back stack when it does not have the same taskAffinity but you can probably explain that to us, @alperozturk96?

@tobiasKaminsky @AndyScherzinger do you maybe have an opinion on this?

Firstly, I am not security expert or hacker or task hijacker expert and might be wrong. I just researched wanted to share for all of us and saw proof of concepts. If you set launchMode to singleTask, you can perform task hijacking, and it's not difficult to do it. Our source code is open to everyone, which means that attackers can read the code and might try to develop against that code. Vice versa, with the warnings or help of others, we can recognize vulnerabilities earlier and fix it.

Gained real experiences can either be following the path others have opened, or no one has encountered that problem before, and you might need to find something new and pave the way yourself. Even as you write these sentences, you're drawing inspiration from various sources and echoing or repeating others. The comments posted here are not unalterable truths.

Let's get back to the issue. The name of the vulnerabilities is StrandHogg, and two different version exists of it. v1 may affect up to Android 10 and v2 may affect up to Android 9. Since our minimum supported Android version is 7 we have to prevent it.

What's singleInstance?

The "singleInstance" mode also differs from "singleTask" and "singleInstancePerTask" in only one respect: an activity with the "singleTask" or "singleInstancePerTask" launch mode lets other activities, necessarily "standard" and "singleTop" activities, be part of its task.

A "singleInstance" activity, on the other hand, permits no other activities to be part of its task. It must be the only activity in the task. If it starts another activity, that activity is assigned to a different task, as if FLAG_ACTIVITY_NEW_TASK were in the intent.

Source, official android documentation

Why we need to set taskAffinity to ""?

If you set android:taskAffinity="" for all exported activities, you'll lose the ability to utilize features that require managing multiple tasks concurrently within your application.

Official Android Documentation about StrandHogg Attack and you can see the in the bottom resources, and you can read original academic paper and learn more about it, like I did.

[larena1]

You're again trying to explain launchMode by citing docs and all that and not answering my question regarding taskAffinity.

Regarding Strand Hogg: That was first described in 2015 (v1) and v2 a few years later and there Google writes:

Mitigations

Update to android:minSdkVersion="28".

The StrandHogg attack / Task affinity vulnerability was patched in March 2019 and Android SDK versions 28 and newer (Android 9) contain the appropriate OS patches to avoid this vulnerability. While it is possible to partially mitigate version 1 of the StrandHogg attack through individual application configuration, version 2 of the attack can only be prevented by this SDK version patch.

SDK Version 28 was introduced with Android 9 and we are about to see 15 released later this year.

Also regarding Strand Hogg from https://www.guardsquare.com/blog/protecting-against-strandhogg:

The Android Developer Docs [recommend against using android:launchMode=”singleInstance”](https://developer.android.com/guide/topics/manifest/activity-element#lmode) since it has major implications for user interaction. See bonus section 1 at the bottom of this post for the details.

So I can't quite comprehend why you're making such a big deal of a vulnerability that was found and fixed ages ago and then even suggest things that Google recommends against. I don't doubt you only mean good but it's not always helpful to just post random stuff that you find online and don't even understand.

[alperozturk96]

You're again trying to explain launchMode by citing docs and all that and not answering my question regarding taskAffinity.

Regarding Strand Hogg: That was first described in 2015 (v1) and v2 a few years later and there Google writes:

Mitigations

Update to android:minSdkVersion="28".

The StrandHogg attack / Task affinity vulnerability was patched in March 2019 and Android SDK versions 28 and newer (Android 9) contain the appropriate OS patches to avoid this vulnerability. While it is possible to partially mitigate version 1 of the StrandHogg attack through individual application configuration, version 2 of the attack can only be prevented by this SDK version patch.

SDK Version 28 was introduced with Android 9 and we are about to see 15 released later this year.

Also regarding Strand Hogg from https://www.guardsquare.com/blog/protecting-against-strandhogg:

The Android Developer Docs [recommend against using android:launchMode=”singleInstance”](https://developer.android.com/guide/topics/manifest/activity-element#lmode) since it has major implications for user interaction. See bonus section 1 at the bottom of this post for the details.

So I can't quite comprehend why you're making such a big deal of a vulnerability that was found and fixed ages ago and then even suggest things that Google recommends against. I don't doubt you only mean good but it's not always helpful to just post random stuff that you find online and don't even understand.

I didn't see anything on developer.android.com about not using singleInstance against strandhogg. defence techniques source guardsquare

You are doing exactly what you are complaining about. You are sharing links that you don't understand and that you copied left and right. Please read the academic paper and get your answers instead of replying quickly against to me.

I am not trying to make anything sound like a big problem and I don't want to convince you of that.

I think this conversation has gone beyond the sharing of information. Please don't prolong the conversation by replying to me. We shared our ideas, thank you for the conversation.

[larena1]

I didn't see anything on developer.android.com about not using singleInstance against strandhogg

Maybe you didn't understand, Google generally does not recommend using singleInstance. You might've missed it but it's actually also in the docs you shared with us.

From https://developer.android.com/guide/topics/manifest/activity-element:

The other modes, "singleTask" , "singleInstance", and "singleInstancePerTask", are not appropriate for most applications. They result in an interaction model that is likely to be unfamiliar to users and is very different from most other applications. 

You are doing exactly what you are complaining about. You are sharing links that you don't understand and that you copied left and right. Please read the academic paper and get your answers instead of replying quickly against to me.

I don't really see me suggesting unrecommended things that provide terrible UX and that I don't understand to fix vulnerabilites discovered almost 10 years ago and fixed in SDK 28 nearly 6 years ago, which as of today more than 90% of Android devices are on.

I really do hope you read through your resources more thoroughly in the future and don't come up with issues that affect Android 7 or prior next.

@JonasMayerDev Your taskAffinity approach that I also suggested should be more than enough. The vulnerability @alperozturk96 dug up from the depths of the internet does, according to Google, not need mitigation from SDK 28 and up (more than 90% of devices) but setting taskAffinity additionally does not hurt.

[alperozturk96]

I didn't see anything on developer.android.com about not using singleInstance against strandhogg

Maybe you didn't understand, Google generally does not recommend using singleInstance. You might've missed it but it's actually also in the docs you shared with us.

From https://developer.android.com/guide/topics/manifest/activity-element:

The other modes, "singleTask" , "singleInstance", and "singleInstancePerTask", are not appropriate for most applications. They result in an interaction model that is likely to be unfamiliar to users and is very different from most other applications. 

You are doing exactly what you are complaining about. You are sharing links that you don't understand and that you copied left and right. Please read the academic paper and get your answers instead of replying quickly against to me.

I don't really see me suggesting unrecommended things that provide terrible UX and that I don't understand to fix vulnerabilites discovered almost 10 years ago and fixed in SDK 28 nearly 6 years ago, which as of today more than 90% of Android devices are on.

I really do hope you read through your resources more thoroughly in the future and don't come up with issues that affect Android 7 or prior next.

@JonasMayerDev Your taskAffinity approach that I also suggested should be more than enough. The vulnerability @alperozturk96 dug up from the depths of the internet does, according to Google, not need mitigation from SDK 28 and up (more than 90% of devices) but setting taskAffinity additionally does not hurt.

We must support down to API level 24. We have enterprise clients and users using old devices. There is a single Activity design pattern that we can apply and increase performance for our app. Therefore, we will have much better navigation in the future, and it's already in our future plans.

Our min sdk level again is 24. Therefore and unfortunately, we should use singleInstance for now. We cannot ignore old OS versions.

Please understand first when sharing information or presenting an idea. Even after these messages, I see that you still do not have a full grasp of the subject. And you are promoting and offering vulnerable application for our enterprise clients and users that have old OS versions, which is unacceptable.

[larena1]

I didn't see anything on developer.android.com about not using singleInstance against strandhogg

Maybe you didn't understand, Google generally does not recommend using singleInstance. You might've missed it but it's actually also in the docs you shared with us.
From https://developer.android.com/guide/topics/manifest/activity-element:

The other modes, "singleTask" , "singleInstance", and "singleInstancePerTask", are not appropriate for most applications. They result in an interaction model that is likely to be unfamiliar to users and is very different from most other applications. 

You are doing exactly what you are complaining about. You are sharing links that you don't understand and that you copied left and right. Please read the academic paper and get your answers instead of replying quickly against to me.

I don't really see me suggesting unrecommended things that provide terrible UX and that I don't understand to fix vulnerabilites discovered almost 10 years ago and fixed in SDK 28 nearly 6 years ago, which as of today more than 90% of Android devices are on.
I really do hope you read through your resources more thoroughly in the future and don't come up with issues that affect Android 7 or prior next.
@JonasMayerDev Your taskAffinity approach that I also suggested should be more than enough. The vulnerability @alperozturk96 dug up from the depths of the internet does, according to Google, not need mitigation from SDK 28 and up (more than 90% of devices) but setting taskAffinity additionally does not hurt.

We must support down to API level 24. We have enterprise clients and users using old devices. There is a single Activity design pattern that we can apply and increase performance for our app. Therefore, we will have much better navigation in the future, and it's already in our future plans.

Our min sdk level again is 24. Therefore and unfortunately, we should use singleInstance for now. We cannot ignore old OS versions.

Please understand first when sharing information or presenting an idea. Even after these messages, I see that you still do not have a full grasp of the subject. And you are promoting and offering vulnerable application for our enterprise clients and users that have old OS versions, which is unacceptable.

You seem to have trouble reading and comprehending but let me help you once more.

From https://developer.android.com/privacy-and-security/risks/strandhogg

While it is possible to partially mitigate version 1 of the StrandHogg attack through individual application configuration, version 2 of the attack can only be prevented by this SDK version patch.

According to Google, it is only possible to mitigate v1 but not v2, leaving your customers unprotected and your application vulnerable, which you said is unacceptable. Instead, you should rather urge them to throw away their 8 year old devices as they can not be used safely anymore today with an internet connection because there are a lot more vulnerabilities than just Strand Hogg.

I think most other apps from Nextcloud are currently unprotected because they're not using singleInstance but I'll happily help out and create the respective PRs.

Thanks @alperozturk96 for pointing us to this

[jancborchardt]

Hi @larena1, please note that to have a nice working environment in our project we have a Code of Conduct that we ask everyone to observe: https://nextcloud.com/contribute/code-of-conduct/
Especially in online communication things can come across differently and more direct than in person and it is especially important to be friendly.
Thanks for your understanding and for contributing.

[larena1]

@jancborchardt thanks for mentioning that but you somehow only addressed me and not Ozturk while there was not really a difference in our communication.

[jancborchardt]

@larena1 as far as I can see, it started with a remark from this comment and escalated from there:

Do you have any actual experience with this or you just repeat what others write?

#2163 (comment)

[larena1]

@jancborchardt you are right in that I could have phrased that a bit differently and I'm sorry for that.

I want to thank @alperozturk96 for bringing this vulnerability to our attention and I understand now why we have to live with the way the notes app currently is due to the efforts to mitigate half of the vulnerability. It's a small price to pay for improved security and I'm sure nobody wants to put their notes at risk when they might contain sensible information.