🔒 Replace unsafe sprintf with snprintf in tphrase.c

.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.

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>

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)) {