🔒 Replace unsafe sprintf with snprintf in tphrase.c#86
Conversation
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. Co-authored-by: manupawickramasinghe <73810867+manupawickramasinghe@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
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> Co-authored-by: manupawickramasinghe <73810867+manupawickramasinghe@users.noreply.github.com>
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> Co-authored-by: manupawickramasinghe <73810867+manupawickramasinghe@users.noreply.github.com>
…220700 Signed-off-by: M.N Wickramasinghe <95234271+ManupaKDU@users.noreply.github.com>
1 participant
🎯 What: The
⚠️ Risk: While the buffer allocation (
tphrase.cfile intinysrpused the unsafesprintffunction to construct backup file names (.bakand.sav).strlen(pwname) + 5) is mathematically exact for the given format string,sprintfhas no bounds checking. Any future changes to the format string or password length logic without corresponding adjustments to the allocation logic could lead to a buffer overflow, potentially allowing memory corruption.🛡️ Solution: Replaced
sprintfwithsnprintfand used the pre-calculated allocation size (alloc_size) as the bounds limit. This ensures the write operation can never exceed the allocated buffer size.PR created automatically by Jules for task 17484921919210220700 started by @manupawickramasinghe