StrandHogg detection #4498

yostyle · 2021-11-18T01:05:10Z

No description provided.

github-actions · 2021-11-18T01:30:34Z

Unit Test Results

  92 files ±0   92 suites ±0 1m 13s ⏱️ +7s
162 tests ±0 162 ✔️ ±0 0 💤 ±0 0 ❌ ±0
524 runs ±0 524 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit e9814ee. ± Comparison against base commit dd0d2e8.

♻️ This comment has been updated with latest results.

ouchadam · 2021-11-18T10:52:27Z

vector/src/main/AndroidManifest.xml

-        <activity android:name=".features.home.HomeActivity" />
+        <activity
+            android:name=".features.home.HomeActivity"
+            android:launchMode="singleTask" />


I tried adding singleTask to this activity for a deeplink issue and it caused a bunch of odd behaviours (skipping deeplinks, activity not fully loading), have you noticed anything strange after the change?

I didn't see any problem on HomeActivity after this change but I didn't stress it. I would prefer to make more tests.

Indeed, clicking on a link like https://app.element.io/#/room/%23element-web-announcements:matrix.org

creates 2 tasks if Element was already running, so we can see in the task switcher:

See #2299 and the fix #2341 :)

is this a blocker for the fix? maybe singleTop could provide similar protection without allowing multiple instances

No more needed to change launchMode since we generate a random task affinity on each build.

ouchadam · 2021-11-19T10:23:59Z

vector/src/main/java/im/vector/app/features/lifecycle/VectorActivityLifecycleCallbacks.kt

@@ -42,5 +58,81 @@ class VectorActivityLifecycleCallbacks constructor(private val popupAlertManager
    }

    override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) {
+        // restart the app if the task contains an unknown activity
+        if (isTaskCorrupted(activity)) {


would be interesting to know if there's a performance impact for this, might be worth making the check async to avoid slowing down the activity start flow

ouchadam · 2021-11-19T10:28:57Z

vector/src/main/java/im/vector/app/features/home/HomeActivity.kt

@@ -520,7 +521,16 @@ class HomeActivity :
        if (views.drawerLayout.isDrawerOpen(GravityCompat.START)) {
            views.drawerLayout.closeDrawer(GravityCompat.START)
        } else {
-            super.onBackPressed()
+            if (Build.VERSION.SDK_INT < Build.VERSION_CODES.R && supportFragmentManager.backStackEntryCount == 0) {


looks like we could extract this logic to reduce the duplication, either extension or base class function

something like

if (views.drawerLayout.isDrawerOpen(GravityCompat.START)) { views.drawerLayout.closeDrawer(GravityCompat.START) } else { validateBackPressed { super.onBackPressed() } } ... fun Activity.validateBackPressed(onBackPressed: () -> Unit) { if (Build.VERSION.SDK_INT < Build.VERSION_CODES.R && supportFragmentManager.backStackEntryCount == 0) { if (isTaskRoot) { onBackPressed() } else { Timber.e("Application is potentially corrupted by an unknown activity") finishAffinity() } } else { onBackPressed() } }

bmarty

See https://github.com/vector-im/element-android/pull/4498/files#r754454956

It's worth noting that the app seems robust to the StrandHogg vulnerability with the fix. I have the error log when I try the attack.

github-actions · 2022-02-23T14:25:53Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
passed="25" failures="17" errors="0" skipped="2"
[org.matrix.android.sdk.account]
passed="5" failures="0" errors="0" skipped="2"
[org.matrix.android.sdk.internal]
passed="62" failures="1" errors="0" skipped="28"
[org.matrix.android.sdk.ordering]
passed="16" failures="0" errors="0" skipped="0"
[org.matrix.android.sdk.PermalinkParserTest]
passed="2" failures="0" errors="0" skipped="0"

vector/src/main/java/im/vector/app/features/lifecycle/VectorActivityLifecycleCallbacks.kt

Florian14 · 2022-02-24T17:06:46Z

vector/src/main/java/im/vector/app/core/extensions/Activity.kt

@@ -114,6 +116,19 @@ fun AppCompatActivity.hideKeyboard() {
    currentFocus?.hideKeyboard()
 }

+fun AppCompatActivity.validateBackPressed(onBackPressed: () -> Unit) {


Please add documentation to explain that this should be only used for top-level activities. A developer which is not aware of this could think that it's a good practice to use it for all activities.

Funny: I had the same mental path "A developer which is not aware of this could think that it's a good practice to use it for all activities". And now I see this comment in GitHub, and the comment in the code :)

bmarty

Maybe add the suffix to the taskAffinity of VectorCallActivity?

bmarty · 2022-03-02T16:40:34Z

vector/build.gradle

+
+    return alphaCharset[rnd.nextInt(alphaCharset.length())] +
+            rnd.with {
+                (1..7).collect { alphaNumCharset[nextInt(alphaNumCharset.length())] }.join()


Maybe using 8 chars in [a-z] is enough :)

bmarty · 2022-03-02T16:45:00Z

vector/build.gradle

@@ -91,6 +91,17 @@ static def getFdroidVersionSuffix() {
    }
 }

+static def getTaskAffinitySuffix() {


Maybe rename to generateTaskAffinitySuffix

ouchadam · 2022-03-03T09:35:47Z

vector/build.gradle

@@ -131,6 +136,9 @@ android {
        // Required for sonar analysis
        versionName "${versionMajor}.${versionMinor}.${versionPatch}-sonar"

+        // Generate a random app task affinity
+        manifestPlaceholders = [appTaskAffinitySuffix:"${generateTaskAffinitySuffix()}"]


not a blocker since we're not using the build cache, wanted to highlight generating new values on each build/gradle configuration cycle may cause some of the android gradle tasks to become non cachable

Ah, something else I did not think about.
We want to have stable builds, i.e. the same code generates the same binary (related: #842, and context in element-hq/riot-android#3131). It's important for F-Droid.
With this change it will not be the case anymore.
I have not idea how to handle that now.

could we use the short git hash for the suffix? 🤔

We should propably compute the task affinity suffix from tags or commit hash ?

could we use the short git hash for the suffix? 🤔

We can't use it as is. taskAffinity has same format as namespace :

It must have at least two segments (one or more dots).

Each segment must start with a letter.

All characters must be alphanumeric or an underscore [a-zA-Z0-9_].

https://developer.android.com/studio/build/configure-app-module#set-application-id

we could use the git hash as a seed for the random()

That's a good idea to compute a suffix using git hash and just remove forbidden chars (but is there any?).

So many having a appTaskAffinitySuffix like a${git_hash_8_digits} with a a prefix to ensure it's always starting with a letter would fit all our needs. WDYT @yostyle ?

we already have gitRevision. I'm going to reuse it.

bmarty

Thanks for the update. As per my test, it's working well.
I will squash commits to clean up the history of this branch.

* develop: (392 commits) Missing import of at-Ignore annotation. FTUE - Choose a display picture (#5323) Ignore ThreadMessagingTest as it seems to cause other integration tests to fail. Maybe the file is here? I give up for the weekend Frustration at artifact handling vs what's in docs. Update the top bar in a room (#5213) Tweak upload/download of codecov xml file Address review points from adam Remove unneeded code, retaining a comment for how to exclude certain projects Merge pull request #5405 from vector-im/cgizard/ISSUE-5402 Remove the printing of file name to the log as it's doubling up information. Remove exclusions (for now). Fix typo in name of action giving avatar/display name error dialogs human readable error messages - reuses the ErrorDialog logic which translates exceptions to human readable strings Run codecoverage and pass to sonarqube upload for processing. Better codecov based on ouchadam's suggestion Correct name of environment variable Use environment variable that is tied to project property Merge pull request #4498 from vector-im/yostyle/fix_strandhogg ... # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/action/MessageActionsViewModel.kt

yostyle requested a review from bmarty November 18, 2021 01:05

yostyle force-pushed the yostyle/fix_strandhogg branch from 00bed50 to a3b4c31 Compare November 18, 2021 10:12

yostyle marked this pull request as ready for review November 18, 2021 10:23

ouchadam reviewed Nov 18, 2021

View reviewed changes

ouchadam reviewed Nov 19, 2021

View reviewed changes

bmarty requested changes Nov 22, 2021

View reviewed changes

StrandHogg detection

e454fab

yostyle force-pushed the yostyle/fix_strandhogg branch from a3b4c31 to e454fab Compare February 23, 2022 13:56

yostyle force-pushed the yostyle/fix_strandhogg branch from a08946e to 11c5769 Compare February 23, 2022 18:25

launch detection in async

647e22c

yostyle force-pushed the yostyle/fix_strandhogg branch from 11c5769 to 647e22c Compare February 23, 2022 21:44

Florian14 reviewed Feb 24, 2022

View reviewed changes

vector/src/main/java/im/vector/app/features/lifecycle/VectorActivityLifecycleCallbacks.kt Outdated Show resolved Hide resolved

Florian14 reviewed Feb 24, 2022

View reviewed changes

yostyle added 6 commits February 25, 2022 00:06

Add documentation

6013a29

Fix crash after update theme in app settings

14675a5

update app task affinity

f14dc23

Update strandhogg detection

fa17b83

Update exceptions management

15b3ef6

Restore manifest and fix task affinity suffix naming

885a861

yostyle force-pushed the yostyle/fix_strandhogg branch from d29f58d to 885a861 Compare March 2, 2022 15:59

bmarty reviewed Mar 2, 2022

View reviewed changes

Update app task affinity suffix

d9e3f60

ouchadam reviewed Mar 3, 2022

View reviewed changes

ouchadam approved these changes Mar 3, 2022

View reviewed changes

yostyle force-pushed the yostyle/fix_strandhogg branch from feab80d to a12f9b7 Compare March 3, 2022 13:38

Compute task affinity suffix

e9814ee

yostyle force-pushed the yostyle/fix_strandhogg branch from a12f9b7 to e9814ee Compare March 3, 2022 13:40

bmarty approved these changes Mar 3, 2022

View reviewed changes

bmarty merged commit 6e6b04c into develop Mar 3, 2022

bmarty deleted the yostyle/fix_strandhogg branch March 3, 2022 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StrandHogg detection #4498

StrandHogg detection #4498

yostyle commented Nov 18, 2021

github-actions bot commented Nov 18, 2021 •

edited

Loading

ouchadam Nov 18, 2021

yostyle Nov 18, 2021

bmarty Nov 22, 2021

ouchadam Feb 25, 2022

yostyle Mar 2, 2022

ouchadam Nov 19, 2021

yostyle Feb 23, 2022

ouchadam Nov 19, 2021

yostyle Feb 23, 2022

bmarty left a comment •

edited

Loading

github-actions bot commented Feb 23, 2022 •

edited

Loading

Florian14 Feb 24, 2022

yostyle Feb 24, 2022

bmarty Mar 3, 2022

bmarty left a comment

bmarty Mar 2, 2022

yostyle Mar 3, 2022

bmarty Mar 2, 2022

yostyle Mar 3, 2022

ouchadam Mar 3, 2022

bmarty Mar 3, 2022

ouchadam Mar 3, 2022 •

edited

Loading

yostyle Mar 3, 2022

yostyle Mar 3, 2022 •

edited

Loading

ouchadam Mar 3, 2022

bmarty Mar 3, 2022 •

edited

Loading

bmarty Mar 3, 2022

yostyle Mar 3, 2022

bmarty left a comment

StrandHogg detection #4498

StrandHogg detection #4498

Conversation

yostyle commented Nov 18, 2021

github-actions bot commented Nov 18, 2021 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2022 • edited Loading

Matrix SDK

Integration Tests Results:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yostyle Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 18, 2021 •

edited

Loading

bmarty left a comment •

edited

Loading

github-actions bot commented Feb 23, 2022 •

edited

Loading

ouchadam Mar 3, 2022 •

edited

Loading

yostyle Mar 3, 2022 •

edited

Loading

bmarty Mar 3, 2022 •

edited

Loading