gnuboard / gnuboard5

그누보드5 (영카트 포함) 공개형 Git
Other
308 stars 247 forks source link

HTTP 호스트를 보조할 공통의 래퍼(Wrapper) 함수 작성 #261

Open gnh1201 opened 1 year ago

gnh1201 commented 1 year ago

요약

HTTP 호스트를 보조할 공통의 래퍼(Wrapper) 함수 작성이 필요함.

상세

  1. 현 그누보드5 내에서 HTTP_HOST 환경 변수가 사용되는 부분을 모두 검색한 결과, 16개의 파일이 검색됨. https://github.com/search?q=repo%3Agnuboard%2Fgnuboard5%20HTTP_HOST&type=code
  2. HTTP_HOST가 올바른 주소를 가리키지 않는 경우를 보완하기 위해, SERVER_NAME, HTTP_REFERER 등의 다른 변수도 종종 같이 사용하고 있음.
  3. 실질적으로는 여기에 HTTP_X_FORWARDED_HOST 까지 추가되어야 한다면 HTTP_HOST를 보조하는 경우의 수가 더 많아짐. #260
  4. HTTP_HOST 환경변수를 보조할 수 있는 공통의 함수(Wrapper 함수) 작성 필요함.
  5. 잠재적으로 이 부분은 Samesite, CORS 등의 문제와 연관이 있을 수 있음.
kkigomi commented 1 year ago

G5_DOMAIN, G5_HTTPS_DOMAIN 설정을 신뢰하여 활용하도록 개선하는 게 낫지 않을까 싶습니다.

260 처럼 제한된 목적으로 사용할 경우에는 문제가 없을 수 있으나 HTTP_X_FORWARDED_HOST을 비롯한 헤더는 쉽게 설정할 수 있기 때문에 이를 활용하는 wrapper 함수가 제공되고 다양한 목적으로 사용될 때 손쉽게 host를 위조 할 수 있는 문제가 발생할 수 있어보입니다.

이는 활용 방법에 따라 전혀 문제가 되지 않을 수 있으나 HTTP_X_FORWARDED_HOST 헤더로 조작된 host 정보를 이용해 위장하거나 엉뚱한 데이터를 생성하는 등 보안상 문제를 발생시킬 우려가 있습니다.

$_SERVER['HTTP_HOST'] 대신 G5_DOMAIN 상수를 신뢰하여 사용하게하고 HTTP_X_FORWARDED_HOST를 필요에 따라 사용할 수 있게하는 방향이 좋을 것 같습니다. 비록, 자동으로 처리되는 게 아니라서 불편은 있겠지만 좀 더 안전하다고 생각합니다.

// config.php 기본 값
define('G5_DOMAIN', $_SERVER['HTTP_HOST']);

// 필요에 따라 변경하여 사용
define('G5_DOMAIN', $_SERVER['HTTP_X_FORWARDED_HOST']);
gnh1201 commented 1 year ago

말씀주신 변경사항이 의도대로 작동하기 위해선 결국 #260 같은 변경이 이루어져야 한다는게 문제입니다.

// config.php 기본 값
define('G5_DOMAIN', $_SERVER['HTTP_HOST']);

// 필요에 따라 변경하여 사용
define('G5_DOMAIN', $_SERVER['HTTP_X_FORWARDED_HOST']);

결국엔 check_url_host() 함수에서 HTTP_HOST만 가지고 맞춰보기 때문에 이 코드는 실패하게 됩니다.

kkigomi commented 1 year ago

@gnh1201

// config.php 파일에서 HTTP_X_FORWARDED_HOST 환경변수를 사용하도록 사용자 필요에 따라 변경
define('G5_DOMAIN', $_SERVER['HTTP_X_FORWARDED_HOST']);
// common.lib.php
function check_url_host(...) {
...
// 이 코드를
$host = preg_replace('/:[0-9]+$/', '', $_SERVER['HTTP_HOST']);
// 아래와 같이 변경
$host = preg_replace('/:[0-9]+$/', '', G5_DOMAIN);
...
}

이렇게 check_url_host 함수를 바꾸면 되지 않을까요? 환경 상 필요에 따라 define('G5_DOMAIN', $_SERVER['HTTP_X_FORWARDED_HOST']); 이처럼 설정을 변경하면 되고요.

사실 필요하다면 $_SERVER['HTTP_HOST'] = $_SERVER['HTTP_X_FORWARDED_HOST']; 이렇게 해도 문제가 없겠죠.

gnh1201 commented 1 year ago

@kkigomi

넵 ㅎㅎ 저도 HTTP_X_FORWARDED_HOST에 한정에서는 #260 보다는 말씀주신 변경 사항이 더 좋아보입니다!

그 다음 고민이 있다면, 굳이 HTTP_X_FORWARDED_HOST가 아니더라도, 기존에도 있는 HTTP_HOST 변수에 문제가 있는 경우를 보완하기 위해 SERVER_NAME 등을 가져오는 경우까지 깔끔하게 통합할 수 있는 방안이 있으면 더 좋을 것 같습니다.

kkigomi commented 1 year ago

@gnh1201 host를 판별하는데 wrapper 함수를 이용하는 것에 대해서는 HTTP_X_FORWARDED_HOST자동으로 사용되는 것에 대한 문제는 우려됩니다. 다른 이슈에서 프로토콜이나 포트를 대체하는 것까지는 어떤 문제가 없을지는 모르겠으나 호스트는 wrapper 함수를 사용하는 곳에 따라 다른 문제가 발생할 수 있는 우려가 있어 보이기 때문입니다.

흔히 문제가 되는 IP(REMOTE_ADDR) 또한 HTTP_X_FORWARDED_FOR를 대체 사용하도록 변경하면 편리하지만 이 역시 손쉽게 위조되기 때문에 이 환경변수, 헤더가 있다고해서 일반적으로 신뢰할 수 있다고 보기는 어려우니까요.

프록시를 두고있는 환경에서나 신뢰가 가능한 데이터이지, 그렇지 않은 더 많은 일반적인 사용 환경에서는 손쉽게 위장이 가능한 문제가 발생합니다.

고로, #260 PR과 더불어 이런 변경은 프록시, 웹서버에서 HTTP_HOST, REMOTE_ADDR 등을 직접 변경하도록 하거나, 비록 불편하지만 $_SERVER['HTTP_HOST'] = $_SERVER['HTTP_X_FORWARDED_HOST']; 이런 코드를 사용해 변경하는 것이 일반적인 사용 환경에 영향을 주지 않을 것 같습니다.

gnh1201 commented 1 year ago

일단, HTTP_X_FORWARDED_HOST를 기준에 추가한 이후로 전반적으로 CSRF 검증과 관련된 부분들에 문제가 있는 것 같긴 합니다.

다른 곳들은 코드 수정해서 해결은 했다고는 하는데 어떤 부분을 수정했는지는 정리되어 있는게 없다보니, 연구를 더 해봐야할 것 같네요. ㅠㅠ

gnh1201 commented 1 year ago

당장의 사용을 위해 @kkigomi 님의 제안대로 아래와 같이 common.php 맨 위에서 HTTP_HOST 변수를 바꿔주었습니다.

$_SERVER['HTTP_HOST'] = $_SERVER['HTTP_X_FORWARDED_HOST'];

다만, 향후에 더 깔끔한 방법이 나오는 것을 내심 기대하고 있어서 이슈는 제가 닫지는 않겠습니다.