You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Stefan Bodewig <bo...@apache.org> on 2021/06/06 05:51:35 UTC
[compress] Dealing with RuntimeExceptions While Parsing Archives
Hi
I'm thinking about a specific IOException subclass that is thrown when a
RuntimeException "happens" somewhere in the code that parses data in
Zip/SevenZ/TarFile, see
https://github.com/apache/commons-compress/compare/catch-RuntimeExceptions
is this a good idea? Should anything be worded/named differently?
If this seems right I'd add similar code to all
Archive/CompressorInputStream classes as well.
Personally I would not do the same for the code that writes
archives/compresses streams as in this case the library user is fully
responsible for the input.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] Dealing with RuntimeExceptions While Parsing Archives
Posted by Gilles Sadowski <gi...@gmail.com>.
Hello.
Le lun. 7 juin 2021 à 06:51, Stefan Bodewig <bo...@apache.org> a écrit :
>
> On 2021-06-06, Gilles Sadowski wrote:
>
> > Le dim. 6 juin 2021 à 07:51, Stefan Bodewig <bo...@apache.org> a écrit :
>
> >> Hi
>
> >> I'm thinking about a specific IOException subclass that is thrown when a
> >> RuntimeException "happens" somewhere in the code that parses data in
> >> Zip/SevenZ/TarFile, see
>
> > I'm afraid I missed part of the story as to what is the original problem.
>
> Sorry, I should have expanded on that.
>
> When we uncompress a stream / expand an archive our users most of the
> time are not responsible for the input. If the data they hand over to
> Compress is invalid, they expect the library to throw an IOException -
> and in many cases this is true.
Does "Compress" make the promise that all exceptions are
(subclasses) of IOException?
> But the reality is most of our parsing code is written for the good case
> where the archive follows the spec. The code relies on numbers to be
> where they should be and not letters, it may fail to check an offset is
> inside of the bounds of an array and so on. So for certain types of
> broken archives the parsers will throw arbitrary RuntimeException
> (NumberFormat, ArrayIndexOutOfBounds, NegativeArraySize and so on).
>
> People do not expect said RuntimeExcepitons, so they don't catch them.
If the answer to the above question was "no", then it's their
responsibility to protect their code against RuntimeException.
> In a situation where an attacker controls the input this can be used to
> make the application reading it crash. So for certain types of
> applications this might be security relevant, it could be a DoS
> vector. Of course one can argue the calling code should better protect
> itself when it reads untrusted input and catch even undeclared
> exceptions,
I cannot see of how one could argue differently.
That runtime exceptions can occur is a Java fact.
> that's why we've never issued CVEs for exceptions where our
> parser code has not been strict enough in the past.
>
> After Compress 1.20 we've had lots of reports of RuntimeExceptions being
> thrown, many of which have been uncovered by fuzz testing tools (not
> only, but also OSS Fuzz).
>
> What I suggest is a stop-gap. It is not an excuse for not properly
> verifying input in our parsing code
I still don't think that "Compress" should double-check a condition
(e.g. array index, null, ...) that will be checked at some lower level
and whose worst consequence would be to throw an exception.
> but rather a way to limit the impact
> of such oversights.
If the answer to the above question is "yes", then AFAICT the
only fix is what you proposed.
Best regards,
Gilles
>
> Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] Dealing with RuntimeExceptions While Parsing Archives
Posted by Stefan Bodewig <bo...@apache.org>.
On 2021-06-06, Gilles Sadowski wrote:
> Le dim. 6 juin 2021 à 07:51, Stefan Bodewig <bo...@apache.org> a écrit :
>> Hi
>> I'm thinking about a specific IOException subclass that is thrown when a
>> RuntimeException "happens" somewhere in the code that parses data in
>> Zip/SevenZ/TarFile, see
> I'm afraid I missed part of the story as to what is the original problem.
Sorry, I should have expanded on that.
When we uncompress a stream / expand an archive our users most of the
time are not responsible for the input. If the data they hand over to
Compress is invalid, they expect the library to throw an IOException -
and in many cases this is true.
But the reality is most of our parsing code is written for the good case
where the archive follows the spec. The code relies on numbers to be
where they should be and not letters, it may fail to check an offset is
inside of the bounds of an array and so on. So for certain types of
broken archives the parsers will throw arbitrary RuntimeException
(NumberFormat, ArrayIndexOutOfBounds, NegativeArraySize and so on).
People do not expect said RuntimeExcepitons, so they don't catch them.
In a situation where an attacker controls the input this can be used to
make the application reading it crash. So for certain types of
applications this might be security relevant, it could be a DoS
vector. Of course one can argue the calling code should better protect
itself when it reads untrusted input and catch even undeclared
exceptions, that's why we've never issued CVEs for exceptions where our
parser code has not been strict enough in the past.
After Compress 1.20 we've had lots of reports of RuntimeExceptions being
thrown, many of which have been uncovered by fuzz testing tools (not
only, but also OSS Fuzz).
What I suggest is a stop-gap. It is not an excuse for not properly
verifying input in our parsing code but rather a way to limit the impact
of such oversights.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] Dealing with RuntimeExceptions While Parsing Archives
Posted by Matt Sicker <bo...@gmail.com>.
Ah, I see. These exceptions could derive from UncheckedIOException perhaps?
On Sun, 6 Jun 2021 at 15:56, Gilles Sadowski <gi...@gmail.com> wrote:
>
> Le dim. 6 juin 2021 à 22:32, Matt Sicker <bo...@gmail.com> a écrit :
> >
> > Well, if there's a parse error decompressing a file, that makes sense
> > as an IOException of some sort.
>
> The proposal is the change some unspecified RuntimeException
> into an unspecified IOException.
> My opinion is that a checked exception must restricted to signalling
> a problem that can be recovered from.
> The situation described here is that the library doesn't even know
> what the problem is.
>
> Gilles
>
> >
> > On Sun, 6 Jun 2021 at 12:01, Gilles Sadowski <gi...@gmail.com> wrote:
> > >
> > > Le dim. 6 juin 2021 à 07:51, Stefan Bodewig <bo...@apache.org> a écrit :
> > > >
> > > > Hi
> > > >
> > > > I'm thinking about a specific IOException subclass that is thrown when a
> > > > RuntimeException "happens" somewhere in the code that parses data in
> > > > Zip/SevenZ/TarFile, see
> > >
> > > I'm afraid I missed part of the story as to what is the original problem.
> > >
> > > >
> > > > https://github.com/apache/commons-compress/compare/catch-RuntimeExceptions
> > > >
> > > > is this a good idea?
> > >
> > > IMO, no.
> > > Why would one want to create a checked exception from
> > > an unchecked one?
> > >
> > > > Should anything be worded/named differently?
> > >
> > > IllegalStateException
> > >
> > > Gilles
> > >
> > > >
> > > > If this seems right I'd add similar code to all
> > > > Archive/CompressorInputStream classes as well.
> > > >
> > > > Personally I would not do the same for the code that writes
> > > > archives/compresses streams as in this case the library user is fully
> > > > responsible for the input.
> > > >
> > > > Stefan
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] Dealing with RuntimeExceptions While Parsing Archives
Posted by Gilles Sadowski <gi...@gmail.com>.
Le dim. 6 juin 2021 à 22:32, Matt Sicker <bo...@gmail.com> a écrit :
>
> Well, if there's a parse error decompressing a file, that makes sense
> as an IOException of some sort.
The proposal is the change some unspecified RuntimeException
into an unspecified IOException.
My opinion is that a checked exception must restricted to signalling
a problem that can be recovered from.
The situation described here is that the library doesn't even know
what the problem is.
Gilles
>
> On Sun, 6 Jun 2021 at 12:01, Gilles Sadowski <gi...@gmail.com> wrote:
> >
> > Le dim. 6 juin 2021 à 07:51, Stefan Bodewig <bo...@apache.org> a écrit :
> > >
> > > Hi
> > >
> > > I'm thinking about a specific IOException subclass that is thrown when a
> > > RuntimeException "happens" somewhere in the code that parses data in
> > > Zip/SevenZ/TarFile, see
> >
> > I'm afraid I missed part of the story as to what is the original problem.
> >
> > >
> > > https://github.com/apache/commons-compress/compare/catch-RuntimeExceptions
> > >
> > > is this a good idea?
> >
> > IMO, no.
> > Why would one want to create a checked exception from
> > an unchecked one?
> >
> > > Should anything be worded/named differently?
> >
> > IllegalStateException
> >
> > Gilles
> >
> > >
> > > If this seems right I'd add similar code to all
> > > Archive/CompressorInputStream classes as well.
> > >
> > > Personally I would not do the same for the code that writes
> > > archives/compresses streams as in this case the library user is fully
> > > responsible for the input.
> > >
> > > Stefan
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] Dealing with RuntimeExceptions While Parsing Archives
Posted by Matt Sicker <bo...@gmail.com>.
Well, if there's a parse error decompressing a file, that makes sense
as an IOException of some sort.
On Sun, 6 Jun 2021 at 12:01, Gilles Sadowski <gi...@gmail.com> wrote:
>
> Le dim. 6 juin 2021 à 07:51, Stefan Bodewig <bo...@apache.org> a écrit :
> >
> > Hi
> >
> > I'm thinking about a specific IOException subclass that is thrown when a
> > RuntimeException "happens" somewhere in the code that parses data in
> > Zip/SevenZ/TarFile, see
>
> I'm afraid I missed part of the story as to what is the original problem.
>
> >
> > https://github.com/apache/commons-compress/compare/catch-RuntimeExceptions
> >
> > is this a good idea?
>
> IMO, no.
> Why would one want to create a checked exception from
> an unchecked one?
>
> > Should anything be worded/named differently?
>
> IllegalStateException
>
> Gilles
>
> >
> > If this seems right I'd add similar code to all
> > Archive/CompressorInputStream classes as well.
> >
> > Personally I would not do the same for the code that writes
> > archives/compresses streams as in this case the library user is fully
> > responsible for the input.
> >
> > Stefan
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] Dealing with RuntimeExceptions While Parsing Archives
Posted by Gilles Sadowski <gi...@gmail.com>.
Le dim. 6 juin 2021 à 07:51, Stefan Bodewig <bo...@apache.org> a écrit :
>
> Hi
>
> I'm thinking about a specific IOException subclass that is thrown when a
> RuntimeException "happens" somewhere in the code that parses data in
> Zip/SevenZ/TarFile, see
I'm afraid I missed part of the story as to what is the original problem.
>
> https://github.com/apache/commons-compress/compare/catch-RuntimeExceptions
>
> is this a good idea?
IMO, no.
Why would one want to create a checked exception from
an unchecked one?
> Should anything be worded/named differently?
IllegalStateException
Gilles
>
> If this seems right I'd add similar code to all
> Archive/CompressorInputStream classes as well.
>
> Personally I would not do the same for the code that writes
> archives/compresses streams as in this case the library user is fully
> responsible for the input.
>
> Stefan
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org