From f0ce9d0347906706c6a7aff3a2922107c8d7ba5d Mon Sep 17 00:00:00 2001 From: Federico Iosue Date: Fri, 19 May 2023 08:31:55 +0200 Subject: [PATCH 1/5] Added path validation to Security util class to address security advisory https://github.com/federicoiosue/Omni-Notes/security/advisories/GHSA-g38r-4cf6-3v32 --- .../android/omninotes/utils/SecurityTest.kt | 88 +++++++++++++++++++ .../checked/ContentSecurityException.kt | 20 +++++ .../feio/android/omninotes/utils/Security.kt | 18 +++- 3 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 omniNotes/src/androidTest/java/it/feio/android/omninotes/utils/SecurityTest.kt create mode 100644 omniNotes/src/main/java/it/feio/android/omninotes/exceptions/checked/ContentSecurityException.kt diff --git a/omniNotes/src/androidTest/java/it/feio/android/omninotes/utils/SecurityTest.kt b/omniNotes/src/androidTest/java/it/feio/android/omninotes/utils/SecurityTest.kt new file mode 100644 index 000000000..05d4da440 --- /dev/null +++ b/omniNotes/src/androidTest/java/it/feio/android/omninotes/utils/SecurityTest.kt @@ -0,0 +1,88 @@ +/* + * Copyright (C) 2013-2023 Federico Iosue (federico@iosue.it) + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package it.feio.android.omninotes.utils + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import it.feio.android.omninotes.exceptions.checked.ContentSecurityException +import it.feio.android.omninotes.testutils.BaseAndroidTestCase +import it.feio.android.omninotes.utils.Security.Companion.decrypt +import it.feio.android.omninotes.utils.Security.Companion.encrypt +import it.feio.android.omninotes.utils.Security.Companion.validatePath +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertThrows +import org.junit.Test +import org.junit.runner.RunWith + +@RunWith(AndroidJUnit4::class) +class SecurityTest : BaseAndroidTestCase() { + private val LOREM = ("Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor" + + " incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco" + + " laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit " + + "esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa " + + "qui officia deserunt mollit anim ID est laborum.") + + @Test + @Throws(Exception::class) + fun checkUtilityClassWellDefined() { + assertUtilityClassWellDefined(Security::class.java) + } + + @Test + fun encrypt() { + assertNotEquals(TEXT, encrypt(TEXT, PASS)) + } + + @Test + fun decrypt() { + val encryptedText = encrypt(TEXT, PASS) + + assertEquals(TEXT, decrypt(encryptedText, PASS)) + assertNotEquals(TEXT, decrypt(encryptedText, "zaza$PASS")) + } + + @Test + fun decryptUnencrypted() { + assertNotEquals(0, decrypt(LOREM, PASS)!!.length.toLong()) + } + + @Test + fun validatePath_startsWithData() { + val path = "file:///data/data/it.feio.android.omninotes.foss/shared_prefs/it.feio.android.omninotes.foss_preferences.xml" + + assertThrows(ContentSecurityException::class.java) { validatePath(path) } + } + + @Test + fun validatePath_pathTraversal() { + val path = "file:///storage/emulated/0/../../../data/data/it.feio.android.omninotes.foss/shared_prefs/it.feio.android.omninotes.foss_preferences.xml" + + assertThrows(ContentSecurityException::class.java) { validatePath(path) } + } + + @Test + fun validatePath_valid() { + val path = "/images/screenshot/16844742322307525633366385236595.jpg" + + validatePath(path) + } + + companion object { + private const val PASS = "12345uselessPasswords" + private const val TEXT = "Today is a - good - day to test useless things!" + } +} \ No newline at end of file diff --git a/omniNotes/src/main/java/it/feio/android/omninotes/exceptions/checked/ContentSecurityException.kt b/omniNotes/src/main/java/it/feio/android/omninotes/exceptions/checked/ContentSecurityException.kt new file mode 100644 index 000000000..c5789debe --- /dev/null +++ b/omniNotes/src/main/java/it/feio/android/omninotes/exceptions/checked/ContentSecurityException.kt @@ -0,0 +1,20 @@ +/* + * Copyright (C) 2013-2023 Federico Iosue (federico@iosue.it) + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +package it.feio.android.omninotes.exceptions.checked + +class ContentSecurityException(message: String) : SecurityException() \ No newline at end of file diff --git a/omniNotes/src/main/java/it/feio/android/omninotes/utils/Security.kt b/omniNotes/src/main/java/it/feio/android/omninotes/utils/Security.kt index a8e1fda9f..97b63b7e7 100644 --- a/omniNotes/src/main/java/it/feio/android/omninotes/utils/Security.kt +++ b/omniNotes/src/main/java/it/feio/android/omninotes/utils/Security.kt @@ -17,15 +17,20 @@ package it.feio.android.omninotes.utils +import android.net.Uri import android.util.Base64 +import it.feio.android.omninotes.exceptions.checked.ContentSecurityException import it.feio.android.omninotes.helpers.LogDelegate import java.nio.charset.StandardCharsets import java.security.MessageDigest import java.security.NoSuchAlgorithmException -import javax.crypto.* +import javax.crypto.Cipher +import javax.crypto.SecretKeyFactory import javax.crypto.spec.DESKeySpec +import kotlin.jvm.Throws -class Security private constructor(){ + +class Security private constructor() { companion object { @JvmStatic @@ -82,5 +87,14 @@ class Security private constructor(){ } } + @JvmStatic + @Throws(ContentSecurityException::class) + fun validatePath(path: String?) { + val uri = Uri.parse(path).path + if (uri?.startsWith("/data")!! || uri.contains("../")) { + throw ContentSecurityException("Invalid") + } + } + } } \ No newline at end of file From bc3e8e0b9915bf1ff99e3ca1c81fb324272d2134 Mon Sep 17 00:00:00 2001 From: Federico Iosue Date: Sat, 20 May 2023 18:24:30 +0200 Subject: [PATCH 2/5] Secured path during attachments creation --- .../testutils/BaseAndroidTestCase.java | 5 ++--- .../android/omninotes/DetailFragment.java | 22 +++++++++++++++---- omniNotes/src/main/res/values/strings.xml | 1 + 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/omniNotes/src/androidTest/java/it/feio/android/omninotes/testutils/BaseAndroidTestCase.java b/omniNotes/src/androidTest/java/it/feio/android/omninotes/testutils/BaseAndroidTestCase.java index 4fef0f84f..b13f174ed 100644 --- a/omniNotes/src/androidTest/java/it/feio/android/omninotes/testutils/BaseAndroidTestCase.java +++ b/omniNotes/src/androidTest/java/it/feio/android/omninotes/testutils/BaseAndroidTestCase.java @@ -188,9 +188,8 @@ protected static void assertUtilityClassWellDefined(final Class clazz) } protected static void assertUtilityClassWellDefined(final Class clazz, - boolean weakClassModifier, - boolean weakConstructorModifier) - throws NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException { + boolean weakClassModifier, boolean weakConstructorModifier) + throws NoSuchMethodException, InstantiationException, IllegalAccessException { if (!weakClassModifier) { assertTrue("class must be final", Modifier.isFinal(clazz.getModifiers())); } diff --git a/omniNotes/src/main/java/it/feio/android/omninotes/DetailFragment.java b/omniNotes/src/main/java/it/feio/android/omninotes/DetailFragment.java index a7565bbdc..318b10c52 100644 --- a/omniNotes/src/main/java/it/feio/android/omninotes/DetailFragment.java +++ b/omniNotes/src/main/java/it/feio/android/omninotes/DetailFragment.java @@ -61,6 +61,7 @@ import static it.feio.android.omninotes.utils.ConstantsBase.SWIPE_MARGIN; import static it.feio.android.omninotes.utils.ConstantsBase.SWIPE_OFFSET; import static it.feio.android.omninotes.utils.ConstantsBase.THUMBNAIL_SIZE; +import static it.feio.android.omninotes.utils.Security.validatePath; import static java.lang.Integer.parseInt; import static java.lang.Long.parseLong; @@ -133,6 +134,7 @@ import it.feio.android.omninotes.async.notes.SaveNoteTask; import it.feio.android.omninotes.databinding.FragmentDetailBinding; import it.feio.android.omninotes.db.DbHelper; +import it.feio.android.omninotes.exceptions.checked.ContentSecurityException; import it.feio.android.omninotes.exceptions.checked.UnhandledIntentException; import it.feio.android.omninotes.helpers.AttachmentsHelper; import it.feio.android.omninotes.helpers.IntentHelper; @@ -167,6 +169,7 @@ import it.feio.android.omninotes.utils.KeyboardUtils; import it.feio.android.omninotes.utils.PasswordHelper; import it.feio.android.omninotes.utils.ReminderHelper; +import it.feio.android.omninotes.utils.Security; import it.feio.android.omninotes.utils.ShortcutHelper; import it.feio.android.omninotes.utils.StorageHelper; import it.feio.android.omninotes.utils.TagsHelper; @@ -551,7 +554,6 @@ private void handleIntents() { } private void importAttachments(Intent i) { - if (!i.hasExtra(Intent.EXTRA_STREAM)) { return; } @@ -559,19 +561,31 @@ private void importAttachments(Intent i) { if (i.getExtras().get(Intent.EXTRA_STREAM) instanceof Uri) { Uri uri = i.getParcelableExtra(Intent.EXTRA_STREAM); // Google Now passes Intent as text but with audio recording attached the case must be handled like this - if (!INTENT_GOOGLE_NOW.equals(i.getAction())) { + if (validatePath(uri.getPath()) && !INTENT_GOOGLE_NOW.equals(i.getAction())) { String name = FileHelper.getNameFromUri(mainActivity, uri); new AttachmentTask(this, uri, name, this).execute(); } } else { ArrayList uris = i.getParcelableArrayListExtra(Intent.EXTRA_STREAM); for (Uri uriSingle : uris) { - String name = FileHelper.getNameFromUri(mainActivity, uriSingle); - new AttachmentTask(this, uriSingle, name, this).execute(); + if (validatePath(uriSingle.getPath())) { + String name = FileHelper.getNameFromUri(mainActivity, uriSingle); + new AttachmentTask(this, uriSingle, name, this).execute(); + } } } } + private boolean validatePath(String path) { + try { + Security.validatePath(path); + return true; + } catch (ContentSecurityException e) { + mainActivity.showMessage(R.string.insecure_content_found, ONStyle.WARN); + return false; + } + } + @SuppressLint("NewApi") private void initViews() { // Sets onTouchListener to the whole activity to swipe notes diff --git a/omniNotes/src/main/res/values/strings.xml b/omniNotes/src/main/res/values/strings.xml index 254270288..f816b3b9c 100644 --- a/omniNotes/src/main/res/values/strings.xml +++ b/omniNotes/src/main/res/values/strings.xml @@ -231,6 +231,7 @@ Confirm undoing changes? Done Nothing selected + Some of the received content was dangerous, it has been ignored Displays some statistics and expiring reminders from Omni Notes From 6c82396b715b4e87bc9f19473e925d3ff728b619 Mon Sep 17 00:00:00 2001 From: Federico Iosue Date: Wed, 31 May 2023 01:16:23 +0200 Subject: [PATCH 3/5] Fixed attachments' count in statistics --- .../src/main/java/it/feio/android/omninotes/db/DbHelper.java | 3 +-- omniNotes/src/main/res/layout/activity_stats.xml | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/omniNotes/src/main/java/it/feio/android/omninotes/db/DbHelper.java b/omniNotes/src/main/java/it/feio/android/omninotes/db/DbHelper.java index 21bb4e99d..f039bde87 100644 --- a/omniNotes/src/main/java/it/feio/android/omninotes/db/DbHelper.java +++ b/omniNotes/src/main/java/it/feio/android/omninotes/db/DbHelper.java @@ -1046,7 +1046,6 @@ public Stats getStats() { mStats.setCharsAvg(avgChars); // Everything about attachments - int attachmentsAll = 0; int images = 0; int videos = 0; int audioRecordings = 0; @@ -1067,7 +1066,7 @@ public Stats getStats() { files++; } } - mStats.setAttachments(attachmentsAll); + mStats.setAttachments(attachments.size()); mStats.setImages(images); mStats.setVideos(videos); mStats.setAudioRecordings(audioRecordings); diff --git a/omniNotes/src/main/res/layout/activity_stats.xml b/omniNotes/src/main/res/layout/activity_stats.xml index 307d620a3..53e08fe23 100644 --- a/omniNotes/src/main/res/layout/activity_stats.xml +++ b/omniNotes/src/main/res/layout/activity_stats.xml @@ -271,7 +271,7 @@ android:gravity="end" android:paddingEnd="7dp" android:paddingStart="7dp" - android:text="@string/stats_attachments" + android:text="@string/stats_images" android:textStyle="bold"> Date: Wed, 31 May 2023 01:16:42 +0200 Subject: [PATCH 4/5] Version 6.2.7 (322) and updated changelog --- gradle.properties | 4 ++-- omniNotes/src/main/res/raw/changelog.xml | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/gradle.properties b/gradle.properties index 7899efa39..a69df043b 100644 --- a/gradle.properties +++ b/gradle.properties @@ -14,8 +14,8 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # -VERSION_NAME=6.2.6 -VERSION_CODE=321 +VERSION_NAME=6.2.7 +VERSION_CODE=322 PACKAGE=it.feio.android.omninotes MIN_SDK=21 TARGET_SDK=31 diff --git a/omniNotes/src/main/res/raw/changelog.xml b/omniNotes/src/main/res/raw/changelog.xml index cfb1232ef..42d6b6249 100644 --- a/omniNotes/src/main/res/raw/changelog.xml +++ b/omniNotes/src/main/res/raw/changelog.xml @@ -16,6 +16,20 @@ --> + + [u]Fix[/u] Security path traversal vulnerability + [u]Fix[/u] Attachments' count in statistics (thanks to S.Vago) + + + + [i]Improved![/i] Reduced required permissions and removed some legacy code + [u]Fix[/u] Minor backup related fixes + + From 3ffe381c61c58cc22ffa2d1a114dc8a4880eb70d Mon Sep 17 00:00:00 2001 From: Federico Iosue Date: Wed, 3 May 2023 16:39:53 +0200 Subject: [PATCH 5/5] Completelly removed legacy Butterknife dependency and usage --- omniNotes/build.gradle | 2 - .../android/omninotes/async/MainMenuTask.java | 43 ++++++++----------- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/omniNotes/build.gradle b/omniNotes/build.gradle index f069f79ce..3cb691353 100644 --- a/omniNotes/build.gradle +++ b/omniNotes/build.gradle @@ -173,8 +173,6 @@ dependencies { transitive = true exclude group: "com.android.support" } - implementation 'com.jakewharton:butterknife:10.2.1' - annotationProcessor 'com.jakewharton:butterknife-compiler:10.2.1' implementation('org.mnode.ical4j:ical4j:3.0.11') { exclude group: 'commons.io' } diff --git a/omniNotes/src/main/java/it/feio/android/omninotes/async/MainMenuTask.java b/omniNotes/src/main/java/it/feio/android/omninotes/async/MainMenuTask.java index 311873cee..2dbcb103d 100644 --- a/omniNotes/src/main/java/it/feio/android/omninotes/async/MainMenuTask.java +++ b/omniNotes/src/main/java/it/feio/android/omninotes/async/MainMenuTask.java @@ -24,17 +24,15 @@ import android.content.res.TypedArray; import android.os.AsyncTask; import androidx.fragment.app.Fragment; -import butterknife.BindView; -import butterknife.ButterKnife; import com.pixplicity.easyprefs.library.Prefs; import de.greenrobot.event.EventBus; import it.feio.android.omninotes.MainActivity; import it.feio.android.omninotes.R; import it.feio.android.omninotes.async.bus.NavigationUpdatedEvent; +import it.feio.android.omninotes.databinding.FragmentNavigationDrawerBinding; import it.feio.android.omninotes.models.NavigationItem; import it.feio.android.omninotes.models.adapters.NavDrawerAdapter; import it.feio.android.omninotes.models.misc.DynamicNavigationLookupTable; -import it.feio.android.omninotes.models.views.NonScrollableListView; import it.feio.android.omninotes.utils.Navigation; import java.lang.ref.WeakReference; import java.util.ArrayList; @@ -43,18 +41,15 @@ public class MainMenuTask extends AsyncTask> { - private final WeakReference mFragmentWeakReference; + private final WeakReference fragmentWeakReference; private final MainActivity mainActivity; - @BindView(R.id.drawer_nav_list) - NonScrollableListView mDrawerList; - @BindView(R.id.drawer_tag_list) - NonScrollableListView mDrawerCategoriesList; + FragmentNavigationDrawerBinding navDrawer; - public MainMenuTask(Fragment mFragment) { - mFragmentWeakReference = new WeakReference<>(mFragment); - this.mainActivity = (MainActivity) mFragment.getActivity(); - ButterKnife.bind(this, mFragment.getView()); + public MainMenuTask(Fragment fragment) { + fragmentWeakReference = new WeakReference<>(fragment); + mainActivity = (MainActivity) fragment.getActivity(); + navDrawer = FragmentNavigationDrawerBinding.inflate(fragment.getLayoutInflater()); } @Override @@ -65,33 +60,31 @@ protected List doInBackground(Void... params) { @Override protected void onPostExecute(final List items) { if (isAlive()) { - mDrawerList.setAdapter(new NavDrawerAdapter(mainActivity, items)); - mDrawerList.setOnItemClickListener((arg0, arg1, position, arg3) -> { - String navigation = mFragmentWeakReference.get().getResources().getStringArray(R.array + navDrawer.drawerNavList.setAdapter(new NavDrawerAdapter(mainActivity, items)); + navDrawer.drawerNavList.setOnItemClickListener((arg0, arg1, position, arg3) -> { + String navigation = fragmentWeakReference.get().getResources().getStringArray(R.array .navigation_list_codes)[items.get(position).getArrayIndex()]; updateNavigation(position, navigation); }); - mDrawerList.justifyListViewHeightBasedOnChildren(); + navDrawer.drawerNavList.justifyListViewHeightBasedOnChildren(); } } private void updateNavigation(int position, String navigation) { if (mainActivity.updateNavigation(navigation)) { - mDrawerList.setItemChecked(position, true); - if (mDrawerCategoriesList != null) { - mDrawerCategoriesList.setItemChecked(0, false); // Called to force redraw - } + navDrawer.drawerNavList.setItemChecked(position, true); + navDrawer.drawerTagList.setItemChecked(0, false); // Called to force redraw mainActivity.getIntent().setAction(Intent.ACTION_MAIN); EventBus.getDefault() - .post(new NavigationUpdatedEvent(mDrawerList.getItemAtPosition(position))); + .post(new NavigationUpdatedEvent(navDrawer.drawerNavList.getItemAtPosition(position))); } } private boolean isAlive() { - return mFragmentWeakReference.get() != null - && mFragmentWeakReference.get().isAdded() - && mFragmentWeakReference.get().getActivity() != null - && !mFragmentWeakReference.get().getActivity().isFinishing(); + return fragmentWeakReference.get() != null + && fragmentWeakReference.get().isAdded() + && fragmentWeakReference.get().getActivity() != null + && !fragmentWeakReference.get().getActivity().isFinishing(); } private List buildMainMenu() {