Skip to content

Commit d391cba

Browse files
hawkffhawkff
authored
Security & stability fixes (local proxy, crashes, import) (#44)
* fix(import): raise SnakeYAML alias cap to 1000 for large Clash subs (MatsuriDayo#1042) * fix(ui): guard About screen against detached-fragment crash (MatsuriDayo#1192) * fix(ui): replace Filter with coroutine filtering to avoid per-apps OOM (MatsuriDayo#1086) * fix(ui): survive rotation during URL test; guard dialog dismiss (MatsuriDayo#1141) * fix(import): don't abort profile import on stray http link (MatsuriDayo#1128) * feat(security): close/auth local proxy inbound options (MatsuriDayo#1154, MatsuriDayo#1197) * chore(security): untrack release.keystore; ignore signing keys * review: address CR/Greptile feedback (alias cap 200, ISE catch, @volatile, typo)
1 parent 8082f8d commit d391cba

13 files changed

Lines changed: 132 additions & 42 deletions

File tree

‎.gitignore‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,8 @@ local.properties
3232

3333
# Submodules / external sources
3434
/external/
35+
36+
# Signing keys — never commit; provided via CI secrets or local.properties
37+
release.keystore
38+
*.keystore
39+
*.jks

‎app/src/main/AndroidManifest.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@
7474

7575
<activity
7676
android:name="io.nekohasekai.sagernet.ui.MainActivity"
77-
android:configChanges="uiMode"
77+
android:configChanges="uiMode|orientation|screenSize|keyboardHidden"
7878
android:exported="true"
7979
android:launchMode="singleTask">
8080
<intent-filter>

‎app/src/main/java/io/nekohasekai/sagernet/Constants.kt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ object Key {
4747
const val MIXED_SECRET = "mixedSecret" // storage key for the generated inbound secret
4848
const val MIXED_USERNAME = "neko" // username presented to the authed mixed inbound
4949
const val ALLOW_ACCESS = "allowAccess"
50+
const val REQUIRE_PROXY_IN_VPN = "requireProxyInVPN" // keep local mixed inbound open in VPN mode
51+
const val PROXY_MODE_INBOUND_AUTH = "proxyModeInboundAuth" // authenticate loopback inbound in Proxy mode
5052
const val SPEED_INTERVAL = "speedInterval"
5153
const val SHOW_DIRECT_SPEED = "showDirectSpeed"
5254

‎app/src/main/java/io/nekohasekai/sagernet/database/DataStore.kt‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,19 @@ object DataStore : OnPreferenceDataStoreChangeListener {
169169
// inbound is authenticated, except when appendHttpProxy is active on
170170
// Android Q+ (the system HTTP proxy cannot supply credentials).
171171
(serviceMode == Key.MODE_VPN &&
172-
!(appendHttpProxy && Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q))
172+
!(appendHttpProxy && Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q)) ||
173+
// Proxy service mode loopback inbound: optionally authenticate too
174+
// (issue #1197 residual gap). Off by default to preserve the
175+
// "open localhost proxy" use case; users opt in for hardening.
176+
(serviceMode == Key.MODE_PROXY && proxyModeInboundAuth)
177+
178+
// Keep the local mixed (SOCKS/HTTP) inbound open in VPN mode. Default false: in
179+
// TUN mode the local proxy port is usually unnecessary, and closing it removes the
180+
// local port-scan attack surface entirely (PR #1154 / issue #1197).
181+
var requireProxyInVPN by configurationStore.boolean(Key.REQUIRE_PROXY_IN_VPN)
182+
183+
// Authenticate the loopback mixed inbound in Proxy service mode (issue #1197).
184+
var proxyModeInboundAuth by configurationStore.boolean(Key.PROXY_MODE_INBOUND_AUTH)
173185

174186
val mixedInboundUser: String get() = if (mixedInboundAuthed) Key.MIXED_USERNAME else ""
175187
val mixedInboundPass: String get() = if (mixedInboundAuthed) mixedSecret else ""

‎app/src/main/java/io/nekohasekai/sagernet/fmt/ConfigBuilder.kt‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package io.nekohasekai.sagernet.fmt
22

3+
import android.os.Build
34
import android.widget.Toast
45
import io.nekohasekai.sagernet.*
56
import io.nekohasekai.sagernet.bg.VpnService
@@ -147,6 +148,14 @@ fun buildConfig(
147148
val bypassDNSBeans = hashSetOf<AbstractBean>()
148149
val isVPN = DataStore.serviceMode == Key.MODE_VPN
149150
val bind = if (!forTest && DataStore.allowAccess) "0.0.0.0" else LOCALHOST
151+
// Whether the local mixed (SOCKS/HTTP) inbound is present in the final config.
152+
// In VPN/TUN mode it is omitted unless the user opts in (requireProxyInVPN) or the
153+
// system HTTP proxy needs it (appendHttpProxy). See issue #1197 / PR #1154.
154+
val keepMixedInbound = !forTest && (
155+
!isVPN ||
156+
DataStore.requireProxyInVPN ||
157+
(DataStore.appendHttpProxy && Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q)
158+
)
150159
val remoteDns = DataStore.remoteDns.split("\n")
151160
.mapNotNull { dns -> sanitizeDnsEntry(dns).takeIf { it.isNotBlank() && !it.startsWith("#") } }
152161
val directDNS = DataStore.directDns.split("\n")
@@ -252,7 +261,11 @@ fun buildConfig(
252261
}
253262
}
254263
})
255-
inbounds.add(Inbound_MixedOptions().apply {
264+
// Local mixed (SOCKS/HTTP) inbound. In VPN/TUN mode this port is usually
265+
// unnecessary and only widens the local attack surface (issue #1197), so it
266+
// is omitted unless the user opts in (requireProxyInVPN) or the system HTTP
267+
// proxy needs it (appendHttpProxy routes the device proxy through this port).
268+
if (keepMixedInbound) inbounds.add(Inbound_MixedOptions().apply {
256269
type = "mixed"
257270
tag = TAG_MIXED
258271
listen = bind
@@ -570,7 +583,7 @@ fun buildConfig(
570583
outbound = mainProxyTag
571584
})
572585

573-
route.rules.add(Rule_DefaultOptions().apply {
586+
if (keepMixedInbound) route.rules.add(Rule_DefaultOptions().apply {
574587
inbound = listOf(TAG_MIXED)
575588
outbound = mainProxyTag
576589
})

‎app/src/main/java/io/nekohasekai/sagernet/group/RawUpdater.kt‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,13 @@ object RawUpdater : GroupUpdater() {
267267
// tag; a YAML mapping is returned as a LinkedHashMap natively.
268268
val loaderOptions = LoaderOptions().apply {
269269
codePointLimit = 10 * 1024 * 1024 // 10 MiB
270+
// SnakeYAML 2.x defaults maxAliasesForCollections to 50 as a
271+
// billion-laughs guard. Legitimate large Clash/Mihomo configs
272+
// reuse anchors heavily and exceed 50 (issue #1042). Raise to a
273+
// finite cap (not Int.MAX_VALUE). 200 covers known real-world
274+
// configs while keeping alias-expansion amplification bounded
275+
// (codePointLimit bounds input size, not the expanded object graph).
276+
maxAliasesForCollections = 200
270277
}
271278
// In SnakeYAML 2.x, Yaml(BaseConstructor) adopts the constructor's
272279
// LoaderOptions (getLoadingConfig()), so codePointLimit set above is

‎app/src/main/java/io/nekohasekai/sagernet/ktx/Formats.kt‎

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,11 @@ suspend fun parseProxies(text: String): List<AbstractBean> {
113113

114114
val entities = ArrayList<AbstractBean>()
115115
val entitiesByLine = ArrayList<AbstractBean>()
116+
// An http(s) link that fails to parse as an HTTP proxy is a subscription candidate.
117+
// Don't abort import immediately (issue #1128): a file may contain valid profile
118+
// links alongside a plain promo/Telegram URL. Remember the first candidate and only
119+
// treat the input as a subscription if NO profiles parsed at all.
120+
var subscriptionCandidate: String? = null
116121

117122
fun String.parseLink(entities: ArrayList<AbstractBean>) {
118123
if (startsWith("clash://install-config?") || startsWith("sn://subscription?")) {
@@ -142,14 +147,17 @@ suspend fun parseProxies(text: String): List<AbstractBean> {
142147
entities.add(parseHttp(this))
143148
}.onFailure {
144149
Logs.w(it)
145-
val clashUrl = HttpUrl.Builder()
146-
.scheme("https")
147-
.host("install-config")
148-
.addQueryParameter("url", this)
149-
.build()
150-
.toString()
151-
.replaceFirst("https://", "clash://")
152-
throw (SubscriptionFoundException(clashUrl))
150+
if (subscriptionCandidate == null) {
151+
val clashUrl = HttpUrl.Builder()
152+
.scheme("https")
153+
.host("install-config")
154+
.addQueryParameter("url", this)
155+
.build()
156+
.toString()
157+
.replaceFirst("https://", "clash://")
158+
// Defer: only thrown later if no profile links were parsed.
159+
subscriptionCandidate = clashUrl
160+
}
153161
}
154162
} else if (startsWith("vmess://")) {
155163
Logs.d("Try parse v2ray link: $this")
@@ -258,6 +266,12 @@ suspend fun parseProxies(text: String): List<AbstractBean> {
258266
for (link in linksByLine) {
259267
link.parseLink(entitiesByLine)
260268
}
269+
// No profile links parsed but we saw an unparsable http(s) URL: treat the whole
270+
// input as a subscription link (single-URL paste / file). When profiles WERE found,
271+
// the stray URL is ignored so the profiles still import (issue #1128).
272+
if (entities.isEmpty() && entitiesByLine.isEmpty()) {
273+
subscriptionCandidate?.let { throw SubscriptionFoundException(it) }
274+
}
261275
// var isBadLink = false
262276
if (entities.onEach { it.initializeDefaultValues() }.size == entitiesByLine.onEach { it.initializeDefaultValues() }.size) run test@{
263277
entities.forEachIndexed { index, bean ->

‎app/src/main/java/io/nekohasekai/sagernet/ui/AboutFragment.kt‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ class AboutFragment : ToolbarFragment(R.layout.layout_about) {
104104
.addItem(
105105
MaterialAboutActionItem.Builder()
106106
.icon(R.drawable.ic_baseline_layers_24)
107-
.text(getString(R.string.version_x, "sing-box"))
107+
.text(activityContext.getString(R.string.version_x, "sing-box"))
108108
.subText(Libcore.versionBox())
109109
.setOnClickAction { }
110110
.build())
@@ -130,7 +130,7 @@ class AboutFragment : ToolbarFragment(R.layout.layout_about) {
130130
MaterialAboutActionItem.Builder()
131131
.icon(R.drawable.ic_baseline_nfc_24)
132132
.text(
133-
getString(
133+
activityContext.getString(
134134
R.string.version_x,
135135
pluginId
136136
) + " (${Plugins.displayExeProvider(pkg.packageName)})"
@@ -248,6 +248,11 @@ class AboutFragment : ToolbarFragment(R.layout.layout_about) {
248248
haveUpdate && !releaseName.contains(BuildConfig.VERSION_NAME)
249249
}
250250
runOnMainDispatcher {
251+
// The async work above may outlive the fragment's attachment
252+
// (e.g. user navigates away). Touching requireContext()/app
253+
// resources while detached throws IllegalStateException
254+
// (issue #1192). Bail out if no longer attached.
255+
if (!isAdded) return@runOnMainDispatcher
251256
if (haveUpdate) {
252257
val context = requireContext()
253258
MaterialAlertDialogBuilder(context)
@@ -272,6 +277,7 @@ class AboutFragment : ToolbarFragment(R.layout.layout_about) {
272277
} catch (e: Exception) {
273278
Logs.w(e)
274279
runOnMainDispatcher {
280+
if (!isAdded) return@runOnMainDispatcher
275281
Toast.makeText(app, e.readableMessage, Toast.LENGTH_SHORT).show()
276282
}
277283
}

‎app/src/main/java/io/nekohasekai/sagernet/ui/AppManagerActivity.kt‎

Lines changed: 43 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,6 @@ import android.view.Menu
1313
import android.view.MenuItem
1414
import android.view.View
1515
import android.view.ViewGroup
16-
import android.widget.Filter
17-
import android.widget.Filterable
1816
import androidx.annotation.UiThread
1917
import androidx.core.util.contains
2018
import androidx.core.util.set
@@ -42,7 +40,9 @@ import io.nekohasekai.sagernet.utils.PackageCache
4240
import io.nekohasekai.sagernet.widget.ListListener
4341
import kotlinx.coroutines.Dispatchers
4442
import kotlinx.coroutines.Job
43+
import kotlinx.coroutines.delay
4544
import kotlinx.coroutines.ensureActive
45+
import kotlinx.coroutines.launch
4646
import kotlinx.coroutines.withContext
4747
import moe.matsuri.nb4a.utils.NGUtil
4848
import kotlin.coroutines.coroutineContext
@@ -101,7 +101,6 @@ class AppManagerActivity : ThemedActivity() {
101101
}
102102

103103
private inner class AppsAdapter : RecyclerView.Adapter<AppViewHolder>(),
104-
Filterable,
105104
FastScrollRecyclerView.SectionedAdapter {
106105
var filteredApps = apps
107106

@@ -130,26 +129,25 @@ class AppManagerActivity : ThemedActivity() {
130129

131130
override fun getItemCount(): Int = filteredApps.size
132131

133-
private val filterImpl = object : Filter() {
134-
override fun performFiltering(constraint: CharSequence) = FilterResults().apply {
135-
var filteredApps = if (constraint.isEmpty()) apps else apps.filter {
136-
it.name.contains(constraint, true) || it.packageName.contains(
137-
constraint, true
138-
) || it.uid.toString().contains(constraint)
139-
}
140-
if (!sysApps) filteredApps = filteredApps.filter { !it.sys }
141-
count = filteredApps.size
142-
values = filteredApps
143-
}
144-
145-
override fun publishResults(constraint: CharSequence, results: FilterResults) {
146-
@Suppress("UNCHECKED_CAST")
147-
filteredApps = results.values as List<ProxiedApp>
148-
notifyDataSetChanged()
132+
// Replaces android.widget.Filter, which spawned a new worker thread per
133+
// filter() call (OutOfMemoryError: pthread_create failed, issue #1086).
134+
// Computes the filtered list off the main thread and publishes on main;
135+
// callers cancel the previous job via applyFilter() below.
136+
fun computeFiltered(constraint: CharSequence): List<ProxiedApp> {
137+
var result = if (constraint.isEmpty()) apps else apps.filter {
138+
it.name.contains(constraint, true) || it.packageName.contains(
139+
constraint, true
140+
) || it.uid.toString().contains(constraint)
149141
}
142+
if (!sysApps) result = result.filter { !it.sys }
143+
return result
150144
}
151145

152-
override fun getFilter(): Filter = filterImpl
146+
fun publishFiltered(result: List<ProxiedApp>) {
147+
filteredApps = result
148+
@Suppress("NotifyDataSetChanged")
149+
notifyDataSetChanged()
150+
}
153151

154152
override fun getSectionName(position: Int): String {
155153
return filteredApps[position].name.firstOrNull()?.toString() ?: ""
@@ -162,9 +160,27 @@ class AppManagerActivity : ThemedActivity() {
162160
private lateinit var binding: LayoutAppsBinding
163161
private val proxiedUids = SparseBooleanArray()
164162
private var loader: Job? = null
163+
private var filterJob: Job? = null
164+
// Read on Dispatchers.Default in computeFiltered(); written from Default/IO/Main.
165+
// @Volatile guarantees cross-thread visibility so filtering never sees a stale list.
166+
@Volatile
165167
private var apps = emptyList<ProxiedApp>()
166168
private val appsAdapter = AppsAdapter()
167169

170+
// Coroutine-based replacement for the old Filter.filter(...) calls. Cancels the
171+
// in-flight filter job (so rapid keystrokes don't pile up work) and computes the
172+
// filtered list on a background dispatcher before publishing on main.
173+
// debounceMs > 0 is used for the search box to avoid filtering on every keystroke.
174+
@UiThread
175+
private fun applyFilter(constraint: String = binding.search.text?.toString() ?: "", debounceMs: Long = 0) {
176+
filterJob?.cancel()
177+
filterJob = lifecycleScope.launch {
178+
if (debounceMs > 0) delay(debounceMs)
179+
val result = withContext(Dispatchers.Default) { appsAdapter.computeFiltered(constraint) }
180+
appsAdapter.publishFiltered(result)
181+
}
182+
}
183+
168184
private fun initProxiedUids(str: String = DataStore.individual) {
169185
proxiedUids.clear()
170186
val apps = cachedApps
@@ -184,7 +200,7 @@ class AppManagerActivity : ThemedActivity() {
184200
loading.crossFadeFrom(binding.list)
185201
val adapter = binding.list.adapter as AppsAdapter
186202
withContext(Dispatchers.IO) { adapter.reload() }
187-
adapter.filter.filter(binding.search.text?.toString() ?: "")
203+
applyFilter()
188204
if (apps.isEmpty()) {
189205
binding.list.visibility = View.GONE
190206
binding.appPlaceholder.root.crossFadeFrom(loading)
@@ -241,19 +257,20 @@ class AppManagerActivity : ThemedActivity() {
241257
ViewCompat.setOnApplyWindowInsetsListener(binding.root, ListListener)
242258

243259
binding.search.addTextChangedListener {
244-
appsAdapter.filter.filter(it?.toString() ?: "")
260+
applyFilter(it?.toString() ?: "", debounceMs = 250)
245261
}
246262

247263
binding.showSystemApps.isChecked = sysApps
248264
binding.showSystemApps.setOnCheckedChangeListener { _, isChecked ->
249265
sysApps = isChecked
250-
appsAdapter.filter.filter(binding.search.text?.toString() ?: "")
266+
applyFilter()
251267
}
252268

253269
instance = this
254270
loadApps()
255271
}
256272

273+
@Volatile
257274
private var sysApps = true
258275

259276
override fun onCreateOptionsMenu(menu: Menu): Boolean {
@@ -277,7 +294,7 @@ class AppManagerActivity : ThemedActivity() {
277294
.joinToString("\n") { it.packageName }
278295
apps = apps.sortedWith(compareBy({ !isProxiedApp(it) }, { it.name.toString() }))
279296
onMainDispatcher {
280-
appsAdapter.filter.filter(binding.search.text?.toString() ?: "")
297+
applyFilter()
281298
}
282299
}
283300

@@ -290,7 +307,7 @@ class AppManagerActivity : ThemedActivity() {
290307
DataStore.individual = ""
291308
apps = apps.sortedWith(compareBy({ !isProxiedApp(it) }, { it.name.toString() }))
292309
onMainDispatcher {
293-
appsAdapter.filter.filter(binding.search.text?.toString() ?: "")
310+
applyFilter()
294311
}
295312
}
296313
}
@@ -363,7 +380,7 @@ class AppManagerActivity : ThemedActivity() {
363380
DataStore.individual =
364381
apps.filter { isProxiedApp(it) }.joinToString("\n") { it.packageName }
365382
apps = apps.sortedWith(compareBy({ !isProxiedApp(it) }, { it.name.toString() }))
366-
appsAdapter.filter.filter(binding.search.text?.toString() ?: "")
383+
applyFilter()
367384
} catch (e: Exception) {
368385
Logs.e(e)
369386
}

‎app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -914,7 +914,7 @@ class ConfigurationFragment @JvmOverloads constructor(
914914
}
915915
test.cancel = {
916916
test.dialogStatus.set(2)
917-
dialog.dismiss()
917+
try { dialog.dismiss() } catch (e: IllegalStateException) { Logs.w(e) } // dialog window may be gone after rotation (#1141)
918918
runOnDefaultDispatcher {
919919
mainJob.cancel()
920920
testJobs.forEach { it.cancel() }
@@ -983,7 +983,7 @@ class ConfigurationFragment @JvmOverloads constructor(
983983
}
984984
test.cancel = {
985985
test.dialogStatus.set(2)
986-
dialog.dismiss()
986+
try { dialog.dismiss() } catch (e: IllegalStateException) { Logs.w(e) } // dialog window may be gone after rotation (#1141)
987987
runOnDefaultDispatcher {
988988
mainJob.cancel()
989989
testJobs.forEach { it.cancel() }

0 commit comments

Comments
 (0)