diff --git a/contributions/65659.md b/contributions/65659.md new file mode 100644 index 0000000..867829b --- /dev/null +++ b/contributions/65659.md @@ -0,0 +1,176 @@ +--- +pr-url: https://github.com/nodejs/node/pull/65659 +--- + +## 문제 내용 + +`--permission-audit`는 [문서상](https://nodejs.org/api/cli.html#--permission-audit) 권한 검사를 수행하되 **차단하지 않고** diagnostics channel로 위반 사항만 발행하는 모드입니다. 그런데 세 경로가 이 계약을 어기고 실제로 차단하고 있었습니다([#65419](https://github.com/nodejs/node/issues/65419), 신고자 @naugtur). + +로컬 빌드(v27.0.0-pre)로 재현한 결과입니다. + +| API | 플래그 없음 | `--permission` | `--permission-audit` | 판정 | +| --- | --- | --- | --- | --- | +| `fs.statSync` | OK | 차단 | OK | 정상 | +| `fs.lstatSync` | OK | 차단 | **차단** | 버그 | +| `fs.symlinkSync` | OK | 차단 | **차단** | 버그 | +| `process.dlopen` | — | 차단 | **차단** | 버그 | + +같은 fs 모듈인데 `stat`은 정상이고 `lstat`은 차단된다는 비대칭이 원인 추적의 실마리였습니다. + +### 원인 1 — JS 계층 검사가 audit 모드를 확인하지 않음 + +C++ 매크로는 audit 모드를 존중합니다(`src/permission/permission.h`). + +```cpp +if (!is_granted(...)) { // 여기서 audit 이벤트 발행 + if (!warning_only()) { // audit 모드면 throw 생략 + ThrowAccessDenied(...); +``` + +그런데 `lstat`과 `symlink`의 검사는 C++이 아니라 JS 계층에 있고, 두 번째 단계가 없었습니다. + +```js +// lib/fs.js +if (permission.isEnabled() && !permission.has('fs.read', path)) { + throw new ERR_ACCESS_DENIED(...); // audit 모드에서도 무조건 throw +} +``` + +`permission.isEnabled()`는 `--permission || --permission-audit`로 정의되어 audit 모드에서도 `true`입니다. `stat`이 정상이었던 이유는 검사가 C++ 매크로 경로에 있어서였습니다. + +### 원인 2 — dlopen은 권한 검사에 도달하기 전에 차단 + +`src/env.cc`가 audit 모드에도 애드온을 비활성화하고, `DLOpen()`이 audit 인식 매크로보다 **먼저** 그 플래그를 검사합니다. + +```cpp +// src/node_binding.cc +if (env->no_native_addons()) { + return THROW_ERR_DLOPEN_DISABLED(...); // audit 모드 무시 +} +THROW_IF_INSUFFICIENT_PERMISSIONS(env, kAddon, ""); // 여기까지 도달 못 함 +``` + +에러 코드부터 `ERR_ACCESS_DENIED`가 아닌 `ERR_DLOPEN_DISABLED`이고, **audit 이벤트조차 발행되지 않습니다**. 관측이 목적인 모드에서 관측 자체가 불가능한 상태였습니다. + +## 해결 과정과 검증 + +| 항목 | 내용 | +| --- | --- | +| 분류 태그 | 버그수정 | +| 상태 | Open (2026.08.30 제출, 커밋 `4efc964`) | +| 변경 규모 | 5개 파일, +184 / -7, 커밋 1개 | +| 리뷰 요청 | @nodejs/security-wg (CODEOWNERS 자동 라우팅) | + +### 선례 확인 — 헬퍼는 이미 있었다 + +수정 방향을 정하기 전에 기존 구현을 먼저 뒤졌습니다. `permission.isAuditMode()` 헬퍼가 이미 존재했고, `lib/ffi.js`에서만 쓰이고 있었습니다. + +```js +// lib/ffi.js — 올바른 선례 +if (permission.has('ffi') || permission.isAuditMode()) { + return; +} +``` + +`git log -S "isAuditMode" --oneline -- lib/` 결과 커밋이 하나뿐이었습니다(#63047). 즉 헬퍼가 만들어질 때 ffi에만 배선되고 **fs는 누락**된 것이었고, 새 API를 만들 필요 없이 같은 패턴을 확장하면 되는 문제였습니다. + +### 수정 1 — JS 계층 6곳 + +`lib/fs.js` 4곳(`lstat`, `lstatSync`, `symlink`, `symlinkSync`)과 `lib/internal/fs/promises.js` 2곳입니다. + +```js +if (permission.isEnabled() && !permission.has('fs.read', path) && + !permission.isAuditMode()) { +``` + +**조건 순서가 핵심입니다.** audit 이벤트는 `permission.has()`가 타고 들어가는 C++ `Permission::is_scope_granted()`에서 발행됩니다. `isAuditMode()`를 앞에 두면 단축 평가로 `has()`가 호출되지 않아 이벤트가 사라집니다. 관측 기능을 살리려면 `has()`가 먼저 평가되어야 하고, 이는 `lib/ffi.js`의 기존 패턴과도 일치합니다. + +### 수정 2 — src/env.cc + +```cpp +if (!options_->allow_addons) { + // In audit mode addon loading must stay enabled: the denial is + // published through the diagnostics channel by the permission + // check in DLOpen() instead of being rejected upfront. + if (!options_->permission_audit) { + options_->allow_native_addons = false; + } + permission()->Apply(this, args, permission::PermissionScope::kAddon); +} +``` + +`Apply(kAddon)`은 그대로 두었습니다. `kAddon` 스코프가 거부 상태로 남아야 `DLOpen()`의 audit 인식 매크로가 이벤트를 발행하고, `warning_only()`에 의해 throw만 생략되기 때문입니다. 이 한 곳을 고치면 `src/node_worker.cc`의 worker 전파 경로도 함께 해결되어(같은 플래그를 상속) 추가 수정이 불필요했습니다. + +### 신규 테스트 2개 + +기존 `test-permission-audit-fs-does-not-deny.js`의 구조(`spawnSync` + 자식 프로세스에서 `dc.subscribe` + stdout 파싱)를 그대로 따랐습니다. + +- `test-permission-audit-fs-lstat-symlink-does-not-deny.js` — audit 모드에서 통과 + 이벤트의 `permission`/`resource` 단언 +- `test-permission-audit-addons-does-not-deny.js` — audit 모드에서 `ERR_DLOPEN_DISABLED`가 아닌 `ERR_DLOPEN_FAILED`인지 단언(실제 `.node` 파일 없이 검증 가능) + +각 테스트에 **`--permission` 강제 모드 단언을 대조군으로** 넣었습니다. 완화가 강제 모드로 새지 않았다는 증명이 없으면 보안 관련 변경은 리뷰를 통과하기 어렵다고 판단했습니다. + +### 검증 순서 + +문서(`doc/contributing/writing-tests.md`)의 규칙을 따르되, 순서를 의도적으로 잡았습니다. + +1. **수정 전 바이너리에서 신규 테스트를 먼저 실행** — audit 케이스 3개만 실패하고 강제 모드 대조군 3개는 통과. 테스트가 실제로 이 버그를 잡는다는 증거를 먼저 확보했습니다. +2. 수정 후 3-모드 재현 매트릭스 재실행 — audit 모드가 플래그 없는 기준선과 완전히 일치, 강제 모드는 무변화 +3. `python3 tools/test.py --mode=release -J "parallel/test-permission*"` — 75/75 통과 +4. `ffi/test-ffi-permission-audit`, `parallel/test-fs-symlink*` — 통과 +5. `make test JOBS=14` 전체 스위트 — cctest 222개, jstest 5,942개, 실패 0 +6. `eslint`(JS 4파일), `python3 tools/cpplint.py src/env.cc` — 통과 + +### 판단이 필요했던 지점 — CVE 관련 코드 완화 + +`symlink` 제한은 [CVE-2025-55130](https://github.com/nodejs/node/commit/3296b2e734c) 대응으로 들어간 코드였습니다(2025-11, 비공개 보안 PR). 보안 조치를 완화하는 변경이라 근거가 필요했습니다. + +`git log`로 시점을 확인한 결과, 해당 CVE 커밋은 audit 모드 도입(#61869, 2026-05)보다 **앞서 있어 audit을 고려할 수 없었던** 상태였습니다. 의도적 예외가 아니라 시점상 누락으로 판단했고, 이 근거와 함께 **반대 대안**(symlink만 예외로 유지하고 문서에 명시)도 PR 본문에 병기해 security-wg가 판정할 수 있게 했습니다. + +### 신고자 응대 + +이슈 본문에 신고자가 "직접 고치고 싶다, 포인터가 필요하다"고 적어둔 상태였고 열흘간 답이 없었습니다. 선점 문제를 피하기 위해 이슈에 **원인 분석 코멘트를 먼저 올리고**(요청받은 포인터), PR 본문과 코멘트 양쪽에 "직접 작업 중이셨다면 기꺼이 양보하겠다"는 의사를 명시했습니다. + +## 배운 점 + +### 1. 한 함수만 읽고 내린 판단은 뒤집힐 수 있다 + +이번 조사 중, 앞서 머지된 제 문서 PR(#64888)이 틀렸다고 판단한 순간이 있었습니다. `Stream::SendStopSending()`을 보면 `EndReadable()`이 방향 검사보다 먼저 무조건 실행되기 때문입니다. + +```cpp +void Stream::SendStopSending(error_code code) { + EndReadable(); // 무조건 실행되는 것처럼 보임 + if (!is_pending()) { + if (is_local_unidirectional()) return; +``` + +한 겹 더 내려가 보니 `is_readable()`이 로컬 단방향 스트림에 `false`를 반환하고, `EndReadable()`의 첫 줄이 `if (!is_readable()) return;`이었습니다. **결국 문서가 맞았습니다.** + +Node 코어에서 `is_readable()`, `is_writable()`, `is_pending()` 같은 술어 함수는 조건을 감추는 자리입니다. 호출 그래프를 한 겹만 보고 내린 결론은 정의까지 펼치면 반대로 뒤집힐 수 있다는 걸 실제로 겪었습니다. + +### 2. 이슈 목록의 빈자리는 대부분 이유가 있다 + +기여할 이슈를 찾으면서 열린 이슈 200건을 GraphQL로 교차참조와 함께 받아 분류했습니다. + +| 분류 | 건수 | +| --- | --- | +| 이미 열린 PR 있음 | 105건 (53%) | +| PR 전혀 없음 | 61건 | +| 머지된 PR만 있음(이미 수정됨) | 21건 | +| 닫힌 PR만 있음(시도 실패) | 13건 | + +절반 이상이 선점 상태였고, 비어 있는 61건도 본문을 읽어보면 대부분 이유가 있었습니다. 메인테이너가 "버그가 아니다"라고 판단했거나, 신고자가 철회했거나, 설계가 미확정이거나, 신고자 본인이 착수 예정이었습니다. 실제로 `#65509`는 신고자가 "구현 전 방향 합의를 구한다"고 적어둔 상태였고, `#63305`은 jasnell이 "backpressure model is still an open question"이라고 답한 상태여서 둘 다 제외했습니다. + +**교차참조만으로는 "PR 없음"까지만 알 수 있고, 임자·반대·철회는 스레드를 읽어야 드러납니다.** 자동 필터로 후보를 좁히되 최종 판단은 본문 정독이라는 절차를 세우게 되었습니다. + +### 3. 빌드 트리는 브랜치를 따라오지 않는다 + +한 달 전 빌드해둔 트리에서 새 브랜치를 만들고 `ninja`를 돌렸더니 `child_process_permission.cc` 없음 오류가 났습니다. 그 사이 `src/permission/`이 리팩토링되어(파일 통합, `BooleanPermissions` 도입) 기존 `build.ninja`가 무효화된 것이었고, `./configure --ninja` 재실행이 필요했습니다. + +또 하나, `make -j14 test`가 조용히 실패했습니다. 이 저장소의 Makefile은 `-j` 대신 `JOBS=14` 변수를 요구하는데, 잘못된 호출이 ninja에 빈 `-j`를 넘겨 즉시 죽었고 `tail`이 종료 코드를 삼켜 성공처럼 보였습니다. 출력 파일을 직접 열어보고서야 발견했습니다. **긴 명령의 결과는 종료 코드가 아니라 출력 내용으로 확인해야 한다**는 교훈입니다. + +### 4. AI 사용 정책은 문서화되어 있다 + +`doc/contributing/ai-guidelines.md`가 공식 정책으로 존재합니다. AI 사용을 금지하지 않지만 **도구 사용 사실과 개인 검증 내용의 공개**를 요구하고, 특히 리뷰 피드백 응답의 AI 자동화를 명시적으로 금지합니다. + +관례를 확인하기 위해 `git log --grep="Assisted-by"`를 돌려보니 랜딩 커밋이 198개 있었고, TSC 멤버들도 사용하고 있었습니다. 이 형식을 따라 커밋에 `Assisted-by` 트레일러를 남기고, PR 본문에 직접 수행한 검증(재현, 소스 추적, 테스트 실행)을 밝혔습니다. 리뷰 응답은 정책에 따라 직접 작성할 예정입니다.