주문 상세 화면 주소에 order_no 가 그대로 들어 있었다. 숫자를 하나 줄여 봤더니 남의 주문이 나왔다.
Table of contents
Open Table of contents
숫자를 바꾸니 남의 주문이 나왔다
코드를 보니 번호로 조회해서 그대로 보여 주고 있었다.
public function view($id) {
$order = $this->Order_model->get($id);
$this->load->view('order/view', $order);
}Order_model->get 이 WHERE order_no = ? 하나로 끝난다. 누구 주문인지를 안 본다. 로그인은 필요했지만 로그인만 하면 아무 주문이나 볼 수 있었다.
인증과 권한
여기서 구분이 필요했다. 인증은 누구인지를 확인하는 것이고 권한은 이 사람이 이것을 볼 수 있는지를 확인하는 것이다. 인증은 있고 권한이 없었다.
WHERE order_no = ? AND user_id = ? 로 소유자를 조건에 함께 넣게 고쳤다. 조회한 뒤 PHP 에서 비교하지 않고 쿼리에 넣은 것은 비교를 빠뜨릴 자리를 없애기 위해서다.
$order = $this->Order_model->get_by_user($id, $this->session->user_id);
if (!$order) { show_404(); }응답도 403 이 아니라 show_404 로 통일했다. 권한이 없다고 답하면 그 번호가 존재한다는 사실을 알려 주는 셈이기 때문이다. 번호를 훑으면서 응답이 갈리는 것을 보면 어느 번호가 존재하는지 알 수 있다. 존재 여부도 정보다.
한 곳이 아니라 패턴이었다
같은 모양이 다른 데도 있는지 찾았다.
$ grep -rn 'function view($id)' application/controllers/주문과 문의와 배송지에서 나왔고 전부 $id 만 받고 소유 확인이 없었다. 한 자리의 실수가 아니라 화면을 만드는 방식 자체가 그랬던 것이다.
같은 검사를 매번 적기보다 한 곳에 두기로 했다.
protected function require_owner($model, $id) {
$row = $model->get_by_user($id, $this->session->user_id);
if (!$row) show_404();
return $row;
}각 화면에서 한 줄로 부른다. 다만 이건 부르는 것을 잊으면 그대로다. 막아 주는 것이 아니라 부르기 쉽게 해 주는 것이다.
기본을 막는 쪽으로 갔다
더 확실한 방법은 기본을 막고 열 때만 여는 것이었다. CodeIgniter 의 컨트롤러는 public 메서드가 그대로 주소가 되니 기본이 열린 쪽이다. 기본이 막혀 있으면 여는 것을 빠뜨렸을 때 안 보인다. 빠뜨렸을 때 어느 쪽이 안전한가가 다르다.
안 보이면 바로 문의가 오고 뚫리면 아무도 모른 채 지나간다. 실수가 드러나는 쪽을 골랐다. 이미 만들어진 것을 전부 바꾸기는 컸으므로 새로 만드는 것은 기본 막힘으로 하고 기존은 공통 함수를 부르게 하는 것으로 나눴다.
주소의 번호를 uniqid 같은 값으로 바꾸는 것도 생각했는데 그건 가리는 것이지 막는 것이 아니다. 맞히면 여전히 통과한다. 소유 확인이 있어야 맞혀도 안 된다. 둘 다 하되 순서를 정해서 막는 것을 먼저 했다. 가리는 것도 값은 있다. 시도 자체가 줄어든다.
고치고 나서 access_log 에 번호만 바꿔 가며 연속으로 요청한 흔적이 있는지 봤는데 그런 패턴은 안 보였다. 다만 logrotate 가 남겨 둔 기간만 본 것이라 그 전은 모른다. 뚫려 있던 기간이 얼마인지는 알 수 없었다.
정리
- 주소에
order_no가 있으면 바꿔서 남의 것을 볼 수 있다 - 인증과 권한은 다르다. 로그인만 확인하면 부족하다
WHERE order_no = ? AND user_id = ?로 조건에 함께 넣는다- 조회 뒤 PHP 에서 비교하지 말고 쿼리에 넣어 빠뜨릴 자리를 없앤다
403이 아니라show_404로 통일한다. 존재 여부도 정보다grep -rn으로 같은 모양을 전부 찾는다. 한 자리의 실수가 아니다- 공통 함수는 부르는 것을 잊으면 그대로다
- CodeIgniter 는
public메서드가 주소가 되니 기본이 열린 쪽이다 uniqid로 가리는 것은 막는 것이 아니다. 확인이 먼저다