You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by "Greg Squires (JIRA)" <ji...@apache.org> on 2009/02/09 19:54:59 UTC
[jira] Created: (SHINDIG-906) BasicImageRewriter bad memory
allocation arguments
BasicImageRewriter bad memory allocation arguments
--------------------------------------------------
Key: SHINDIG-906
URL: https://issues.apache.org/jira/browse/SHINDIG-906
Project: Shindig
Issue Type: Bug
Components: Gadget Rendering Server (Java)
Affects Versions: trunk
Environment: Win32, 32bit
Reporter: Greg Squires
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.
Re: [jira] Commented: (SHINDIG-906) BasicImageRewriter bad memory
allocation arguments
Posted by Louis Ryan <lr...@google.com>.
Although far from perfect using Sanselan.getICCProfile does actually do what
we want. i.e check if the profile bytes exists, read the profile and then
fail with an exception if no profile can be read. Its not exactly elegant
however as it fails with a NullPointerException in Sanselan.java line 323.
Im going to duplicate this code in our logic to protect ourselves from
changes in Sanselan however.
On Mon, Mar 9, 2009 at 9:33 AM, Louis Ryan <lr...@google.com> wrote:
> 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) <ji...@apache.org>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.
>>
>>
>
Re: [jira] Commented: (SHINDIG-906) BasicImageRewriter bad memory
allocation arguments
Posted by Louis Ryan <lr...@google.com>.
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) <ji...@apache.org> 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.
>
>
[jira] Commented: (SHINDIG-906) BasicImageRewriter bad memory
allocation arguments
Posted by "Louis Ryan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-906?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12679472#action_12679472 ]
Louis Ryan commented on SHINDIG-906:
------------------------------------
Dont switch to getICCProfileBytes as that provides no protection. We definitely want to protect against these kinds of overflow and having having this code throw an error if the expected buffer length is negative due to overflow on 32bit systems is definitely a good thing. We are recovering from this exception and simply not rewriting the image.
@Greg
Im aware of the issue where getICCProfile can cause an OutOfMemoryError through excessively large byte[] allocations (both on 32 bit and 64 bit systems) but this is far preferable to a buffer overrun occurring when we attempt to use ImageIO to read the JPEG as that occurs in native code and has security vulnerabilities. OutOfMemory errors can occaisionally be recovered from. Greg do you have a specifc example image that is causing you headaches that is not included in the tests.
> 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.
[jira] Commented: (SHINDIG-906) BasicImageRewriter bad memory
allocation arguments
Posted by "Greg Squires (JIRA)" <ji...@apache.org>.
[ 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.
[jira] Updated: (SHINDIG-906) BasicImageRewriter bad memory
allocation arguments
Posted by "Louis Ryan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-906?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Louis Ryan updated SHINDIG-906:
-------------------------------
Attachment: (was: SHINDIG-906.patch)
> 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.
[jira] Updated: (SHINDIG-906) BasicImageRewriter bad memory
allocation arguments
Posted by "Vincent Siveton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-906?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Vincent Siveton updated SHINDIG-906:
------------------------------------
Attachment: SHINDIG-906.patch
I propose to use Sanselan.getICCProfileBytes() instead of Sanselan.getICCProfile()
> 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
> Attachments: SHINDIG-906.patch
>
> 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.