-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[shared_preferences] Limit Android decoding #8187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
## 2.3.4 | ||
|
||
* Restrict types when decoding preferences. | ||
|
||
## 2.3.3 | ||
|
||
* Updates Java compatibility version to 11. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// Copyright 2013 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
package io.flutter.plugins.sharedpreferences | ||
|
||
import java.io.IOException | ||
import java.io.InputStream | ||
import java.io.ObjectInputStream | ||
import java.io.ObjectStreamClass | ||
|
||
/** | ||
* An ObjectInputStream that only allows string lists, to prevent injected prefs from instantiating | ||
* arbitrary objects. | ||
*/ | ||
class StringListObjectInputStream(input: InputStream) : ObjectInputStream(input) { | ||
@Throws(ClassNotFoundException::class, IOException::class) | ||
override fun resolveClass(desc: ObjectStreamClass?): Class<*>? { | ||
val allowList = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We talked out of band and I am comfortable with this approach but I think this list is probably the most risky to non malicious apps. I wonder if we should apply this patch to G3 and see if any unit tests break as additional testing. |
||
setOf( | ||
"java.util.Arrays\$ArrayList", | ||
"java.util.ArrayList", | ||
"java.lang.String", | ||
"[Ljava.lang.String;") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK after sleeping on it I think this approach makes sense. I am worried about the fragility but I think your error with desc.name contains the information we would need to parse if it came up. The better solutions from a dev perspective are still vulnerable so I think the allow list is what we have to go with. |
||
val name = desc?.name | ||
if (name != null && !allowList.contains(name)) { | ||
throw ClassNotFoundException(desc.name) | ||
} | ||
return super.resolveClass(desc) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,13 +5,17 @@ | |
package io.flutter.plugins.sharedpreferences | ||
|
||
import android.content.Context | ||
import android.util.Base64 | ||
import androidx.test.core.app.ApplicationProvider | ||
import androidx.test.ext.junit.runners.AndroidJUnit4 | ||
import io.flutter.embedding.engine.plugins.FlutterPlugin | ||
import io.flutter.plugin.common.BinaryMessenger | ||
import io.mockk.every | ||
import io.mockk.mockk | ||
import java.io.ByteArrayOutputStream | ||
import java.io.ObjectOutputStream | ||
import org.junit.Assert | ||
import org.junit.Assert.assertThrows | ||
import org.junit.Test | ||
import org.junit.runner.RunWith | ||
|
||
|
@@ -48,6 +52,7 @@ internal class SharedPreferencesTest { | |
every { flutterPluginBinding.binaryMessenger } returns binaryMessenger | ||
every { flutterPluginBinding.applicationContext } returns testContext | ||
plugin.onAttachedToEngine(flutterPluginBinding) | ||
plugin.clear(null, emptyOptions) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests were potentially cross-contaminating, so I added this to give each test a clean slate. |
||
return plugin | ||
} | ||
|
||
|
@@ -171,4 +176,24 @@ internal class SharedPreferencesTest { | |
Assert.assertNull(all[doubleKey]) | ||
Assert.assertNull(all[listKey]) | ||
} | ||
|
||
@Test | ||
fun testUnexpectedClassDecodeThrows() { | ||
// Only String should be allowed in an encoded list. | ||
val badList = listOf(1, 2, 3) | ||
// Replicate the behavior of ListEncoder.encode, but with a non-List<String> list. | ||
val byteStream = ByteArrayOutputStream() | ||
val stream = ObjectOutputStream(byteStream) | ||
stream.writeObject(badList) | ||
stream.flush() | ||
val badPref = LIST_PREFIX + Base64.encodeToString(byteStream.toByteArray(), 0) | ||
|
||
val plugin = pluginSetup() | ||
val badListKey = "badList" | ||
// Inject the bad pref as a string, as that is how string lists are stored internally. | ||
plugin.setString(badListKey, badPref, emptyOptions) | ||
assertThrows(ClassNotFoundException::class.java) { | ||
plugin.getStringList(badListKey, emptyOptions) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These try/catch blocks weren't doing anything since they were only catching and throwing the same types, so I removed them as an opportunistic cleanup.