Skip to content

Commit

Permalink
stop JPEG load after 20 warnings
Browse files Browse the repository at this point in the history
mitigates some DoS attacks somewhat

see https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24383
  • Loading branch information
jcupitt committed Apr 3, 2022
1 parent 55b857d commit 89bd46d
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 9 deletions.
6 changes: 6 additions & 0 deletions libvips/foreign/jpeg2vips.c
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,12 @@ read_jpeg_header( ReadJpeg *jpeg, VipsImage *out )
size_t data_length;
int i;

/* Trace level 3 means emit warning messages as they happen. This
* lets us spot files with crazy numbers of warnings early and
* prevents some DoS attacks.
*/
jpeg->eman.pub.trace_level = 3;

/* Read JPEG header. libjpeg will set out_color_space sanely for us
* for YUV YCCK etc.
*/
Expand Down
32 changes: 23 additions & 9 deletions libvips/foreign/vips2jpeg.c
Original file line number Diff line number Diff line change
Expand Up @@ -165,20 +165,34 @@
void
vips__new_output_message( j_common_ptr cinfo )
{
char buffer[JMSG_LENGTH_MAX];
/* Some DoS attacks use jpg files with thousands of warnings. Try to
* limit the effect these have.
*/
if( cinfo->err->num_warnings >= 20 ) {
if( cinfo->err->num_warnings == 20 ) {
vips_error( "VipsJpeg",
"%s", _( "too many warnings" ) );
}

jpeg_abort( cinfo );
}
else {
char buffer[JMSG_LENGTH_MAX];

(*cinfo->err->format_message)( cinfo, buffer );
vips_error( "VipsJpeg", _( "%s" ), buffer );
(*cinfo->err->format_message)( cinfo, buffer );
vips_error( "VipsJpeg", _( "%s" ), buffer );

#ifdef DEBUG
printf( "vips__new_output_message: \"%s\"\n", buffer );
printf( "vips__new_output_message: \"%s\"\n", buffer );
#endif /*DEBUG*/

/* This is run for things like file truncated. Signal invalidate to
* force this op out of cache.
*/
if( cinfo->client_data )
vips_foreign_load_invalidate( VIPS_IMAGE( cinfo->client_data ) );
/* This is run for things like file truncated. Signal
* invalidate to force this op out of cache.
*/
if( cinfo->client_data )
vips_foreign_load_invalidate(
VIPS_IMAGE( cinfo->client_data ) );
}
}

/* New error_exit handler.
Expand Down

6 comments on commit 89bd46d

@lovell
Copy link
Member

@lovell lovell commented on 89bd46d Apr 4, 2022

Choose a reason for hiding this comment

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

@jcupitt Did you see the new use-after-free fuzz test bugs relating to jpegload since this commit? Could this change result in multiple calls to jpeg_abort?

@jcupitt
Copy link
Member Author

@jcupitt jcupitt commented on 89bd46d Apr 4, 2022

Choose a reason for hiding this comment

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

Argh! Oh dear, I'll add something to make it only abort once. Thanks for the heads-up.

@jcupitt
Copy link
Member Author

@jcupitt jcupitt commented on 89bd46d Apr 4, 2022

Choose a reason for hiding this comment

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

OK, hopefully this is better: ae7e5e7

You have to (eeeek!) longjmp() out.

@jcupitt
Copy link
Member Author

@jcupitt jcupitt commented on 89bd46d Apr 5, 2022

Choose a reason for hiding this comment

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

Ugh this is awful, I've broken everything. I've reverted these changes while we think again.

@lovell
Copy link
Member

@lovell lovell commented on 89bd46d Apr 5, 2022

Choose a reason for hiding this comment

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

I've added a note to all of the OSS Fuzz issues relating to this that were labelled as Bug-Security.

@jcupitt
Copy link
Member Author

@jcupitt jcupitt commented on 89bd46d Apr 5, 2022

Choose a reason for hiding this comment

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

Thank you Lovell, sorry for the mess.

Please sign in to comment.