[CBRD-26777] enhance securities by kisoo-han · Pull Request #134 · CUBRID/cubrid-manager-server

kisoo-han · 2026-07-17T06:16:25Z

http://jira.cubrid.org/browse/CBRD-26777

Description

syetem () 형태의 code 제거
filename security 적용
CUBRID Manager Server의 빌드 환경을 VS2008에서 VS2017로 변경 (최소 수정 방안 채택)

Implementattion

filename에 사용할 수 없는 문자중 '\t' ' ' (SPACE)가 포함되어있음. 이 부분을 허용하지 않을 것인가 아니면, 허용하면서 보안을 유지하는 부분은 고려해야할 듯.
C++11 호환 코드 사용

Remarks

Windows에서도 적용되어야 하기 때문에 이전의 2개 오류를 이 PR에 포함하여 수정함
- CUBRID_VERS () macro 사용 오류
system (cmd) 형태의 호출은 다음을 제외하고, 삭제 또는 제거함
- ts_monitor_process: pgrep -u $(whoami) cmd > pid_file
- ts_stop_statdump: /bin/ps -o pid --ppid %d | grep -v PID | xargs kill
- record_iostat (): "/usr/bin/iostat -x > tmpfile
autoupdate API, ts_auto_update ()는 auto_update.sh, auto_update.bat file을 실행시킴, 보안에 위험이 있어 요청하는 경우 "not support" 오류메시지 응답하며 실패하도록 수정 (기존의 코드는 #if 0 #endif로 막아놓았음)

KEYS for reviewers

is_subpath (x, y): true if y is a subdirectory of x.
is_invalid_filename_with_msg (file, errmsg), true if filename contains Forbidden char set.
is_authorized_filename (x, errmsg), true if x does not contain disallowed characters and is a subdirectory of $CUBRID.
Windows를 고려하여 cubridmanager의 빌드 환경을 VS2008에서 VS2017로 수정합니다.
VS2017로 수정한 경우 이전 VS 버전에서 빌드한 library 링크 문제가 있어서 아래의 library를 링크에 추가하고

'legacy_stdio_definitions.lib;'

모든 main () 함수에 다음의 코드를 추가합니다.

#if _MSC_VER >= 1900
extern "C" {
    FILE __iob_func[3] = { *stdin, *stdout, *stderr };
}
#endif

수정된 빌드 환경에서 동일 코드를 10.2, 11.4에서 빌드 및 테스트 했고 (getbrokersinfo, gettransactioninfo, createdb) API로 테스트 했습니다.
@mhoh3963 , @hwany7seo , @JohnDohnut review를 부탁드립니다.

Disussion point

파일명에 공백이나 탭 문자를 허용하면, 파일명의 구분이 어렵다. CMS API에서의 파일명에서는 공백이나 탭은 제외하는 것이 권고됨

greptile-apps · 2026-07-17T06:19:12Z

_{Reviews (1): Last reviewed commit: "[CBRD-26777] remove master key" | Re-trigger Greptile}

greptile-apps · 2026-07-17T07:23:43Z

_{Reviews (2): Last reviewed commit: "[CBRD-26777] fix error for use of macro ..." | Re-trigger Greptile}

greptile-apps · 2026-07-17T07:41:45Z

_{Reviews (3): Last reviewed commit: "[CBRD-26777] rewrite return values accor..." | Re-trigger Greptile}

greptile-apps · 2026-07-17T07:50:52Z

Security Review

심볼릭 링크 순회(Symlink Traversal) (cm_server_util.cpp, Linux delete_directory): stat() 사용으로 디렉터리 내 심볼릭 링크를 따라 외부 경로를 재귀 삭제할 수 있음. lstat()으로 교체 필요.
인증 백도어 제거 (cm_server_interface.cpp): token == \"test\" 우회 조건 정상 제거. 보안 개선.
system() 호출 제거 (cm_server_util.cpp): 명령어 인젝션 위험 패턴이 직접 파일 시스템 API 호출로 교체됨. 보안 개선.

_{Reviews (4): Last reviewed commit: "[CBRD-26777] check NULL for uRemoveDir (..." | Re-trigger Greptile}

greptile-apps · 2026-07-17T08:06:54Z

_{Reviews (5): Last reviewed commit: "[CBRD-26777] replace stat to lstat for L..." | Re-trigger Greptile}

greptile-apps · 2026-07-17T08:20:21Z

_{Reviews (6): Last reviewed commit: "[CBRD-26777] do nothing at uRemoveDir ()..." | Re-trigger Greptile}

greptile-apps · 2026-07-17T08:26:23Z

_{Reviews (7): Last reviewed commit: "[CBRD-26777] fix compile error" | Re-trigger Greptile}

greptile-apps · 2026-07-17T08:42:39Z

_{Reviews (8): Last reviewed commit: "[CBRD-26777] add static keyword for dele..." | Re-trigger Greptile}

greptile-apps · 2026-07-18T01:47:48Z

_{Reviews (9): Last reviewed commit: "[CBRD-26777] check invalid filename patt..." | Re-trigger Greptile}

mhoh3963 · 2026-07-23T06:54:24Z

+#if defined (WINDOWS)
+const std::string FORBIDDEN_CHARS = " \t$&(|)><\n\r*;";
+#else
+const std::string FORBIDDEN_CHARS = " \t%&(|)><\n\r;*";


FORBIDDEN_CHARS에서 백틱(`) 추가가 필요해보입니다.
벡틱을 거르지 못하면 명령어로 인식될 문제가 발생할 것 같네요.

그럴것 같네요. backtick을 파일명에 사용할수 있는 문자에서 제외하겠습니다.
comment 감사합니다.

mhoh3963 · 2026-07-23T07:08:02Z

다음 2가지 함수는 호출부가 없거나 호출부가 #if 0로 제외되어 있습니다.
해당 함수를 삭제하거나 #if 0로 막는 것에 대해 고려해주세요.
(1) is_process_running() 내부에서 popen() 사용
(2) generate_update_script() 내부에서 system() 사용

mhoh3963 · 2026-07-23T07:13:28Z

+
+  while (i < len)
+    {
+#if defined(_WIN32) || defined(_WIN64)


매크로를 #if defined (WINDOWS) 로 통일하는 것이 좋겠습니다. (해당 파일 내부에서는 WINDOWS 로 사용하고 있습니다.)
혹시 _WIN32 와 _WIN64로 분리한 이유가 있을까요 ?

WINDOWS 도 통일하면 cub_manager.vcxproj 에서도 WINDOWS만 정의해도컴파일이 될 것 같네요.

#if defined (WINDOWS)로 통일하겠습니다.

…d in filenames

greptile-apps · 2026-07-23T07:22:27Z

_{Reviews (47): Last reviewed commit: "[CBRD-26777] add the backtick to the lis..." | Re-trigger Greptile}

greptile-apps · 2026-07-23T07:39:45Z

Security Review

경로 탐색(Path Traversal) — ext_write_private_data: is_invalid_filename_with_msg는 /와 ..을 차단하지 않아, confname = \"../../../etc/cron.d/evil\" 같은 입력이 $CUBRID 외부에 파일을 기록할 수 있습니다.
인증 백도어 제거 (긍정적 변경): cm_server_interface.cpp에서 token == \"test\" 우회 코드가 제거되어 인증 로직이 강화되었습니다.
FORBIDDEN_CHARS의 경로 구분자 미차단: /, \\, ..가 포함되지 않아 경로 탐색 시퀀스가 다수의 API에서 허용됩니다 (이전 댓글에서 지적됨).
wordexp 단어 분리(Word Splitting): 공백 포함 경로 입력 시 is_authorized_filename 검사가 잘린 경로에 수행되어 보안 게이트가 우회될 수 있습니다 (이전 댓글에서 지적됨).

_{Reviews (48): Last reviewed commit: "[CBRD-26777] do not check full path, che..." | Re-trigger Greptile}

greptile-apps · 2026-07-23T07:46:57Z

_{Reviews (49): Last reviewed commit: "[CBRD-26777] do not check full path, che..." | Re-trigger Greptile}

…ly is_invalid_filename_with_msg ()

greptile-apps · 2026-07-23T08:35:39Z

_{Reviews (50): Last reviewed commit: "[CBRD-26777] apply is_authorized_filenam..." | Re-trigger Greptile}

…erate_update_script (), is_process_running ()

kisoo-han · 2026-07-23T09:36:16Z

@mhoh3963
is_process_running(), generate_update_script()는 definition/reference 모두 삭제하겠습니다.

greptile-apps · 2026-07-23T09:51:11Z

_{Reviews (51): Last reviewed commit: "[CBRD-26777] remove unused function, bot..." | Re-trigger Greptile}

…s respectively

greptile-apps · 2026-07-23T10:01:28Z

Security Review

Path Traversal (ext_write_private_data): confname이 알파 문자로 시작하는 경우 is_invalid_filename_with_msg만 호출되어 /가 포함된 경로 탐색 문자열이 검사를 통과함. \"a/../../../etc/cron.d/evil\" 패턴으로 $CUBRID 외부 임의 파일에 쓰기 가능 — server/src/cm_server_extend_interface.cpp:1164
하드코딩 테스트 백도어 제거: cm_server_interface.cpp에서 token == \"test\" 우회 경로가 제거되어 인증 강화됨 (긍정적 변경)

_{Reviews (52): Last reviewed commit: "[CBRD-26777] define macro UNLINK for uni..." | Re-trigger Greptile}

greptile-apps · 2026-07-23T10:25:23Z

_{Reviews (53): Last reviewed commit: "[CBRD-26777] replace system (rm file) to..." | Re-trigger Greptile}

greptile-apps · 2026-07-23T10:40:49Z

_{Reviews (54): Last reviewed commit: "[CBRD-26777] limit conf filename, by gre..." | Re-trigger Greptile}

greptile-apps · 2026-07-23T10:40:56Z

+  if (std::isalpha (static_cast <unsigned char> (confname.c_str ()[0])))
+    {
+      if (attempt_to_access_parent_dir (confname.c_str ()))
+	{
+	  snprintf (_dbmt_error, "filename type is not allowed: %s", confname.c_str ());
+	  return build_server_header (response, ERR_FILE_OPEN_FAIL, _dbmt_error);
+	}
+
+      if (is_invalid_filename_with_msg (confname.c_str (), _dbmt_error))
+	{
+	  return build_server_header (response, ERR_FILE_OPEN_FAIL, _dbmt_error);
+	}
+    }
+  else
+    {
+      if (!is_authorized_filename  (confname.c_str (), _dbmt_error))
+	{
+	  return build_server_header (response, ERR_FILE_OPEN_FAIL, _dbmt_error);
+	}
+    }


static 함수를 외부 번역 단위에서 호출 — 컴파일 오류

attempt_to_access_parent_dir는 cm_server_util.cpp 안에서 static bool로 선언·정의되어 있어 해당 파일 내부에서만 링키지가 유효합니다. cm_server_util.h에도 선언이 없으므로, cm_server_extend_interface.cpp에서 이를 호출하면 "undeclared function" 컴파일 오류가 발생합니다. 이 함수를 헤더에 공개하거나, 대신 이미 공개된 is_authorized_filename / is_invalid_filename_with_msg 조합으로 검증 로직을 대체해야 합니다.

Suggested change

if (std::isalpha (static_cast <unsigned char> (confname.c_str ()[0])))

{

if (attempt_to_access_parent_dir (confname.c_str ()))

{

snprintf (_dbmt_error, "filename type is not allowed: %s", confname.c_str ());

return build_server_header (response, ERR_FILE_OPEN_FAIL, _dbmt_error);

}

if (is_invalid_filename_with_msg (confname.c_str (), _dbmt_error))

{

return build_server_header (response, ERR_FILE_OPEN_FAIL, _dbmt_error);

}

}

else

{

if (!is_authorized_filename (confname.c_str (), _dbmt_error))

{

return build_server_header (response, ERR_FILE_OPEN_FAIL, _dbmt_error);

}

}

if (!is_authorized_filename (confname.c_str (), _dbmt_error))

{

return build_server_header (response, ERR_FILE_OPEN_FAIL, _dbmt_error);

}

greptile-apps · 2026-07-23T11:15:05Z

_{Reviews (55): Last reviewed commit: "[CBRD-26777[ fix error" | Re-trigger Greptile}

greptile-apps · 2026-07-23T11:15:09Z

+  </ItemDefinitionGroup>
+  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
+    <Midl>
+      <TargetEnvironment>X64</TargetEnvironment>


Release|x64 빌드의 스택 버퍼 오버플로우 보호 비활성화

cub_manager.vcxproj의 Release|x64 구성에서 <BufferSecurityCheck>false</BufferSecurityCheck>로 설정되어 있어 MSVC의 /GS 스택 카나리 보호가 비활성화됩니다. 보안 강화를 목적으로 하는 이 PR에서 보안 핵심 바이너리의 스택 오버플로우 탐지를 꺼두는 것은 방향과 반대입니다. 참고로 동일 PR에서 추가된 cm_admin.vcxproj의 Release|x64는 <BufferSecurityCheck>true</BufferSecurityCheck>로 올바르게 설정되어 있습니다. cub_manager.vcxproj도 동일하게 true로 변경해야 합니다.

[CBRD-26777] remove master key

177af0c

kisoo-han self-assigned this Jul 17, 2026

kisoo-han added 2 commits July 17, 2026 16:15

[CBRD-26777] rewrite uRemoveDir () to aoid use of system(cmd)

379241d

[CBRD-26777] fix error for use of macro CUBRID_VERS ()

f678991

greptile-apps Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread server/src/cm_server_util.cpp Outdated

Comment thread server/src/cm_server_util.cpp Outdated

Comment thread server/src/cm_server_util.cpp Outdated

kisoo-han added 3 commits July 17, 2026 16:27

[CBRD-26777] remove UNDO flag

df7801b

[CBRD-26777] use greptile comments

dd70bb9

[CBRD-26777] rewrite return values according to greptile comments

701b0c6

greptile-apps Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread server/src/cm_server_util.cpp

[CBRD-26777] check NULL for uRemoveDir ()

5676b17

greptile-apps Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread server/src/cm_server_util.cpp Outdated

Comment thread server/src/cm_server_util.cpp Outdated

kisoo-han added 2 commits July 17, 2026 16:59

[CBRD-26777] to aaoid possible overflow

868df50

[CBRD-26777] replace stat to lstat for Linux

7c6c692

greptile-apps Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread server/src/cm_server_util.cpp

[CBRD-26777] do nothing at uRemoveDir () if target does not exist

59e6afb

greptile-apps Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread server/src/cm_server_util.cpp Outdated

Comment thread server/src/cm_server_util.cpp Outdated

[CBRD-26777] fix compile error

c1920f3

[CBRD-26777] add static keyword for delete_directory () win

181aec7

[CBRD-26777] check invalid filename pattern

450eeec

greptile-apps Bot reviewed Jul 18, 2026

View reviewed changes

Comment thread server/src/cm_server_util.cpp

[CBRD-26777] reply directory name, when uCreateDir failed

cdccb4a

kisoo-han requested a review from hwany7seo July 23, 2026 05:39

mhoh3963 reviewed Jul 23, 2026

View reviewed changes

[CBRD-26777] add the backtick to the list of chars that cannot be use…

0aa32a4

…d in filenames