[PATCH FOR REVIEW] Potential Buffer Overflow in java_props_md.c

[Andrew Hughes]

----- Original Message -----
> On 01/08/2012 12:40, Andrew Hughes wrote:
> > java_props_md.c allocates a 64 byte buffer for the return value of
> > setlocale
> > on the stack.  However, there appears to be no set limit on the
> > return value:
> >
> > http://pubs.opengroup.org/onlinepubs/009604499/functions/setlocale.html
> >
> > and no check in the code to ensure that its length is 63 characters
> > or less
> > (allowing for '\0').  While language and country are defined by
> > ISO, I don't
> > believe there's any limit on the optional encoding and variant
> > entries.
> >
> > This patch:
> >
> > http://cr.openjdk.java.net/~andrew/buffer_overflow/webrev.01/jdk.patch
> >
> > replaces the allocation with a dynamic buffer based on the length
> > of lc.
> >
> > Original bug:
> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=497666
> >
> > Ok for tl?  If so, can I have a bug ID?
> >
> Just some history on this one.
> 
> Andrew Haley originally brought this up up in 2009 (embarrassing, I
> know), see:
> 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-May/001650.html
> 
> Then in 2011, Omair tried again, see:
> 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-May/006907.html
> 

Well, it's a Red Hat patch and we're all Red Hat employees :-) The original
version, posted by Andrew Haley and in IcedTea, was actually written by Lillian
Angel back in 2008:

2008-09-10  Lillian Angel  <langel at redhat.com>

	* patches/icedtea-lc_ctype.patch: New patch to fix this issue:
        http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=497666
	* Makefile.am: Added patch to list.

2008-09-15  Lillian Angel  <langel at redhat.com>

	* patches/icedtea-lc_ctype.patch: Fixed array size and 
	changed to use malloc/free.

and she's since left Red Hat.

My version is based on this original, the one in IcedTea, with the addition
of a check on the return value of malloc.  I have no idea what happened with Omair's
extended version.  It's not in IcedTea.

> I reviewed it at the time and gave a thumbs up, it's just it was the
> end-game for 7 at the time and that release was being locked down. I
> asked that it be held back until jdk8 and jdk7u.
> 
> Omair's webrev is a larger patch that what you are proposing now,
> mostly
> handling malloc/realloc failing that we nit-picked in the original
> review. I haven't checked IcedTea to know whether it has the minimal
> patch you are proposing now or whether it has Omair's changes.
> 
> In any case, there is a Sun bug open for this:
> 
> 6844255: Potential stack corruption in GetJavaProperties
> 

Can I take it that I can just get on and push Omair's extended version now then,
with that bug ID?

> Also if you read the old mails then you'll see that we were
> scratching
> our heads as to an example that would demonstrate the original issue.
> I
> suspect it may have been something that someone spotted rather than
> someone running with a locale of this length.

Well, the locale can be set be an environment variable, so it could potentially
be anything of any length...

The Debian bug posted above has an example, though I couldn't replicate it.

> 
> -Alan.
> 

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: 248BDC07 (https://keys.indymedia.org/)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F  8F91 3B96 A578 248B DC07