thisgun
commented
1 year ago 안녕하세요. SIR 입니다.
의견 주셔서 감사합니다.
Open kkigomi opened 1 year ago
thisgun
commented
1 year ago 안녕하세요. SIR 입니다.
의견 주셔서 감사합니다.
kkigomi
commented
1 year ago @thisgun -$wr_id로 바뀌었군요.
이게 기준 값이 바뀌어버리는 셈인데 문제가 발생할 수 있습니다.
-$wr_id가 이 이슈의 중복 문제를 해결하는 가장 확실한 방법이지만, 그누보드 커뮤니티에서 흔히 "끌올(끌어 올리기)", "점프" 기능이라고 불리는 wr_num 컬럼의 값을 임의로 변경해 게시물을 가장 앞으로 정렬 시키는 팁이나 스킨, 플러그인 등으로 공유되는 것들이 있습니다. 저도 이런 기능을 만들다가 발견한 이슈입니다.
근데 이런 것들이 마찬가지로 get_next_num() 함수를 이용하는 방법으로 wr_num 컬럼의 값을 변경하는데 이번 변경사항을 아래와 같은 문제가 발생합니다.
| 3개의 글이 순서대로 작성한 상태 (정렬은 wr_num ASC) | wr_id | wr_num |
|---|---|---|
| 3 | -3 | |
| 2 | -2 | |
| 1 | -1 |
| wr_id=1 글을 끌어올리기 (가장 작은 wr_num(-3)에서 1을 뺀 값(-4)으로 지정) | wr_id | wr_num |
|---|---|---|
| 1 | -4 | |
| 3 | -3 | |
| 2 | -2 |
| 새로운 글을 작성하면 wr_id=4로 글이 작성됨 | wr_id | wr_num |
|---|---|---|
| 4 | -4 | |
| 1 | -4 | |
| 3 | -3 | |
| 2 | -2 |
이와 같이 wr_num이 겹치는 문제가 발생합니다.
이런 문제로 일명 "끌올" 기능을 활용하는 사이트에서는 wr_num 중복 문제가 더욱 심각해집니다.
"끌올" 기능을 빈번하게 사용할수록 기존 코드보다 우연이 아닌 확정적으로 문제가 발생합니다.
이 문제는 기존과 같이 get_next_num()을 사용했던 상황을 고려해 호환성을 유지하는 방법으로 서브쿼리로 대체하는 방법이 적절할 것 같습니다.
diff --git forkSrcPrefix/bbs/write_update.php forkDstPrefix/bbs/write_update.php
index d6d47647af35ec6f6a7f830fa5a711bb49b4c70d..6edd6fc69cffdb0bc1aee911448c476d1cf5d7a5 100644
--- forkSrcPrefix/bbs/write_update.php
+++ forkDstPrefix/bbs/write_update.php
@@ -259,7 +259,7 @@ if ($w == '' || $w == 'r') {
}
$sql = " insert into $write_table
- set wr_num = '$wr_num',
+ set wr_num = (select min(wr_num) - 1 from $write_table sq),
wr_reply = '$wr_reply',
wr_comment = 0,
ca_name = '$ca_name',물론 이번에 추가된 코드 $add_wr_update_sql = ($wr_num === 0) ? ", wr_num = '-$wr_id' " : ""; 등은 다시 제거되어야 합니다.
제가 이 이슈에서 wr_parent를 같이 언급한 이유가 기존 방식은 유지하되 wr_num을 서브쿼리로 값을 가져와 사용하여 지연 문제를 줄이도록 개선하고, 이는 완전히 문제를 해결하지 못할 수 있기 때문에 비밀답글의 권한이 잘못 주어지는 문제를 해결하기 위해서 답글의 wr_parent를 답글 자신의 wr_id가 아닌 원본 글의 wr_id를 기록하는 방식... 즉, "댓글(comment)"과 같은 방식의 적용이 가능한가에 대한 것입니다.
현재는 원본글과 마찬가지로 답글 또한 wr_id === wr_parent이므로 원본글을 찾는데 문제가 발생하지만 wr_parent 값을 댓글처럼 원본글의 wr_id를 사용한다면 원본글을 찾을 때 발생하는 문제를 해결할 수 있을 것 같아서 언급한 것입니다. 다만, 아직 관련 코드를 완전히 살피지 못해서 이 부분은 sir 개발자분들이 확인을 해주시길 바랐던 것입니다.
kkigomi
commented
1 year ago wr_parent 관련해서 코드를 다 살피지 못했기 때문에 PR이 아닌 이슈만 남겼던 것인데, 보안취약점은 어쩔 수 없겠지만 버그 등의 문제는 이슈 제출자가 변경된 코드를 확인하는데 시간을 좀 주셨으면 합니다.
이슈 내용을 나름대로 열심히 적었다고는 하지만 윗 댓글처럼 설명하지 못한 부분이나 부족한 부분이 있을 수 있습니다. 저도 업데이트하고 확인하다보니 아차 싶었습니다만, 이러면 제 이슈가 버그를 만들어 내는 원인이 되어버렸다는 생각에 철렁합니다.
내 이슈가 해결이 되었는지 확인하고 이슈 내용이 제대로 전달되었는지 이슈 제출자 스스로도 판단할 시간이 필요합니다. 이슈 제출자에게 3~7일 가량의 시간을 주시면 좋을 것 같습니다.
배포 전에 이슈나 코드에 대해 상호 점검할 시간이 주어졌으면 합니다.
thisgun
commented
1 year ago 안녕하세요. SIR 입니다.
코드를 제공해 주셔서 정말 감사합니다.
알려주신 코드를 참고하여 코드를 수정했습니다.
글, 답변글을 작성할 때
get_next_num()함수로wr_num필드의 값을 가져와 사용하는 과정에서 동시 또는 지연 등의 문제로 이 값이 같은 값이 사용될 수 있음.https://github.com/gnuboard/gnuboard5/blob/641656047d6a9002802383f787a8925888950459/lib/common.lib.php#L792-L800
문제 및 사례
이 원인으로 다음과 같은 문제가 발생할 수 있음
wr_num으로 잘못된 그룹이 지어져 게시물 정렬에 문제가 발생할 수 있음wr_num을 임의로 넣은 값이지만 중복이 발생할 때 같은 현상이 나타날 수 있음wr_num에 의존하여 그룹을 만들어 글, 답글, 댓글에 대한 권한, 컨텐츠 변경 및 이동 등 연관지어 동작하는 코드에서 쉽게 발견되지 않는 알려지지 않는 문제가 발생할 수 있음해결 방안
wr_num을 채워넣기위해get_next_num()함수를 사용해 데이터를 가져오는 과정에서 예상보다 긴 지연이 발생할 경우 많은 문제를 일으킬 수 있으므로 이 함수를 사용하는 대신 서브쿼리 등으로 글 데이터를 insert 할 때 바로 참조하는 방법으로 조금이나마 개선이 가능할 것으로 보입니다.이 문제의 완전한 해결책은 아니지만 현재의 호환성을 유지하면서 개선이 가능한 방법이 될 것 같습니다.
대부분 '원글'을 잘못 찾는 문제이므로 댓글(comment)처럼 답글의
wr_parent에 원글의wr_id를 넣고 원글을 찾는데 활용하는 것으로 해결이 가능할 것으로 보임.다만,
wr_parent가 댓글과의 연관 관계를 가지는데 사용된다면 답글의 댓글에 영향을 줄 수 있음. 정돈되지 않은 코드, 존재하지 않는 개발자 매뉴얼, 커밋 메시지와 코드 변경이력이 추적을 어렵게 하기 때문에 사실상 로직 분석이 불가하여 개발팀에서 해결 방안을 찾아야 할 것 같습니다.