Permalink
Browse files

Avoid overflow in ljpeg_start().

  • Loading branch information...
abrander committed May 11, 2015
1 parent 6eabf1f commit 983bda1f0fa5fa86884381208274198a620f006e
Showing with 2 additions and 1 deletion.
  1. +2 −1 plugins/load-dcraw/dcraw.cc
@@ -890,7 +890,8 @@ struct jhead {
int CLASS ljpeg_start (struct jhead *jh, int info_only)
{
int c, tag, len;
int c, tag;
ushort len;
uchar data[0x10000];
const uchar *dp;

1 comment on commit 983bda1

@abrander

This comment has been minimized.

Show comment
Hide comment
@abrander

abrander Jul 1, 2015

Member

The patch was the one received from David Coffin (author of dcraw) off channel before making the vulnerability public if I remember correctly.

I think the len < 2 check is related to the line

len = (data[2] << 8 | data[3]) - 2;

where len can be USHORT-MAX-1, which may result in a possible read error in the following fread() call - but you can easily trigger a read error even without underflowing. I don't see any security related bugs triggered by omitting the check, len is an unsigned integer, and the data array is of size USHORT_MAX+1. Neither overflow nor underflow can occur.

The fix in netpbm and Fedora is somewhat cleaner than the upstream fix in dcraw, but all are good from a security perspective.

For the interested: Rawstudio plans to remove dcraw completely from a future version and rely solely on RawSpeed - and maybe calling an external dcraw binary as a fall-back option to support ancient file formats.

Member

abrander commented on 983bda1 Jul 1, 2015

The patch was the one received from David Coffin (author of dcraw) off channel before making the vulnerability public if I remember correctly.

I think the len < 2 check is related to the line

len = (data[2] << 8 | data[3]) - 2;

where len can be USHORT-MAX-1, which may result in a possible read error in the following fread() call - but you can easily trigger a read error even without underflowing. I don't see any security related bugs triggered by omitting the check, len is an unsigned integer, and the data array is of size USHORT_MAX+1. Neither overflow nor underflow can occur.

The fix in netpbm and Fedora is somewhat cleaner than the upstream fix in dcraw, but all are good from a security perspective.

For the interested: Rawstudio plans to remove dcraw completely from a future version and rely solely on RawSpeed - and maybe calling an external dcraw binary as a fall-back option to support ancient file formats.

Please sign in to comment.