[RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Langer, Christoph]

Hi Matthias,

I think it would be enough to drop the privileged section and just return "filename" as is. (without conveting to a file object).

I also agree with Sean that the common parts of the new src/java.base/share/classes/sun/security/util/SecurityProperties.java should be aligned with src/java.base/share/classes/sun/net/util/SocketExceptions.java (That is, SocketExceptions calling SecurityProperties)

Best regards
Christoph


> -----Original Message-----
> From: core-libs-dev <core-libs-dev-bounces at openjdk.java.net> On Behalf
> Of Baesken, Matthias
> Sent: Montag, 10. September 2018 09:53
> To: Wang Weijun <weijun.wang at oracle.com>; Sean Mullan
> <sean.mullan at oracle.com>
> Cc: security-dev at openjdk.java.net; core-libs-dev at openjdk.java.net
> Subject: [CAUTION] RE: [RFR] 8205525 : Improve exception messages during
> manifest parsing of jar archives
> 
> Hello are you fine with  changing from  file.getAbsolutePath()    to
> file.getName() ?
> 
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/src/java.base/s
> hare/classes/java/util/jar/Manifest.java.frames.html
> 
> 
> 206     static String getErrorPosition(String filename, final int lineNumber) {
>  207         if (filename == null || !jarPathInExceptionText) {
>  208             return "line " + lineNumber;
>  209         }
>  210
>  211         final File file = new File(filename);
>  212         return AccessController.doPrivileged(new PrivilegedAction<String>()
> {
>  213             public String run() {
>  214                 return "manifest of " + file.getName() + ":" + lineNumber;
>  215             }
> 
> 
> Best regards, Matthias
> 
> 
> > -----Original Message-----
> > From: Wang Weijun <weijun.wang at oracle.com>
> > Sent: Samstag, 8. September 2018 17:43
> > To: Sean Mullan <sean.mullan at oracle.com>
> > Cc: Baesken, Matthias <matthias.baesken at sap.com>; Alan Bateman
> > <Alan.Bateman at oracle.com>; Chris Hegarty <chris.hegarty at oracle.com>;
> > security-dev at openjdk.java.net; core-libs-dev at openjdk.java.net
> > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> > parsing of jar archives
> >
> > Thinking about this again. Looks like the absolute path is not necessary.
> Even
> > if there are multiple files using the same name, they will be in different
> > directories, no matter absolute or relative. Suppose the jarPath info is used
> > for debugging purpose mostly like the developer can find out what the
> > current working directory is and can find the files. *Matthias*: Is the
> absolute
> > path really necessary? Are you working on actual case?
> >
> > As for the possible global effect of a security property, maybe we can
> > emphasis that this is both a security property and system property, and if
> it’s
> > just for one time use, it’s better to use a system property.
> >
> > BTW, does the existing value “hostInfo” of the property have a similar
> > problem?
> >
> > Thanks
> > Max
> >
> > >> 在 2018年9月8日，21:42，Sean Mullan <sean.mullan at oracle.com> 写
> > 道：
> > >>
> > >> On 9/7/18 7:58 PM, Weijun Wang wrote:
> > >> In my understanding, the author deliberately wants to show the
> absolute
> > paths when there are multiple jar files with the same name (Ex: a jar hell).
> > >
> > > If you are very familiar with a particular application and understand the
> risks
> > associated with running it, then I agree that is ok. But security properties
> > apply to all applications using that JDK, and so I don't always think it is
> > practical for an admin to understand every type of application that may be
> > using that JDK and whether or not enabling this property presents a risk.
> > >
> > >> Maybe we can add some more detail in the java.security so an admin
> > knows what exact impact it has.
> > >
> > > It would be a slippery slope in my opinion. You would have to say
> > something like: "enabling this property may allow untrusted code running
> > under a SecurityManager to access sensitive information such as the
> > user.home system property even if it has not been granted permission to
> do
> > so."
> > >
> > > I think we should consider not allowing this property to take effect if a
> > SecurityManager is running.
> > >
> > > --Sean
> > >
> > >> --Max
> > >>> On Sep 8, 2018, at 3:41 AM, Sean Mullan <sean.mullan at oracle.com>
> > wrote:
> > >>>
> > >>> On 8/29/18 10:01 AM, Baesken, Matthias wrote:
> > >>>> Hi Max, thanks for your input .
> > >>>> I created another webrev , this contains now   the suggested
> > SecurityProperties   class :
> > >>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
> > >>>
> > >>> java/util/jar/Attributes.java
> > >>>
> > >>> 469         return AccessController.doPrivileged(new
> > PrivilegedAction<String>() {
> > >>> 470             public String run() {
> > >>> 471                 return file.getAbsolutePath() + ":" + lineNumber;
> > >>> 472             }
> > >>> 473         });
> > >>>
> > >>> I have a serious concern with the code above.
> > >>>
> > >>> With this change, untrusted code running under a SecurityManager
> > could potentially create a JarFile on a non-absolute path ex: "foo.jar", and
> > (somehow) cause an IOException to be thrown which would then reveal
> the
> > absolute path of where the jar was located, and thus could reveal sensitive
> > details about the system (ex: the user's home directory). It could still be
> hard
> > to exploit, since it would have to be a known jar that exists, and you would
> > then need to cause an IOException to be thrown, but it's still theoretically
> > possible.
> > >>>
> > >>> In short, this is a more general issue in that it may allow untrusted code
> > to access something it couldn't have previously. new
> > JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path.
> > >>>
> > >>> Granted this can only be done if the security property is enabled, but I
> > believe this is not something administrators should have to know about
> their
> > environment and account for when enabling this property.
> > >>>
> > >>> The above code should be changed to return only what the caller
> > provides to JarFile, which is the name of the file (without the full path).
> > >>>
> > >>> --Sean