[RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Weijun Wang
weijun.wang at oracle.com
Mon Sep 10 14:31:25 UTC 2018
> On Sep 10, 2018, at 9:59 PM, Baesken, Matthias <matthias.baesken at sap.com> wrote:
>
> New webrev :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.9/
Looks fine to me.
>
> - SocketExceptions class has been adjusted to new sun.security.util.SecurityProperties
> - Attributes getErrorPosition adjusted (see proposal of Christoph " I think it would be enough to drop the privileged section and just return "filename" as is.)
>
> After the changes I wonder - should the jarPath category be renamed to jarFile (or something else) ?
jarFile seems to be an instance of JarFile. I don't think jarPath has any problem.
Thanks
Max
>
>
> Best regards, Matthias
>
>> -----Original Message-----
>> From: Langer, Christoph
>> Sent: Montag, 10. September 2018 10:04
>> To: Baesken, Matthias <matthias.baesken at sap.com>; 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: 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-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
>>
>
More information about the core-libs-dev
mailing list