I don't see where a buffer overflow is possible in the original code, but I'm a hobbyist programmer and probably misunderstood something. Perhaps you'd be willing to post the exploit code you used to demonstrate trigger this overflow? It would be very helpful to understand how it works. I think 8 bytes *3 is 24 and I think in decimal the largest possible number is 18 digits or so, and I'm pretty sure sprintf replaces %u which is another 2 bytes, and since 18 < 26 I'm not seeing it. What am I missing? And wouldn't using snprintf be slower due to the bounds checks and slower due to more function call overhead from that extra parameter required for the bound?
On September 27, 2025 2:59:00 PM EDT, Osama Abdelkader <[email protected]> wrote: >Replace all sprintf calls with snprintf to prevent potential buffer >overflows >when formatting /proc paths with variable PID values. > >All calls now use snprintf with sizeof(filename) to ensure bounds >checking. > >Signed-off-by: Osama Abdelkader <[email protected]> >--- > libbb/procps.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > >diff --git a/libbb/procps.c b/libbb/procps.c >index de640d29e..613cfd021 100644 >--- a/libbb/procps.c >+++ b/libbb/procps.c >@@ -197,7 +197,7 @@ static NOINLINE void procps_read_smaps(pid_t pid, >procps_status_t *sp) > char filename[sizeof("/proc/%u/smaps") + sizeof(int)*3]; > char buf[PROCPS_BUFSIZE] ALIGN4; > >- sprintf(filename, "/proc/%u/smaps", (int)pid); >+ snprintf(filename, sizeof(filename), "/proc/%u/smaps", (int)pid); > > file = fopen_for_read(filename); > if (!file) >@@ -304,7 +304,7 @@ procps_status_t* FAST_FUNC >procps_scan(procps_status_t* sp, int flags) > /* We found another /proc/PID. Do not use it, > * there will be /proc/PID/task/PID (same PID!), > * so just go ahead and dive into /proc/PID/task. */ >- sprintf(filename, "/proc/%u/task", pid); >+ snprintf(filename, sizeof(filename), "/proc/%u/task", >pid); > /* Note: if opendir fails, we just go to next /proc/XXX > */ > sp->task_dir = opendir(filename); > sp->main_thread_pid = pid; >@@ -332,10 +332,10 @@ procps_status_t* FAST_FUNC >procps_scan(procps_status_t* sp, int flags) > > #if ENABLE_FEATURE_SHOW_THREADS > if (sp->task_dir) >- filename_tail = filename + sprintf(filename, >"/proc/%u/task/%u/", >sp->main_thread_pid, pid); >+ filename_tail = filename + snprintf(filename, >sizeof(filename), >"/proc/%u/task/%u/", sp->main_thread_pid, pid); > else > #endif >- filename_tail = filename + sprintf(filename, >"/proc/%u/", pid); >+ filename_tail = filename + snprintf(filename, >sizeof(filename), >"/proc/%u/", pid); > > if (flags & PSSCAN_UIDGID) { > struct stat sb; >@@ -560,7 +560,7 @@ int FAST_FUNC read_cmdline(char *buf, int col, >unsigned pid, const char *comm) > int sz; > char filename[sizeof("/proc/%u/cmdline") + sizeof(int)*3]; > >- sprintf(filename, "/proc/%u/cmdline", pid); >+ snprintf(filename, sizeof(filename), "/proc/%u/cmdline", pid); > sz = open_read_close(filename, buf, col - 1); > if (sz < 0) > return sz; >-- >2.43.0 > >_______________________________________________ >busybox mailing list >[email protected] >https://lists.busybox.net/mailman/listinfo/busybox
_______________________________________________ busybox mailing list [email protected] https://lists.busybox.net/mailman/listinfo/busybox
