Skip to content

Fix buffer overflow in ppdLoadAttributes(). #51

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dcoppa wants to merge 1 commit into from
+1 −1
Closed

Fix buffer overflow in ppdLoadAttributes(). #51

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ppd/ppd-ipp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ ppdLoadAttributes(
while (*ptr && isspace(*ptr)) ptr ++;
if (!isdigit(*ptr))
{
strcpy(buf, ptr);
snprintf(buf, sizeof(buf), "%s", ptr);
ptr = buf;
Copy link
Member

@tillkamppeter tillkamppeter Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that, at least for the string in buf comming from a correct *cupsFilter(2): ... line, the crash cannot have been caused by the string pointed at by ptr being too long for buf, as ptr is pointing somewhere into buf. The problem is that, according to the man page, strcpy() is not suitable for copying overlapping strings/buffers, and so that could have caused the crash here.

Now, with the replacement by snprintf() the crash has gone away in this particular case, but here the man page also says that this function is not suitable for overlapping strings/buffers, so one should expect that a crash still can happen in other situations.

To be safe one could eaither do a for() loop moving the characters to the beginning of the buffer one-by-one or the memmove() function which according to its man page supports overlapping memory areas.

memmove(buf, ptr, strnlen(ptr, sizeof(buf) - 1) + 1);

could be used here and should work reliably, at least if buf contains a correctly \0-terminated string.

Copy link
Member

@michaelrsweet michaelrsweet Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strlen(ptr) + 1 (to also move the terminating nul character)

while (*ptr && !isspace(*ptr)) ptr ++;
if (*ptr)
Expand Down