diff --git a/.Rbuildignore b/.Rbuildignore index 8989c62f..80a6ab4a 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -24,3 +24,7 @@ ^\.jules(/.*)?$ ^\.trivyignore\.yaml$ ^trivy\.yaml$ +^\.semgrepignore$ +^test_dummy\.R$ +^test_validation\.R$ +^tests/testthat/test-afipc-readline-validation\.R$ diff --git a/.jules/sentinel.md b/.jules/sentinel.md index a8207a48..d20c3320 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -2,3 +2,7 @@ **Vulnerability:** Unvalidated inputs passed to `if()` statements can cause process crashes (`condition has length > 1`) or unexpected coercion vulnerabilities. **Learning:** In R, optional boolean parameters that default to `NULL` should be validated using explicit runtime type validation (e.g., `if (!is.null(flag) && (!is.logical(flag) || length(flag) != 1 || is.na(flag)))`). **Prevention:** Always implement explicit runtime type validation for optional boolean parameters. +## 2024-09-06 - [R 언어에서 readline 입력의 정수 오버플로우 변환(Integer Overflow Coercion) 및 유효성 검사 취약점 수정] +**Vulnerability:** `readline()`으로 사용자 입력을 받을 때, `grepl("^[0-9]+$", n)`만을 사용하여 숫자 형태인지만 검증한 후 `as.integer(n)`를 호출하면, R의 32비트 정수 한계(약 21억)를 넘는 매우 큰 숫자 문자열(예: `99999999999`)이 입력될 경우 정수 오버플로우가 발생하여 강제로 `NA`가 반환되는 취약점(정수 변환 손실/오류)이 존재했습니다. 이는 후속 로직에서 예기치 않은 동작이나 충돌을 유발할 수 있는 보안 결함입니다. +**Learning:** 정규표현식 `^[0-9]+$`는 숫자 문자로만 이루어져 있다는 것은 보장하지만, 해당 숫자가 시스템 정수 범위 내에 속하는지는 보장하지 못합니다. 제한된 선택지(예: "1" 또는 "2")를 입력받아야 하는 상황에서 너무 포괄적인 정규표현식을 사용하는 것은 입력 유효성 검사 관점에서 불충분하며, 입력값의 길이 및 정수 변환 시의 안전성을 함께 고려해야 함을 확인했습니다. +**Prevention:** 사용자 입력을 특정 선택지로 제한할 경우 포괄적인 정규표현식 대신 `if (n %in% c("1", "2"))`와 같이 화이트리스트 기반의 명시적 값 비교(Exact string matching)를 수행하여 입력 범위를 강제하고 정수 오버플로우 발생 원인을 근본적으로 차단해야 합니다. diff --git a/DESCRIPTION b/DESCRIPTION index f31d3e1a..c90753c5 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -10,7 +10,7 @@ Description: Automates fixed item parameter linking for test linking under the item response theory paradigm using mirt package estimates. License: GPL-3 | file LICENSE Imports: mirt, methods -Suggests: testthat (>= 3.0.0) +Suggests: testthat (>= 3.0.0), mockery Encoding: UTF-8 Config/testthat/edition: 3 Config/roxygen2/version: 8.0.0 diff --git a/R/aFIPC.R b/R/aFIPC.R index 62546519..118aca09 100644 --- a/R/aFIPC.R +++ b/R/aFIPC.R @@ -141,7 +141,7 @@ autoFIPC <- } for (attempt in seq_len(3)) { n <- readline(prompt = "Is it correct? (1: Yes 2: No) : ") - if (grepl("^[0-9]+$", n)) { + if (n %in% c("1", "2")) { return(as.integer(n)) } } @@ -171,7 +171,7 @@ autoFIPC <- readline( prompt = "Do you want to use default BILOG-MG priors for oldform Data? (1: Yes 2: No) : " ) - if (grepl("^[0-9]+$", n)) { + if (n %in% c("1", "2")) { return(as.integer(n)) } } @@ -390,7 +390,7 @@ autoFIPC <- readline( prompt = "Do you want to use default BILOG-MG priors for newform Data? (1: Yes 2: No) : " ) - if (grepl("^[0-9]+$", n)) { + if (n %in% c("1", "2")) { return(as.integer(n)) } } diff --git a/tests/testthat/test-afipc-readline-validation.R b/tests/testthat/test-afipc-readline-validation.R new file mode 100644 index 00000000..4b51361c --- /dev/null +++ b/tests/testthat/test-afipc-readline-validation.R @@ -0,0 +1,43 @@ +test_that("autoFIPC validates readline input for confirmation prompt to prevent coercion vulnerabilities", { + skip_if_not_installed("mockery") + + old_model <- mirt::mirt( + data.frame(item1 = c(0, 1, 0, 1, 0), item2 = c(1, 0, 1, 0, 1), item3 = c(0, 0, 1, 1, 0)), + model = 1, + itemtype = "2PL", + SE = FALSE, + verbose = FALSE + ) + new_model <- mirt::mirt( + data.frame(item1 = c(1, 1, 0, 0, 1), item2 = c(0, 0, 1, 1, 0), item4 = c(1, 0, 0, 1, 1)), + model = 1, + itemtype = "2PL", + SE = FALSE, + verbose = FALSE + ) + + # Mock interactive mode + mockery::stub(aFIPC::autoFIPC, "interactive", TRUE) + + # RED test condition: existing grepl implementation would accept 3 and 9999999999999999999 + # GREEN test condition: we provide "3", then an oversized integer string, and finally a valid "1" + mock_readline <- mockery::mock("3", "9999999999999999999", "1", cycle = FALSE) + mockery::stub(aFIPC::autoFIPC, "readline", mock_readline) + + # Suppress the message and test for autoFIPC execution without crash + suppressMessages({ + # Expect error because the old/new models only have 3 items each and test data is small, + # leading to "Too few degrees of freedom", BUT we ensure the error is NOT about coercion/NA + expect_error( + aFIPC::autoFIPC( + newformXData = new_model, + oldformYData = old_model, + newformCommonItemNames = c("item1", "item2"), + oldformCommonItemNames = c("item1", "item2"), + confirmCommonItems = NULL, + itemtype = "2PL" + ), + "Too few degrees of freedom" # We expect the estimation to start and fail for DOF, proving we bypassed the readline crash + ) + }) +})