diff --git a/.jules/sentinel.md b/.jules/sentinel.md index e7253d0f531731..d55428b43b5f75 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -102,6 +102,10 @@ **Vulnerability:** In `package/network/config/netifd/files/lib/netifd/utils.uc`, the `handler_load` function iterates over `.sh` scripts in a directory and uses their `basename` to execute them via a string interpolated `system()` call (`system("./${script} ...")`). If an attacker could place a maliciously named file in the parsed directory (e.g., `$(touch \/tmp\/pwned).sh`), it would result in arbitrary command execution. **Learning:** In `ucode` scripts, `system()` with a string argument is executed by the shell (`/bin/sh -c`). When using variables derived from filenames or external sources within these string templates, failure to sanitize allows shell metacharacter injection. **Prevention:** To protect `system()` or `fs.popen()` when string interpolation is unavoidable, always strictly validate variables using regex allowlists (e.g., `if (match(script, /[^a-zA-Z0-9_.-]/)) continue;`) to ensure only safe characters are evaluated by the shell. +## 2024-05-24 - Buffer Overflow via `sprintf` in tinysrp/tphrase.c +**Vulnerability:** A buffer overflow vulnerability existed because `sprintf` was used to write to dynamically allocated buffers (`bakfile` and `bakfile2`) without bounds checking. While the buffer size was mathematically exact (`strlen(pwname) + 5`), `sprintf` is inherently unsafe and flagged by security scanners. +**Learning:** `sprintf` lacks bounds checking, making it an anti-pattern even when buffer sizes are "known" to be correct, as subsequent changes to the formatting string or length calculation could easily introduce overflows. +**Prevention:** Always use bounded string formatting functions like `snprintf` instead of `sprintf`. Calculate and reuse the allocation size variable for both the buffer allocation and the `snprintf` bounds check to ensure consistency and prevent off-by-one errors. ## 2026-05-22 - [Fix buffer overflow risk in AR8327 LED driver] **Vulnerability:** Unbounded `strcpy` used for copying dynamic string into flexible array member without reusing precalculated length. **Learning:** In kernel modules, `strcpy` should be replaced with `strscpy`, and size variables should be precalculated to avoid TOCTOU races between allocation and copying. diff --git a/my_commit_message.txt b/my_commit_message.txt new file mode 100644 index 00000000000000..a2b694a38a51b4 --- /dev/null +++ b/my_commit_message.txt @@ -0,0 +1,9 @@ +ead: Replace unsafe `sprintf` with `snprintf` in tphrase.c + +The buffer 'bakfile' and 'bakfile2' were allocated as +`strlen(pwname) + 5`, which is exactly enough for '.bak\0'. However, using +`sprintf` is unsafe and bad practice compared to `snprintf`. This commit +replaces `sprintf` with `snprintf` using an explicit `alloc_size` length +boundary to prevent potential buffer overflows. + +Signed-off-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> diff --git a/package/network/services/ead/src/tinysrp/tphrase.c b/package/network/services/ead/src/tinysrp/tphrase.c index e1188f5fe83179..0cf7e814ac8f36 100644 --- a/package/network/services/ead/src/tinysrp/tphrase.c +++ b/package/network/services/ead/src/tinysrp/tphrase.c @@ -129,24 +129,28 @@ t_changepw(pwname, diff) FILE * passfp; FILE * bakfp; + size_t alloc_size; + if(pwname == NULL) pwname = DEFAULT_PASSWD; if((passfp = fopen(pwname, "rb")) == NULL || fstat(fileno(passfp), &st) < 0) return -1; - if((bakfile = malloc(strlen(pwname) + 5)) == NULL) { + alloc_size = strlen(pwname) + 5; + + if((bakfile = malloc(alloc_size)) == NULL) { fclose(passfp); return -1; } - else if((bakfile2 = malloc(strlen(pwname) + 5)) == NULL) { + else if((bakfile2 = malloc(alloc_size)) == NULL) { fclose(passfp); free(bakfile); return -1; } - sprintf(bakfile, "%s.bak", pwname); - sprintf(bakfile2, "%s.sav", pwname); + snprintf(bakfile, alloc_size, "%s.bak", pwname); + snprintf(bakfile2, alloc_size, "%s.sav", pwname); if((bakfp = fopen(bakfile2, "wb")) == NULL && (unlink(bakfile2) < 0 || (bakfp = fopen(bakfile2, "wb")) == NULL)) {