[
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.
