[FEAT/#285] 보안 취약점 대응 및 강화를 합니다. #286
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| package com.sopt.clody.core.security.login | ||
|
|
||
| import android.content.Context | ||
| import android.content.pm.PackageManager | ||
| import android.os.Build | ||
| import java.io.File | ||
| import javax.inject.Inject | ||
|
|
||
| /** | ||
| * 기본 보안 점검 구현체. | ||
| * | ||
| * 디바이스 루팅 여부 및 Chrome 브라우저 설치 여부를 확인하는 기능 구현. | ||
| * | ||
| */ | ||
|
|
||
| class DefaultLoginSecurityChecker @Inject constructor() : LoginSecurityChecker { | ||
|
|
||
| /** | ||
| * 디바이스가 루팅되었는지 여부를 검사. | ||
| * Step | ||
| * - `Build.TAGS`에 `test-keys`가 포함되어 있는지 확인 | ||
| * - 루팅에 사용되는 바이너리 또는 앱의 존재 여부 검사 | ||
| * - `which su` 명령어 실행 결과를 통해 `su` 명령어의 존재 여부 확인 | ||
| * | ||
| * @return 디바이스가 루팅되었다면 `true`, 그렇지 않다면 `false` | ||
| */ | ||
| override fun isDeviceRooted(): Boolean { | ||
| val buildTags = Build.TAGS | ||
| if (buildTags != null && buildTags.contains("test-keys")) return true | ||
|
|
||
| val paths = arrayOf( | ||
| "/system/app/Superuser.apk", | ||
| "/sbin/su", "/system/bin/su", "/system/xbin/su", | ||
| "/data/local/xbin/su", "/data/local/bin/su", | ||
| "/system/sd/xbin/su", "/system/bin/failsafe/su", | ||
| "/data/local/su", | ||
| ) | ||
| if (paths.any { File(it).exists() }) return true | ||
|
|
||
| return try { | ||
| Runtime.getRuntime().exec(arrayOf("/system/xbin/which", "su")) | ||
| .inputStream.bufferedReader().readLine() != null | ||
| } catch (e: Exception) { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 디바이스에 Chrome 브라우저가 설치되어 있는지 여부 | ||
| * | ||
| * `com.android.chrome` 패키지의 존재 여부로 Chrome 설치 여부를 판단. | ||
| * | ||
| * @return Chrome이 설치되어 있다면 `true`, 아니라면 `false` | ||
| */ | ||
| override fun isChromeInstalled(context: Context): Boolean = try { | ||
| context.packageManager.getPackageInfo("com.android.chrome", 0) | ||
| true | ||
| } catch (e: PackageManager.NameNotFoundException) { | ||
| false | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package com.sopt.clody.core.security.login | ||
|
|
||
| import android.content.Context | ||
|
|
||
| interface LoginSecurityChecker { | ||
| fun isDeviceRooted(): Boolean | ||
| fun isChromeInstalled(context: Context): Boolean | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package com.sopt.clody.core.security.login | ||
|
|
||
| import dagger.Binds | ||
| import dagger.Module | ||
| import dagger.hilt.InstallIn | ||
| import dagger.hilt.components.SingletonComponent | ||
| import javax.inject.Singleton | ||
|
|
||
| @Module | ||
| @InstallIn(SingletonComponent::class) | ||
| abstract class SecurityModule { | ||
|
|
||
| @Binds | ||
| @Singleton | ||
| abstract fun bindLoginSecurityChecker( | ||
| impl: DefaultLoginSecurityChecker, | ||
| ): LoginSecurityChecker | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,33 @@ | ||||||||||||||||||||
| package com.sopt.clody.core.security.weview | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import android.content.Context | ||||||||||||||||||||
| import android.content.Intent | ||||||||||||||||||||
| import android.net.http.SslError | ||||||||||||||||||||
| import android.webkit.SslErrorHandler | ||||||||||||||||||||
| import android.webkit.WebResourceRequest | ||||||||||||||||||||
| import android.webkit.WebView | ||||||||||||||||||||
| import android.webkit.WebViewClient | ||||||||||||||||||||
|
|
||||||||||||||||||||
| class SecureWebViewClient( | ||||||||||||||||||||
| private val context: Context, | ||||||||||||||||||||
| private val allowedDomains: List<String> = listOf("notion.so", "forms.gle"), | ||||||||||||||||||||
| ) : WebViewClient() { | ||||||||||||||||||||
|
|
||||||||||||||||||||
| override fun shouldOverrideUrlLoading(view: WebView, request: WebResourceRequest): Boolean { | ||||||||||||||||||||
| val host = request.url.host ?: return true | ||||||||||||||||||||
| val isSafeDomain = allowedDomains.any { host.contains(it) } | ||||||||||||||||||||
|
Comment on lines
+17
to
+18
There was a problem hiding this comment. Potential security bypass with substring matching. The current implementation uses Consider using exact domain matching or suffix checking: - val isSafeDomain = allowedDomains.any { host.contains(it) }
+ val isSafeDomain = allowedDomains.any { allowedDomain ->
+ host == allowedDomain || host.endsWith(".$allowedDomain")
+ }🤖 Prompt for AI Agents |
||||||||||||||||||||
|
|
||||||||||||||||||||
| return if (isSafeDomain) { | ||||||||||||||||||||
| false | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| Intent(Intent.ACTION_VIEW, request.url).let { | ||||||||||||||||||||
| context.startActivity(it) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+23
to
+25
There was a problem hiding this comment. 🛠️ Refactor suggestion Potential security risk with unverified intent launching. Launching intents without verification could be exploited by malicious URLs (e.g., Add intent verification and safe launching: - Intent(Intent.ACTION_VIEW, request.url).let {
- context.startActivity(it)
- }
+ Intent(Intent.ACTION_VIEW, request.url).let { intent ->
+ if (intent.resolveActivity(context.packageManager) != null &&
+ (request.url.scheme == "http" || request.url.scheme == "https")) {
+ context.startActivity(intent)
+ }
+ }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
| true | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| override fun onReceivedSslError(view: WebView, handler: SslErrorHandler, error: SslError) { | ||||||||||||||||||||
| handler.cancel() | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🛠️ Refactor suggestion
Potential command injection vulnerability and incomplete detection.
The
which sucommand execution has security and reliability issues:/system/xbin/whichmay not exist on all devicessuactually existsConsider a safer approach:
📝 Committable suggestion
🧰 Tools
🪛 detekt (1.23.8)
[warning] 43-43: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents