Skip to content

Use size_t where appropriate and guard against overflow #1384

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
wants to merge 2 commits into from
Closed

Use size_t where appropriate and guard against overflow #1384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f042297
Use size_t where appropriate and guard against overflow
LinusU Mar 15, 2019
b010794
Guard JPEG width/height against maximum supported
LinusU Mar 16, 2019
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ project adheres to [Semantic Versioning](http://semver.org/).
### Changed
### Added
### Fixed
* Use size_t where appropriate and guard against overflow
* Fix electron 5 and node 12 compatibility
* Fix encoding options (quality) parameter in `canvas.toDataURL()`

Expand Down
77 changes: 52 additions & 25 deletions src/Image.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
#include <node_buffer.h>
#include "Util.h"

/* Cairo limit:
* https://lists.cairographics.org/archives/cairo/2010-December/021422.html
*/
static constexpr const size_t canvas_max_side = (1 << 15) - 1;

#ifdef HAVE_GIF
typedef struct {
uint8_t *buf;
Expand Down Expand Up @@ -252,7 +257,7 @@ NAN_METHOD(Image::SetSource){
argv[0] = Nan::ErrnoException(errorInfo.cerrno, errorInfo.syscall.c_str(), errorInfo.message.c_str(), errorInfo.path.c_str());
} else if (!errorInfo.message.empty()) {
argv[0] = Nan::Error(Nan::New(errorInfo.message).ToLocalChecked());
} else {
} else {
argv[0] = Nan::Error(Nan::New(cairo_status_to_string(status)).ToLocalChecked());
}
onerrorFn.As<Function>()->Call(Isolate::GetCurrent()->GetCurrentContext()->Global(), 1, argv);
Expand Down Expand Up @@ -453,7 +458,7 @@ Image::loadSurface() {
return loadPNG();
}


if (isGIF(buf)) {
#ifdef HAVE_GIF
return loadGIF(stream);
Expand Down Expand Up @@ -607,10 +612,7 @@ Image::loadGIFFromBuffer(uint8_t *buf, unsigned len) {
width = naturalWidth = gif->SWidth;
height = naturalHeight = gif->SHeight;

/* Cairo limit:
* https://lists.cairographics.org/archives/cairo/2010-December/021422.html
*/
if (width > 32767 || height > 32767) {
if (width > canvas_max_side || height > canvas_max_side) {
GIF_CLOSE_FILE(gif);
return CAIRO_STATUS_INVALID_SIZE;
}
Expand Down Expand Up @@ -643,9 +645,9 @@ Image::loadGIFFromBuffer(uint8_t *buf, unsigned len) {
uint32_t *dst_data = (uint32_t*) data;

if (!gif->Image.Interlace) {
if (naturalWidth == img->Width && naturalHeight == img->Height) {
for (int y = 0; y < naturalHeight; ++y) {
for (int x = 0; x < naturalWidth; ++x) {
if (naturalWidth == static_cast<size_t>(img->Width) && naturalHeight == static_cast<size_t>(img->Height)) {
for (size_t y = 0; y < naturalHeight; ++y) {
for (size_t x = 0; x < naturalWidth; ++x) {
*dst_data = ((*src_data == alphaColor) ? 0 : 255) << 24
| colormap->Colors[*src_data].Red << 16
| colormap->Colors[*src_data].Green << 8
Expand All @@ -657,18 +659,18 @@ Image::loadGIFFromBuffer(uint8_t *buf, unsigned len) {
}
} else {
// Image does not take up whole "screen" so we need to fill-in the background
int bottom = img->Top + img->Height;
int right = img->Left + img->Width;
size_t bottom = static_cast<size_t>(img->Top) + static_cast<size_t>(img->Height);
size_t right = static_cast<size_t>(img->Left) + static_cast<size_t>(img->Width);

uint32_t bgPixel =
((bgColor == alphaColor) ? 0 : 255) << 24
| colormap->Colors[bgColor].Red << 16
| colormap->Colors[bgColor].Green << 8
| colormap->Colors[bgColor].Blue;

for (int y = 0; y < naturalHeight; ++y) {
for (int x = 0; x < naturalWidth; ++x) {
if (y < img->Top || y >= bottom || x < img->Left || x >= right) {
for (size_t y = 0; y < naturalHeight; ++y) {
for (size_t x = 0; x < naturalWidth; ++x) {
if (y < static_cast<size_t>(img->Top) || y >= bottom || x < static_cast<size_t>(img->Left) || x >= right) {
*dst_data = bgPixel;
dst_data++;
} else {
Expand All @@ -686,16 +688,16 @@ Image::loadGIFFromBuffer(uint8_t *buf, unsigned len) {
// Image is interlaced so that it streams nice over 14.4k and 28.8k modems :)
// We first load in 1/8 of the image, followed by another 1/8, followed by
// 1/4 and finally the remaining 1/2.
int ioffs[] = { 0, 4, 2, 1 };
int ijumps[] = { 8, 8, 4, 2 };
size_t ioffs[] = { 0, 4, 2, 1 };
size_t ijumps[] = { 8, 8, 4, 2 };

uint8_t *src_ptr = src_data;
uint32_t *dst_ptr;

for(int z = 0; z < 4; z++) {
for(int y = ioffs[z]; y < naturalHeight; y += ijumps[z]) {
for(size_t z = 0; z < 4; z++) {
for(size_t y = ioffs[z]; y < naturalHeight; y += ijumps[z]) {
dst_ptr = dst_data + naturalWidth * y;
for(int x = 0; x < naturalWidth; ++x) {
for(size_t x = 0; x < naturalWidth; ++x) {
*dst_ptr = ((*src_ptr == alphaColor) ? 0 : 255) << 24
| (colormap->Colors[*src_ptr].Red) << 16
| (colormap->Colors[*src_ptr].Green) << 8
Expand Down Expand Up @@ -780,12 +782,13 @@ static void jpeg_mem_src (j_decompress_ptr cinfo, void* buffer, long nbytes) {
#endif

void Image::jpegToARGB(jpeg_decompress_struct* args, uint8_t* data, uint8_t* src, JPEGDecodeL decode) {
int stride = naturalWidth * 4;
for (int y = 0; y < naturalHeight; ++y) {
size_t stride = naturalWidth * 4;

for (size_t y = 0; y < naturalHeight; ++y) {
jpeg_read_scanlines(args, &src, 1);
uint32_t *row = (uint32_t*)(data + stride * y);
for (int x = 0; x < naturalWidth; ++x) {
int bx = args->output_components * x;
for (size_t x = 0; x < naturalWidth; ++x) {
size_t bx = args->output_components * x;
row[x] = decode(src + bx);
}
}
Expand All @@ -799,8 +802,26 @@ void Image::jpegToARGB(jpeg_decompress_struct* args, uint8_t* data, uint8_t* src
cairo_status_t
Image::decodeJPEGIntoSurface(jpeg_decompress_struct *args) {
cairo_status_t status = CAIRO_STATUS_SUCCESS;

uint8_t *data = new uint8_t[naturalWidth * naturalHeight * 4];

size_t stride = naturalWidth * 4;
if (naturalWidth != 0 && stride / naturalWidth != 4) {
Copy link
Collaborator

@zbjornson zbjornson Mar 15, 2019

Choose a reason for hiding this comment

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

We've got a host of these unchecked muls... might be worth copying a version of this function so it's easier to continue addressing them later.

https://github.com/nodejs/node/blob/master/src/util-inl.h#L297-304

(not sure if that link works, github is acting up. Line 297-304.)

// The integer overflowed, which means we cannot allocate that much memory.
jpeg_abort_decompress(args);
jpeg_destroy_decompress(args);
this->errorInfo.set(NULL, "malloc", errno);
return CAIRO_STATUS_NO_MEMORY;
}

size_t size = naturalHeight * stride;
if (naturalHeight != 0 && size / naturalHeight != stride) {
// The integer overflowed, which means we cannot allocate that much memory.
jpeg_abort_decompress(args);
jpeg_destroy_decompress(args);
this->errorInfo.set(NULL, "malloc", errno);
return CAIRO_STATUS_NO_MEMORY;
}
Copy link
Collaborator

@zbjornson zbjornson Mar 15, 2019

Choose a reason for hiding this comment

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

You could consider combining these two if bodies into one since you don't have to check overflow immediately after doing the mul.


uint8_t *data = new uint8_t[size];
if (!data) {
jpeg_abort_decompress(args);
jpeg_destroy_decompress(args);
Expand Down Expand Up @@ -1078,6 +1099,12 @@ Image::loadJPEG(FILE *stream) {

jpeg_read_header(&args, 1);
jpeg_start_decompress(&args);

if (args.output_width > canvas_max_side || args.output_height > canvas_max_side) {
jpeg_destroy_decompress(&args);
return CAIRO_STATUS_INVALID_SIZE;
}

width = naturalWidth = args.output_width;
height = naturalHeight = args.output_height;

Expand Down
8 changes: 4 additions & 4 deletions src/Image.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ using JPEGDecodeL = std::function<uint32_t (uint8_t* const src)>;
class Image: public Nan::ObjectWrap {
public:
char *filename;
int width, height;
int naturalWidth, naturalHeight;
size_t width, height;
size_t naturalWidth, naturalHeight;
Copy link
Collaborator

@zbjornson zbjornson Mar 15, 2019

Choose a reason for hiding this comment

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

I'm trying to find any cases where int isn't big enough. The only one I found is reading a jpeg, where it's unsigned. GifLib, Cairo and librsvg all use int as far as I can tell. Was this type itself responsible for overflow?

One concern making this size_t is that width and height are passed directly to e.g. cairo_image_surface_create(format, int width, int height).

Copy link
Collaborator Author

@LinusU LinusU Mar 16, 2019

Choose a reason for hiding this comment

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

If the image is 32798x32798 it will overflow and allocate way to little bytes since:

Screenshot 2019-03-16 at 10 12 57

Good point about passing to cairo, I think that we already check for some specific value since cairo only accepts a maxiumum width or height of 2^15 - 1 (I think, ref:

node-canvas/src/Image.cc

Lines 610 to 613 in 3d81e0a

/* Cairo limit:
* https://lists.cairographics.org/archives/cairo/2010-December/021422.html
*/
if (width > 32767 || height > 32767) {
)

Okay, I just realized that the image I'm loading has a larger width & height than that, maybe we should just check that before even setting naturalWidht and naturalHeight at all 🤔

Copy link
Collaborator

@zbjornson zbjornson Mar 16, 2019

Choose a reason for hiding this comment

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

I meant to propose: keep the members as int (or unsigned) since that's the largest Cairo can take, and only cast to a larger type where you multiply them and risk overflow.

static Nan::Persistent<v8::FunctionTemplate> constructor;
static void Initialize(Nan::ADDON_REGISTER_FUNCTION_ARGS_TYPE target);
static NAN_METHOD(New);
Expand Down Expand Up @@ -120,8 +120,8 @@ class Image: public Nan::ObjectWrap {
#ifdef HAVE_RSVG
RsvgHandle *_rsvg;
bool _is_svg;
int _svg_last_width;
int _svg_last_height;
size_t _svg_last_width;
size_t _svg_last_height;
#endif
~Image();
};