Re: [PATCH] libbb/procps: replace sprintf with snprintf for memory safety

[Jody Bruchon]

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 
 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 
>---
> 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
>busybox@busybox.net
>https://lists.busybox.net/mailman/listinfo/busybox
___
busybox mailing list
busybox@busybox.net
https://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] libbb/procps: replace sprintf with snprintf for memory safety

[Osama Abdelkader]

On Sat, Sep 27, 2025 at 08:26:17PM +0100, Harald van Dijk wrote:
> On 27/09/2025 19:59, Osama Abdelkader wrote:
> > Replace all sprintf calls with snprintf to prevent potential buffer 
> > overflows
> > when formatting /proc paths with variable PID values.
> 
> In the context of your first change, it shows filename is defined as
> 
>   char filename[sizeof("/proc/%u/smaps") + sizeof(int)*3];
> 
> to ensure that the buffer is always large enough. The same looks true for
> the others as well.
> 
> If you want to change busybox to account for the possibility that the buffer
> size calculation is wrong anywhere, then please keep in mind that operating
> on a truncated file name, as your patch would make it do, would be doing the
> wrong thing.
> 
> Cheers,
> Harald van Dijk

Thank you for the feedback. You're absolutely correct.

After analyzing the buffer size calculations:
- sizeof("/proc/%u/smaps") = 15
- sizeof(int)*3 = 12  
- Total buffer = 27 bytes
- Max PID string length = 10 bytes
- Result: "/proc/2147483647/smaps" = 22 bytes

The buffers are indeed correctly sized to handle maximum PID values. 
My patch would have caused truncation, leading to incorrect behavior.

I'll look for actual memory safety issues where buffers are genuinely 
undersized or bounds checking is missing.
___
busybox mailing list
busybox@busybox.net
https://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] libbb/procps: replace sprintf with snprintf for memory safety

[Harald van Dijk]

On 27/09/2025 19:59, Osama Abdelkader wrote:

Replace all sprintf calls with snprintf to prevent potential buffer overflows
when formatting /proc paths with variable PID values.


In the context of your first change, it shows filename is defined as

  char filename[sizeof("/proc/%u/smaps") + sizeof(int)*3];

to ensure that the buffer is always large enough. The same looks true 
for the others as well.


If you want to change busybox to account for the possibility that the 
buffer size calculation is wrong anywhere, then please keep in mind that 
operating on a truncated file name, as your patch would make it do, 
would be doing the wrong thing.


Cheers,
Harald van Dijk
___
busybox mailing list
busybox@busybox.net
https://lists.busybox.net/mailman/listinfo/busybox