Skip to content
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.

Sign up for GitHub

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

프로필 파일 업로드 취약점 패치 #342

Merged
merged 3 commits into from
Jul 3, 2024
+5 −5
Merged

프로필 파일 업로드 취약점 패치 #342

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b00fab8
Update :: S3Util.kt 수정
Jueuunn7 Jul 3, 2024
c0ffda2
Update S3Util.kt
Jueuunn7 Jul 3, 2024
cb4740e
Update S3Util.kt
Jueuunn7 Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions src/main/kotlin/com/msg/gauth/global/thirdparty/aws/s3/S3Util.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,8 @@ class S3Util(

val splitFile = image.originalFilename.toString().split(".")

if(splitFile.size < 2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위 검증 로직이 잘못되어있는 것 같네요

splitFile의 인자가 2개 미만이면 if문을 실행시키는 것으로 보이는데

만약에 script.svg.php라면 splitFile 배열에 담기는 것은 [script, svg, php]가 담기게 되는데 이것이 if문의 결과를 false로 만들게 되네요 그래서 통과가 되던 것 같구요

그래서 last()를 사용하는 법도 있겠지만 splitFile.size >= 2로 바꾸는 것도 방법이 될 수 있을 것 같습니다

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 해주셔서 감사합니다.🙇🙇

splitFile.size >= 2 이렇게 if문을 바꾸는 방법도 좋은 방법인것 같습니다.

해당 취약점은 .의 갯수가 여러개라서 발생하는것보단 확장자를 한번 더 덮어씌우는데 중점이 있습니다.

만약에 사용자가 hello.world.png파일을 업로드하게 되면 정상적인 파일도 .이 2개 이상이 되기 때문에 정상적인 파일도 필터링 해서 가용성에 문제가 생길수도 있을 것 같습니다.

이 두가지의 이유와 가독성이 향상되는 이유로 last()를 사용하는 것은 어떨까요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

잘못 전달한 부분이 있네요 일단 그 부분에 대해서는 사과 드립니다

저도 배열에 직접 접근하는 것 보다는 last() 사용하는 것이 더 좋아보이네요
하지만 한번 더 정확한 검증을 위해서 If문을 삭제하지 않고 처음부터 xxx.xxx의 형식인 파일만 받을 수 있도록 하자는 취지로 삭제한 If문을 되돌리며 동시에 처음에 제시한 조건이 아닌 if(splitFile.size != 2)로 변경하자는 의견이었습니다

빠르게 답변 드리려다 보니 깊게 생각하지 못하고 잘못된 조건을 전달드렸네요

Copy link
Contributor Author

@Jueuunn7 Jueuunn7 Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 의견 감사합니다.🙇🙇

if문을 사용하면 검증을 더욱 정확히 할 수 있지만 last()보단 가용성이 떨어질 수 있습니다.
또한 last()를 사용하면 가용성과 가독성을 향상시킬 수 있지만 if문 보단 무결성이 떨어질 수 있습니다.

무결성, 가용성과 가독성의 균형을 맞춰야하는 문제 같은데 저는 last()를 사용하는 방법이 더 이점이 많다고 생각합니다.

어떻게 생각하시나요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

두개 전부 반영하자는 의견이었습니다!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cb4740e
전부 반영하는 방법도 좋은 방법 같습니다! 수정했습니다.

throw FileExtensionInvalidException()

val extension = splitFile[1].lowercase()

val extension = splitFile.last().lowercase()

if(list.none { it == extension })
throw FileExtensionInvalidException()

Expand All @@ -49,4 +46,4 @@ class S3Util(

amazonS3.deleteObject(bucket, key)
}
}
}