Skip to content
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.

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

CVE-2024-28219: Use strncpy to avoid buffer overflow #7928

Merged
merged 3 commits into from
Apr 1, 2024
Merged

CVE-2024-28219: Use strncpy to avoid buffer overflow #7928

merged 3 commits into from
Apr 1, 2024

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Apr 1, 2024

A straightforward way to stop strcpy from overflowing is to use strncpy.

The test image is from https://github.com/saucecontrol/Compact-ICC-Profiles/blob/master/profiles/sGrey-v2-nano.icc, licensed under CC0.

@hugovk hugovk added the automerge Automatically merge PRs that are ready label Apr 1, 2024
@hugovk
Copy link
Member Author

hugovk commented Apr 1, 2024

Merging; we don't need to wait for valgrind or AppVeyor to finish testing the latest commit which was docs only.

@hugovk hugovk merged commit 2776126 into main Apr 1, 2024
108 of 109 checks passed
@hugovk
Copy link
Member Author

hugovk commented Apr 1, 2024

I'll update CHANGES.rst with this when bumping the version.

@radarhere radarhere deleted the lcms branch April 1, 2024 09:21
@hugovk
Copy link
Member Author

hugovk commented Apr 1, 2024

Or you already did, no problem :)

Comment on lines +204 to +205
strncpy(self->mode_in, mode_in, 8);
strncpy(self->mode_out, mode_out, 8);

Choose a reason for hiding this comment

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

strncpy doesn’t null-terminate oversized entries.

Suggested change
strncpy(self->mode_in, mode_in, 8);
strncpy(self->mode_out, mode_out, 8);
strncpy(self->mode_in, mode_in, 8);
self->mode_in[7] = '\0';
strncpy(self->mode_out, mode_out, 8);
self->mode_out[7] = '\0';

But are these even used? The Python ImageCmsTransform wrapper has inputMode and outputMode attributes already, and they work normally. Is there a need to preserve a slightly different .transform.inputMode?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not, see #7931 for a suggestion to remove them.

@ThiefMaster
Copy link

Is this exploitable when all you do is opening user-provided images and converting them? Or only when you explicitly used the ImageCms module?

@radarhere
Copy link
Member

Something outside of Pillow would have to use ImageCms, yes.

@ArielPrevu3D
Copy link

ArielPrevu3D commented Apr 10, 2024

It looks like Image.convert can lead to a buffer overflow if the mode parameter is user-supplied. Your code does not have to use ImageCms directly to be exploitable.

profiles[0], profiles[1], self.mode, mode

@radarhere
Copy link
Member

If you look a few lines earlier

Pillow/src/PIL/Image.py

Lines 1070 to 1079 in 955c5da

if "LAB" in (self.mode, mode):
other_mode = mode if self.mode == "LAB" else self.mode
if other_mode in ("RGB", "RGBA", "RGBX"):
from . import ImageCms
srgb = ImageCms.createProfile("sRGB")
lab = ImageCms.createProfile("LAB")
profiles = [lab, srgb] if self.mode == "LAB" else [srgb, lab]
transform = ImageCms.buildTransform(
profiles[0], profiles[1], self.mode, mode

you will see that one of the modes has to be "LAB", and the other has to be one of "RGB", "RGBA" or "RGBX",
none of which is more than 8 characters.

So it isn't exploitable from Image.convert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PRs that are ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants