Skip to content

Commit

Permalink
Fix potential BMP stack overflow (Issue #453)
Browse files Browse the repository at this point in the history
  • Loading branch information
@michaelrsweet
michaelrsweet committed Nov 5, 2021
1 parent 86d1543 commit 27d0898
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
- Fixed a crash bug when a HTML comment contains an invalid nul character
(Issue #439)
- Fixed a crash bug with bogus BMP images (Issue #444)
- Fixed a stack overflow bug with bogus BMP images (Issue #453)


# Changes in HTMLDOC v1.9.12
Expand Down
4 changes: 4 additions & 0 deletions htmldoc/image.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -919,12 +919,16 @@ image_load_bmp(image_t *img, /* I - Image to load into */
return (-1);

if (info_size > 40)
{
for (info_size -= 40; info_size > 0; info_size --)
getc(fp);
}

// Get colormap...
if (colors_used == 0 && depth <= 8)
colors_used = 1 << depth;
else if (colors_used > 256)
return (-1);

fread(colormap, (size_t)colors_used, 4, fp);

Expand Down

5 comments on commit 27d0898

@setharnold
Copy link

Choose a reason for hiding this comment

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

Are there consequences if depth is negative? How about info_size? etc.

Thanks

@michaelrsweet
Copy link
Owner Author

Choose a reason for hiding this comment

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

@setharnold info_size and depth are both unsigned values in the file. In the case of info_size, the consequence of a 32-bit unsigned value being interpreted as a 32-bit signed value (when int is 32-bits) will be that the extra image information will not be skipped and you'll end up with bogus image data embedded in the output. For depth I only support specific values (1, 4, 8, and 24) - an unsupported value will result in a blank image. I should probably change this to return an error but it isn't critical.

@setharnold
Copy link

@setharnold setharnold commented on 27d0898 Nov 25, 2021 via email

Choose a reason for hiding this comment

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

@michaelrsweet
Copy link
Owner Author

Choose a reason for hiding this comment

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

@setharnold The bogus data comes from the file - this doesn't cause any weird memory accesses, it will just mean that the reader will look for colormap and image data inside the extra image information area and populate the pixel array accordingly. More than likely the output will contain a garbage rectangle but it will be safe garbage with no out-of-bounds memory accesses.

@setharnold
Copy link

Choose a reason for hiding this comment

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

@michaelrsweet beautiful, thanks! :)

Please sign in to comment.