> I think it would be enough to drop the privileged section and just return > "filename" as is. (without conveting to a file object).
OK, any objections on this ? > 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) OK I will include this in the next webrev . Thanks, Matthias > -----Original Message----- > From: Langer, Christoph > Sent: Montag, 10. September 2018 10:04 > To: Baesken, Matthias <matthias.baes...@sap.com>; Wang Weijun > <weijun.w...@oracle.com>; Sean Mullan <sean.mul...@oracle.com> > Cc: security-...@openjdk.java.net; core-libs-dev@openjdk.java.net > Subject: RE: [RFR] 8205525 : Improve exception messages during manifest > parsing of jar archives > > 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-boun...@openjdk.java.net> On Behalf > > Of Baesken, Matthias > > Sent: Montag, 10. September 2018 09:53 > > To: Wang Weijun <weijun.w...@oracle.com>; Sean Mullan > > <sean.mul...@oracle.com> > > Cc: security-...@openjdk.java.net; core-libs-dev@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.w...@oracle.com> > > > Sent: Samstag, 8. September 2018 17:43 > > > To: Sean Mullan <sean.mul...@oracle.com> > > > Cc: Baesken, Matthias <matthias.baes...@sap.com>; Alan Bateman > > > <alan.bate...@oracle.com>; Chris Hegarty > <chris.hega...@oracle.com>; > > > security-...@openjdk.java.net; core-libs-dev@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.mul...@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.mul...@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 >