Greg Thanks for drilling into this so thoroughly. The code you post below from Sanselan is certainly troubling clearly makes the existing unit-tests useless. One valid short term workaround is to read the ICC profile bytes and then attempt to read the profile. If we bytes but the profile is null we can assume corruption, this would be better than where we are today so Im going to do that now.
As for safe-image handling in general I posted on the Sanselan list a while back suggesting that they include this in the goals for their project as Im sure its something that would give them more traction. I am planning at some stage to offer patches for this but it wont be until after 0.9 is out. If you felt like getting involved that would be a great place to contribute. -Louis On Fri, Mar 6, 2009 at 6:40 PM, Greg Squires (JIRA) <[email protected]> wrote: > > [ > https://issues.apache.org/jira/browse/SHINDIG-906?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12679796#action_12679796] > > Greg Squires commented on SHINDIG-906: > -------------------------------------- > > For the example, unfortunately not. I apologise for not referring to the > exact shindig revision number. > > It occurred around rev 741361. I have noticed that more recent revisions > have extended the ImageRewiter tests to handle bad ICC profile data, > including negative or very large allocation requests. My first concern here > was that the exception specification in Sanselan may be a little loose, in > that an incorrect positive value exceeding the byte allocation is considered > an IOException, while a negative length is considered by default to be a > NegativeArraySizeException, which is of a type not in the function > specifications, which declare exception types of ImageReadError and > IOException. For the benefit of exception handlers, it would be better to be > consistent and test for negative numbers with say: > > if (length < 0 || ( start + length > bytes.length) ){ throw new IOException > ...} instead I would have thought., though as these numbers are sourced from > a valid io read, then strictly I would have thought it to be an exception of > type ImageReadError. ( See BasicImageRewriter.java line 137 ) > > More importantly though, Sanselan returns a null IccProfileInfo object ( > IccProfileParser.java ) on any exception event, and never throws either an > IoException or ImageReadError through any ICC profile parsing, if I am > correct. > > ie: > > public IccProfileInfo getICCProfileInfo(ByteSource byteSource) > { > > InputStream is = null; > > try > { > > IccProfileInfo result; > { > is = byteSource.getInputStream(); > > result = readICCProfileInfo(is); > } > > if (result == null) > return null; > > is.close(); > is = null; > > for (int i = 0; i < result.tags.length; i++) > { > IccTag tag = result.tags[i]; > byte bytes[] = > byteSource.getBlock(tag.offset, tag.length); > // > Debug.debug("bytes: " + bytes.length); > tag.setData(bytes); > // > tag.dump("\t" + i + ": "); > } > // > result.fillInTagData(byteSource); > > return result; > } > catch (Exception e) > { > // Debug.debug("Error: " + > file.getAbsolutePath()); > Debug.debug(e); > } > finally > { > try > { > if (is != null) > is.close(); > } > catch (Exception e) > { > Debug.debug(e); > } > > } > > if (debug) > Debug.debug(); > > return null; > } > > In other words, cases of ordinary or extreme ICC profile data corruption > are treated the same as having no ICC profile data at all. I would have > thought that this should be a notifiable exception of type ImageReadError, > for the benefit of shindig. > > Also, as you mention Louis, what first brought it to my attention was that > the large number exceeded 31 bits, and could lead to massive memory > allocations under these conditions, especially where a crafted image > contained many tags. No limitation checks are performed. Without referring > to the ICC specifications, I'm not sure how much limit checking can be done. > > Therefore, my concern for shindig is of both security and resource attacks, > together with perhaps a lack of Exception strictness for the purposes of > Exception handling in the ImageRewriter classes. > > > BasicImageRewriter bad memory allocation arguments > > -------------------------------------------------- > > > > Key: SHINDIG-906 > > URL: https://issues.apache.org/jira/browse/SHINDIG-906 > > Project: Shindig > > Issue Type: Bug > > Components: Java > > Affects Versions: trunk > > Environment: Win32, 32bit > > Reporter: Greg Squires > > Original Estimate: 48h > > Remaining Estimate: 48h > > > > The Basic ImageRewriter relies on Sanselan.getICCProfile, which has > limited bounds checking. Other metadata functions are also affected. > > This function can throw an Exception in ByteSourceArray.java due to a > negative byte[] allocation size. The length argument has been found to wrap > when called from IccProfileParser.java. > > In 64bit machines, issues related to incorrect metadata, or ICC data can > lead to incorrect and excess memory allocations, which often fail. These > large numbers however modulo on 32bit and result in negative signed values. > > The shindig test JPEGOptimizerTest behaves differently on 64 bit and 32 > bit platforms. > > Line 45 ByteSourceArray.java: > > public byte[] getBlock(int start, int length) throws IOException > > { > > if (start + length > bytes.length) > > throw new IOException("Could not read block (block > start: " + start > > + ", block length: " + length + ", > data length: " > > + bytes.length + ")."); > > byte result[] = new byte[length]; > > System.arraycopy(bytes, start, result, 0, length); > > return result; > > } > > -- > This message is automatically generated by JIRA. > - > You can reply to this email to add a comment to the issue online. > >
