프로필 파일 업로드 취약점 패치 #342
프로필 파일 업로드 취약점 패치 #342
Conversation
Jueuunn7
requested review from
dolong2,
KimTaeO,
baekteun and
Umjiseung
as code owners
| @@ -21,11 +21,8 @@ class S3Util( | |||
|
|
|||
| val splitFile = image.originalFilename.toString().split(".") | |||
|
|
|||
| if(splitFile.size < 2) | |||
There was a problem hiding this comment.
위 검증 로직이 잘못되어있는 것 같네요
splitFile의 인자가 2개 미만이면 if문을 실행시키는 것으로 보이는데
만약에 script.svg.php라면 splitFile 배열에 담기는 것은 [script, svg, php]가 담기게 되는데 이것이 if문의 결과를 false로 만들게 되네요 그래서 통과가 되던 것 같구요
그래서 last()를 사용하는 법도 있겠지만 splitFile.size >= 2로 바꾸는 것도 방법이 될 수 있을 것 같습니다
There was a problem hiding this comment.
리뷰 해주셔서 감사합니다.🙇🙇
splitFile.size >= 2 이렇게 if문을 바꾸는 방법도 좋은 방법인것 같습니다.
해당 취약점은 .의 갯수가 여러개라서 발생하는것보단 확장자를 한번 더 덮어씌우는데 중점이 있습니다.
만약에 사용자가 hello.world.png파일을 업로드하게 되면 정상적인 파일도 .이 2개 이상이 되기 때문에 정상적인 파일도 필터링 해서 가용성에 문제가 생길수도 있을 것 같습니다.
이 두가지의 이유와 가독성이 향상되는 이유로 last()를 사용하는 것은 어떨까요?
There was a problem hiding this comment.
잘못 전달한 부분이 있네요 일단 그 부분에 대해서는 사과 드립니다
저도 배열에 직접 접근하는 것 보다는 last() 사용하는 것이 더 좋아보이네요
하지만 한번 더 정확한 검증을 위해서 If문을 삭제하지 않고 처음부터 xxx.xxx의 형식인 파일만 받을 수 있도록 하자는 취지로 삭제한 If문을 되돌리며 동시에 처음에 제시한 조건이 아닌 if(splitFile.size != 2)로 변경하자는 의견이었습니다
빠르게 답변 드리려다 보니 깊게 생각하지 못하고 잘못된 조건을 전달드렸네요
There was a problem hiding this comment.
좋은 의견 감사합니다.🙇🙇
if문을 사용하면 검증을 더욱 정확히 할 수 있지만 last()보단 가용성이 떨어질 수 있습니다.
또한 last()를 사용하면 가용성과 가독성을 향상시킬 수 있지만 if문 보단 무결성이 떨어질 수 있습니다.
무결성, 가용성과 가독성의 균형을 맞춰야하는 문제 같은데 저는 last()를 사용하는 방법이 더 이점이 많다고 생각합니다.
어떻게 생각하시나요?
There was a problem hiding this comment.
cb4740e
전부 반영하는 방법도 좋은 방법 같습니다! 수정했습니다.
💡 배경 및 개요
프로필사진 파일 업로드 취약점을 보완하였습니다.
Resolves: #341
📃 작업내용
해당 취약점을 패치 후 필요없는 로직을 삭제하였습니다.
✅ PR 체크리스트
.env,노션,README)"API 개발 완료됐어요","환경값 추가되었어요")