Re: Review Request: 8150468: ClassCircularityError on error in security policy file
> On May 6, 2016, at 11:43 AM, Sean Mullan wrote: > > Please review this fix for https://bugs.openjdk.java.net/browse/JDK-8150468: > >http://cr.openjdk.java.net/~mullan/webrevs/8150468/webrev.00/ > > The fix is to record bad policy files as they are parsed and ignore them > during any subsequent permission checks. Looks okay. PolicyFile::init catches ParsingException that always calls ParsingException::getLocalizedMessage and prints the localized message. Is that necessary? We don’t typically localize the exception message if thrown at runtime. Mandy
Re: RFR: regex changes -- sun.security.util.Debug issue
> On May 10, 2016, at 1:44 PM, Xueming Shen wrote: > > On 5/10/16 1:10 PM, Alan Bateman wrote: >> >> >> On 10/05/2016 19:57, Xueming Shen wrote: >>> webrev has been updated as suggested, now the lazily-init-class-holder is >>> used >>> to delay the Debug initialization. Tests all passed locally. A jprt job is >>> out to confirm. >>> >>> Here is the updated webrev >>> >>> http://cr.openjdk.java.net/~sherman/regexBackTrack.Lamnda.CanonEQ/webrev/ >> This looks okay, are exploded builds okay too? >> >> -Alan > > > jprt job appears to be OK. > > exploded build failed here and there for various reasons, for example "access > denied". but it has > nothing to do with the regex change. a jdk9_dev build without any update > fails as well with the same > reason at same place. here is one example > The security policy in the build only grants to jrt: code source and running with the exploded image won’t work with that. It’d be nice to avoid running with a different java.policy: https://bugs.openjdk.java.net/browse/JDK-8155858 Mandy
Re: RFR 8062758: Update java/security/Security/ClassLoaderDeadlock/Deadlock2.sh with the removal of -Djava.ext.dirs
> On Jun 5, 2016, at 10:40 PM, Bhanu Gopularam > wrote: > > Hi all, > > Please review fix for following bug : > > Bug - https://bugs.openjdk.java.net/browse/JDK-8062758 > > Issue - Test java/security/Security/ClassLoaderDeadlock/Deadlock2.sh started > failing after removal of java.ext.dirs option > > Actually the test loads two classes SunJCE provider class (uses > ExtClassLoader in 8) and java.xml.parsers.DocumentBuilderFactory (loaded by > bootstrap classloader) class using two separate threads simultaneously. In > jdk-9 the provider classes are part of java.base and loaded by bootstrap > loader. The ext classloader is replaced by platform classloader which is > confined to load classes from modules only. Removing this particular test as > scenario mentioned in it is not possible with new JDK-9 changes. > > Webrev - http://cr.openjdk.java.net/~bgopularam/8062758/webrev.00 Removing this test seems okay since there are existing tests checking for deadlocks. Mandy
Re: [9] RFR 8157489: AppleProvider in java.base/macosx/classes/module-info.java.extra
> On Jun 9, 2016, at 3:22 PM, Valerie Peng wrote: > > > Anyone can help reviewing this one-line change which removes a redundant > declaration? > > As Apple provider is instantiated directly (see > sun.security.jca.ProviderConfig.java) and not loaded through ServiceLoader , > we can safely remove the line for ServiceLoader lookup. No new regression > test as this is just a minor performance fix. > > Webrev: http://cr.openjdk.java.net/~valeriep/8157489/ So all builtin security providers in java.base will not be found from ServiceLoader.load(Provider.class). test/java/security/Provider/DefaultProviderList.java should then be updated to expect all providers are not from java.base and check Class::getModule(). Currently the test simply skips some builtin security providers. Mandy
Re: [9] RFR 8157489: AppleProvider in java.base/macosx/classes/module-info.java.extra
> On Jun 9, 2016, at 4:22 PM, Valerie Peng wrote: > > > Thanks for the comments. I will update the reg test, i.e. > test/java/security/Provider/DefaultProviderList.java, to check that they are > from java.base. > To clarify: what I mean is that you should remove the logic that skips the built-in security provider if found. Also for the security provider, it should check it comes from a module other than java.base. > I don't see a need for built-in security providers to be found through > ServiceLoader.load(Provider.class) though. The expected API usage is to get > the provider instance through Security.getProvider(String provName). Right that’s implementation details. I have no issue with that. Mandy > Regards, > Valerie > > On 6/9/2016 3:31 PM, Mandy Chung wrote: >>> On Jun 9, 2016, at 3:22 PM, Valerie Peng wrote: >>> >>> >>> Anyone can help reviewing this one-line change which removes a redundant >>> declaration? >>> >>> As Apple provider is instantiated directly (see >>> sun.security.jca.ProviderConfig.java) and not loaded through ServiceLoader >>> , we can safely remove the line for ServiceLoader lookup. No new regression >>> test as this is just a minor performance fix. >>> >>> Webrev: http://cr.openjdk.java.net/~valeriep/8157489/ >> So all builtin security providers in java.base will not be found from >> ServiceLoader.load(Provider.class). >> >> test/java/security/Provider/DefaultProviderList.java should then be updated >> to expect all providers are not from java.base and check Class::getModule(). >> Currently the test simply skips some builtin security providers. >> >> Mandy >>
Re: [9] RFR 8157489: AppleProvider in java.base/macosx/classes/module-info.java.extra
> On Jun 9, 2016, at 5:26 PM, Valerie Peng wrote:
>
> Webrev updated at: http://cr.openjdk.java.net/~valeriep/8157489/webrev.01
>
51 if (!pClass.getModule().getName().equals("java.base")) {
You can check if Module object is Object.class.getModule() instead of checking
the module name.
Since you expect no provider from java.base, the test should detect that e.g.
if (pClass.getModule() == Object.class.getModule())
throw new RuntimeException(…);
Otherwise, looks okay.
Mandy
Re: RFR: 8155039: Simplify code to setup SSLContextImpl and TrustManagerFactoryImpl
Hi Claes, I don’t like the PropertiesWrapper idea. The caller should be cautious in storing any sensitive information. For the system properties, these callsites use it in the local scope that I don’t see any reason and benefit to introduce a wrapper. I didn’t follow this discussion closely and I may miss some reason ? Mandy > On Jun 9, 2016, at 5:31 PM, Claes Redestad wrote: > > Hi, > > by using a non-exported type, PropertiesWrapper, to encapsulate Properties > this change makes it impossible for a JDK developer to accidentally leak > system properties outside of java.base without breaking encapsulation. I > believe that was yours and Sean's main concern about the API changes to > GetPropertyAction that this is hopefully a final amendment to. > > Generally the changes to streamline system property access bring about minor > improvements to startup and footprint by reducing the number of classes > generated and loaded as well as the number of doPriv calls done. > > /Claes > > On 2016-06-08 03:24, Xuelei Fan wrote: >> Hi Claes, >> >> What's the necessary to get all system properties for just one specific >> one? Can you explain more about the necessary to make the change? >> >> Thanks, >> Xuelei >> >> On 6/8/2016 3:44 AM, Claes Redestad wrote: >>> Hi, >>> >>> there is some lingering concern that this and related changes make it >>> that much easier to accidentally leak the system Properties object >>> outside of core modules. By wrapping access to the system Properties >>> object in a class residing in a non-exported package we disallow this at >>> little to no cost: >>> >>> http://cr.openjdk.java.net/~redestad/8155039/webrev.02/ >>> >>> /Claes >>> >>> On 2016-05-12 15:37, Claes Redestad wrote: Hi, the API this improvement depends on was updated to reflect more clearly that this is taking a privileged action: https://bugs.openjdk.java.net/browse/JDK-8155775 Here's the updated webrev: http://cr.openjdk.java.net/~redestad/8155039/webrev.01/ Thanks! /Claes On 2016-04-25 19:28, Claes Redestad wrote: > Hi, > > SSLContextImpl and TrustManagerFactoryImpl has some setup code that > could be > simplified, getting rid of a couple of anonymous classes in the process. > > Webrev: http://cr.openjdk.java.net/~redestad/8155039/webrev.00 > Bug: https://bugs.openjdk.java.net/browse/JDK-8155039 > > Alternatively we could remove OpenFileInputStreamAction instead since > these two uses > introduced here are actually the only active uses of this old > convenience class. > > Thanks! > > /Claes >>
Re: RFR: 8155039: Simplify code to setup SSLContextImpl and TrustManagerFactoryImpl
> On Jun 10, 2016, at 4:33 AM, Sean Mullan wrote: > > On 06/09/2016 10:32 PM, Mandy Chung wrote: >> Hi Claes, >> >> I don’t like the PropertiesWrapper idea. The caller should be >> cautious in storing any sensitive information. For the system >> properties, these callsites use it in the local scope that I don’t >> see any reason and benefit to introduce a wrapper. I didn’t follow >> this discussion closely and I may miss some reason ? > > The original code used multiple calls to System.getProperty wrapped in a > doPrivileged. Claes' first iteration of the fix changed this to use a > GetPropertyAction.privilegedGetProperties method that returned a Properties > object. I expressed a concern that this was now exposing an object that, if > accidentally leaked to untrusted code could cause much more damage than the > original code (since the code would be able to set/get/remove *any* system > property). Hence the current fix which uses a wrapper class which is not > exported. I actually see the original code is clearer to the reader and involves one single doPrivileged. I would avoid introducing PropertiesWrapper which I don’t think it’s the right way to protect security information. Sean may suggest to revert to the original code which I won’t object. Mandy
Re: [9] RFR 8157489: AppleProvider in java.base/macosx/classes/module-info.java.extra
> On Jun 10, 2016, at 3:51 PM, Valerie Peng wrote: > > Webrev updated at: http://cr.openjdk.java.net/~valeriep/8157489/webrev.02. Looks fine. Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jun 12, 2016, at 11:33 AM, Alan Bateman wrote: > > > > On 12/06/2016 13:44, Wang Weijun wrote: >> I was about to send out a new webrev (CCC just approved) but noticed a >> behavior change. >> >> Although "-addprovider SUN" is useless it still worked when I posted >> webrev.03, but now it failed, because ServiceLoader.load(Provider.class) >> does not contain "SUN" anymore. Maybe it is inside java.base and loaded in a >> shortcut mode? >> > "SUN" ,"SunJCE", "SunRsaSign", and "SunJSSE" are built-in, I think Valerie > has code in sun.security.jca.ProviderConfig for this. I don't recall > java.base ever declaring that it `provides` these providers, except maybe via > a META-INF/services configuration file for a short period from the original > JCE work and the dropping the service configuration files. I think Alan is right. They were not loaded via ServiceLoader.load because of the build complexity to get multiple service config files before the module system went in jdk9. As it stands now, no provides java.security.Provider in java.base after JDK-8157489 is resolved. Mandy
Re: [9] RFR 8154191: Deprivilege java.smartcardio module
> On Jun 13, 2016, at 5:10 PM, Valerie Peng wrote: > > Sean, > > Can you please review the changes for deprivileging the java.smartcardio > module? > I have to update one makefile in the top-level workspace besides the > java.policy file in the jdk workspace. > One java.smartcardio regression test uses custom policy file which needs to > be updated as well. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8154191 > Webrevs: > top-level=> http://cr.openjdk.java.net/~valeriep/8154191/webrev.00/ > jdk=> http://cr.openjdk.java.net/~valeriep/8154191/jdk/webrev.00/ make/common/Modules.gmk change looks okay to me. Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jun 13, 2016, at 8:28 PM, Wang Weijun wrote:
>
> OK, please take a review at the new version at
>
> http://cr.openjdk.java.net/~weijun/8130302/webrev.04/
>
The new -addProvider option is good. I mostly reviewed KeyStoreUtil.java and
skimmed through other files that I assume the security team will review them in
details.
267 * Loads a security provider in a module with its name.
This does not limit to modules. It can load security providers from classpath
via ServiceLoader.
282 for (Provider p : ServiceLoader.load(Provider.class)) {
This should use ServiceLoader.load(Provider.class,
ClassLoader.getSystemClassLoader()) instead of loading with TCCL.
291 throw new IllegalArgumentException();
Nit: good to have a message even if it’s not used.
295 * Loads a non-modularized security provider with its full-qualified
name.
I suggest to reword it to “Loads a security provider by a fully-qualified class
name”
move line 306 to 317
241 throw (InvalidParameterException)
This cast should not be needed?
319 Class clazz;
320 if (cl != null) {
321 clazz = cl.loadClass(provClass);
322 } else {
323 clazz = Class.forName(provClass);
324 }
You should call Class.forName(provClass, false, cl) instead.
Mandy
Re: RFR 8130302: jarsigner and keytool -providerClass needs be re-examined for modules
> On Jun 15, 2016, at 1:27 AM, Wang Weijun wrote:
>
> All suggestions accepted. Webrev updated at
>
> http://cr.openjdk.java.net/~weijun/8130302/webrev.05
>
>> 241 throw (InvalidParameterException)
>>
>> This cast should not be needed?
>>
>
> } catch (UcryptoException ue) {
>throw (InvalidParameterException)
>new InvalidParameterException("Error using " + configArg).
>initCause(ue.getCause());
> }
>
> initCause() returns Throwable but the method's signature throws
> InvalidParameterException.
>
Perhaps have a local variable for InvalidParameterException exception.
Mandy
Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java
I’m including security-dev which would be a better list to review this test fix. Valerie, Does this test have to be order-sensitive? I think this test would be cleaner to make it order-insensitive and simply test the security provider initialization. See my comments below. > On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline > wrote: > > Hi. > > Please take a look on a suggested for for the > java/lang/SecurityManager/CheckSecurityProvider.java test. > > The test in question depend on a list of modules, some of them are > platform-specific. Listing all the dependencies in one test is causing the > test to be skipped on every platform. In an offline conversation it was > decided that it is better to split this tests into a few tests to declare the > per-platform module dependencies. > > The bug: https://bugs.openjdk.java.net/browse/JDK-8158670 > The suggested fix: http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/ The copyright header start year of the new tests should be 2016. I would suggest to make CheckSecurityProvide a platform-neutral test, i.e., - drop @requires - make line 94-97 to ignore the platform-dependent provider if it’s present in the white list If we could make this test order-insensitive, it’d be cleaner to maintain a platform-neutral list of security providers and one list for the platform-dependent security providers for each platform. Just an idea. Mandy
Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java
Valerie’s suggestion is a good one. The test will require a minimum image with cross-platform security providers to run the test while it can still verify other platform-specific providers. Mandy > On Jun 29, 2016, at 10:41 AM, Valerie Peng wrote: > > > It's not like the test silently passes as the test still covers the > cross-platform modules. > The way I view this is that the platform=specific modules are "optional" and > we update the expected result by detecting their presence (or the not). It's > not a hack or workaround, but rather an enhancement for the test to handle > different images. > > Just my .02, > Valerie > > On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote: >>> On Jun 28, 2016, at 5:22 PM, Valerie Peng wrote: >>> >>> >>> One of the purpose of this test is to test the ordering (see the initial >>> bug which this test is for: JDK-6997010). >>> >>> The original test already detects the OS and will skip certain providers >>> accordingly. >>> Instead of splitting the test into multiple platform-specific tests, maybe >>> we can keep the original test but add module-presence checking? Is there >>> API available to query if certain module are present? >> ModuleFinder.ofSystem().find(String). >> >> We can have only the cross-platform modules listed in @modules and make the >> test to pass silently if the required platform-specific modules are not >> present. >> >> So, for example, on windows, if the test would be executed against an image >> which have no jdk.crypto.mscapi, the test will not run any checks and report >> pass. >> >> This would help to avoid the additional test creation, but it will add >> another silently passing test, which is less clean. >> >> Mandy? >> >> Shura >> >>> If yes, then we can leave out the platform-specific providers from the >>> @modules line and skip the providers if either the OS does not match or the >>> module is not present. >>> >>> If we can't query what modules are available, then we may have to think of >>> something else. >>> Valerie >>> >>> On 6/27/2016 12:27 PM, Mandy Chung wrote: >>>> I’m including security-dev which would be a better list to review this >>>> test fix. >>>> >>>> Valerie, >>>>Does this test have to be order-sensitive? I think this test would be >>>> cleaner to make it order-insensitive and simply test the security provider >>>> initialization. >>>> >>>> See my comments below. >>>> >>>>> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline >>>>> wrote: >>>>> >>>>> Hi. >>>>> >>>>> Please take a look on a suggested for for the >>>>> java/lang/SecurityManager/CheckSecurityProvider.java test. >>>>> >>>>> The test in question depend on a list of modules, some of them are >>>>> platform-specific. Listing all the dependencies in one test is causing >>>>> the test to be skipped on every platform. In an offline conversation it >>>>> was decided that it is better to split this tests into a few tests to >>>>> declare the per-platform module dependencies. >>>>> >>>>> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670 >>>>> The suggested fix: >>>>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/ >>>> The copyright header start year of the new tests should be 2016. >>>> >>>> I would suggest to make CheckSecurityProvide a platform-neutral test, i.e., >>>> - drop @requires >>>> - make line 94-97 to ignore the platform-dependent provider if it’s >>>> present in the white list >>>> >>>> If we could make this test order-insensitive, it’d be cleaner to maintain >>>> a platform-neutral list of security providers and one list for the >>>> platform-dependent security providers for each platform. Just an idea. >>>> >>>> Mandy >>>> >
Re: RFR: 8157847: Deprecate the java.security.acl API with forRemoval=true
> On Jul 1, 2016, at 5:31 AM, Sean Mullan wrote: > > Please review the changes for this RFE to mark the java.security.acl API with > forRemoval=true. The intention is to remove this API in JDK 10. > > The APIs in java.security.acl were deprecated in 1.9 but have had the > following warning in the package description for many releases: > > "The classes and interfaces in this package have been superseded by classes > in the java.security package. See that package and, for example, > java.security.Permission for details." > > Since there have been suitable replacements since 1.2, there is no reason to > retain this package and it should be removed. > > webrev: http://cr.openjdk.java.net/~mullan/webrevs/8157847/webrev.00/ Should be @since=“9” rather than “1.9”. Otherwise, looks okay. Mandy
Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java
> On Jul 1, 2016, at 6:20 PM, Alexandre (Shura) Iline
> wrote:
>
> Please review the new version of the fix.
> http://cr.openjdk.java.net/~shurailine/8158670/webrev.01/
This looks much better. Small comment: you can use Layer::findModule and also
Optional::ifPresent, like this:
boot.findModule("jdk.crypto.ucrypto”)
.ifPresent(m ->
expected.add("com.oracle.security.ucrypto.UcryptoProvider”));
Mandy
Re: Questions on deprivileging a module
> On Jul 5, 2016, at 12:15 AM, Wang Weijun wrote: > >>> 1. How does updating /make/common/Modules.gmk affect an exploded build? >> The mappings are used for both exploded and images build so the >> configuration in this make file is for both. > > I see. BTW, which file contain the mappings? It’s generated at the build time: $BUILD_OUTPUTDIR/support/gensrc/java.base/jdk/internal/module/ModuleLoaderMap.java Mandy
Re: Strange test failure when referencing a class in a deprivileged module
> On Jul 5, 2016, at 8:11 AM, Alan Bateman wrote: > > On 05/07/2016 09:22, Wang Weijun wrote: > >>> On Jul 5, 2016, at 11:53 AM, Wang Weijun wrote: >>> >>> The exception is at the end of this mail. The test passes if I change >>> X.go() to calling a method inside this class >> Update: any call that triggers a permission check will do. For example: >> >> > Yes, tricky issue because checkPermission is triggering class loading and > permission checks. It worked previously because the java.sql module was > defined to the boot loader. Max - are you running the test with exploded image (see JDK-8155858 [1])? The java.policy grants permission to modules in the jimage. Mandy [1] https://bugs.openjdk.java.net/browse/JDK-8155858
Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java
> On Jul 5, 2016, at 12:42 PM, Alexandre (Shura) Iline > wrote: > > This made sense, than you, Mandy. > > Please review new version: > http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/ You can use Layer::findModule instead of Configuration::findModule. You can also use List::equals. Mandy
Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java
> On Jul 5, 2016, at 1:53 PM, Alexandre (Shura) Iline > wrote: > > >> On Jul 5, 2016, at 1:36 PM, Mandy Chung wrote: >> >> >>> On Jul 5, 2016, at 12:42 PM, Alexandre (Shura) Iline >>> wrote: >>> >>> This made sense, than you, Mandy. >>> >>> Please review new version: >>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/ >> >> You can use Layer::findModule instead of Configuration::findModule. > > That is correct. Changed in place. > >> >> You can also use List::equals. > > I am assuming you are suggesting to use List::equals in the bottom part of > the test where the expected result is compared with the actual list of > providers. The whole reason I redid that section to provide more information > in the jtr file, both for a case of a failure and to find out what providers > were actually expected for given configuration. I do not see how List:equals > help me with that. Information on size mismatch is useful, and also the > information on unexpected provider name. What you have is fine. The information is useful to help diagnosis. The alternative I was thinking is to check List::equals and if not equals, do line-108-117. It’s a minor thing and up to you. Mandy
Re: RFR 8159528: Deprivilege java.security.jgss, jdk.security.jgss and jdk.security.auth
> On Jul 11, 2016, at 4:15 PM, Weijun Wang wrote: > > Hi All > > Please review the code change at > > dev: http://cr.openjdk.java.net/~weijun/8159528/dev/webrev.00 Good to keep the PLATFORM_MODULE list in alphabetical order. So java.security.jgss should be moved up. > dev/jdk: http://cr.openjdk.java.net/~weijun/8159528/jdk/webrev.00 > Two comments related to the use of unsafe here. You use reflection to get access to Unsafe instance to bypass the caller’s class loader check. I think Unsafe::getUnsafe should be relaxed for null or ClassLoader::getPlatformLoader to access. On the other hand, we should also examine the use of Unsafe and see if they can be converted to VarHandle or other means. Maybe you can file a JBS issue for Unsafe::getUnsafe to allow the platform class loader to access it and another one to examine the use of unsafe in java.security.jgss module. I file a JBS issue to track this (JDK-8161121) to audit the callers of VM::isSystemDomainLoader and make appropriate adjustment. Mandy
Re: RFR 8159528: Deprivilege java.security.jgss, jdk.security.jgss and jdk.security.auth
> On Jul 11, 2016, at 9:38 PM, Weijun Wang wrote: > > > > On 7/11/2016 16:50, Mandy Chung wrote: >> >>> On Jul 11, 2016, at 4:15 PM, Weijun Wang wrote: >>> >>> Hi All >>> >>> Please review the code change at >>> >>> dev: http://cr.openjdk.java.net/~weijun/8159528/dev/webrev.00 >> >> Good to keep the PLATFORM_MODULE list in alphabetical order. So >> java.security.jgss should be moved up. > > The existing list in not in alphabetical order: > >jdk.jsobject \ >jdk.xml.dom \ >jdk.localedata \ >jdk.naming.dns \ >jdk.scripting.nashorn \ >jdk.zipfs \ > > Shall I reorder them? yes please if you don’t mind. Mandy
Re: @run main/policy ignores system default java.policy
> On Jul 13, 2016, at 3:23 PM, Weijun Wang wrote:
>
>
>
> On 7/8/2016 19:34, Sean Mullan wrote:
>>
>> Use the new jtreg "java.security.policy=p" option. This will
>> concatentate the specified policy with the configured system policy files.
>
> I've used this new option.
>
> However, it also picks up ~/.java.policy, which I believe will bring in some
> noise. Maybe jtreg can be enhanced to ignore ~/.java.policy?
IMO it’s a configuration issue. We should make sure ${java.home}/.java.policy
is not present when running the regression tests.
In addition, to ignore ${java.home}/.java.policy, it’s a change in the policy
file I believe (not in jtreg).
Mandy
Re: [9] RFR 8159488 "Deprivilege java.xml.crypto" and 8161171 "Missed the make/common/Modules.gmk file when integrating JDK-8154191"
> On Jul 14, 2016, at 8:10 AM, Valerie Peng wrote: > > Sean, > > Can you please review the following two webrevs? > > Bug: https://bugs.openjdk.java.net/browse/JDK-8159488 > Webrev: http://cr.openjdk.java.net/~valeriep/8159488/ > Update to Modules.gmk looks good. > While making changes for 8159488, I noticed a problem with my earlier putback > of 8154191 - the top level Modules.gmk was not integrated. > So, I filed 8161171: Missed the make/common/Modules.gmk file when integrating > JDK-8154191. > Can you also review this? It's essentially the same change as the one > reviewed. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8161171 > Webrev: http://cr.openjdk.java.net/~valeriep/8161171/webrev.00/ +1 You can go ahead and push the fix for JDK-8161171. Mandy
Re: RFR: 8159752: Grant de-privileged module permissions by default with java.security.policy override option
> On Jul 15, 2016, at 4:05 AM, Sean Mullan wrote:
>
> Please review this change to the default Policy provider implementation to
> grant de-privileged module permissions by default even when the
> java.security.policy override option is specified or when the
> Policy.getInstance API is used:
>
> http://cr.openjdk.java.net/~mullan/webrevs/8159752/webrev.00/
>
Thanks for addressing the `==` overriding issue. As you replied in another
mail, this patch prepares the future work to modularize java.policy for modules.
I skimmed on the patch. The build change and java.policy and default.policy
files look okay. I will leave the detailed review to the security team.
Nit: line 312-314 - an alternative is to use:
Paths.get(System.getProperty("java.home”), “lib”, “security”,
“default.policy”);
Mandy
Re: [9] RFR 8154113: java.security.AccessControlException: access denied ("java.security.SecurityPermission" "authProvider.SunMSCAPI")
A passing comment: The test can simply specify @modules jdk.crypto.mscapi and this test will only run when jdk.crypto.mscapi is present; in other words, line 45-48 are no longer needed. Mandy > On Aug 3, 2016, at 6:09 AM, Sean Mullan wrote: > > Hi Valerie, > > Can you also clean up the noaccess.policy and access.policy files and remove > the permissions that are now granted to the jdk.crypto.mscapi module in > default.policy? > > Also, in the test, I would use the java.security.policy== option instead of > the policy option. The policy option is an older jtreg option that I think we > should phase out from tests. The java.security.policy option is easier to > understand because it is consistent with the system property. > > --Sean > > On 08/02/2016 06:04 PM, Valerie Peng wrote: >> Sean, >> >> Would you be able to review this policy update for SunMSCAPI provider? >> In addition to the policy update, the test is updated to replace the >> shell script with @run tags. >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8154113 >> Webrev: http://cr.openjdk.java.net/~valeriep/8154113/webrev.00/ >> >> Thanks, >> Valerie
Re: [9] RFR 8154113: java.security.AccessControlException: access denied ("java.security.SecurityPermission" "authProvider.SunMSCAPI")
> On Aug 4, 2016, at 7:20 AM, Sean Mullan wrote: > > On 08/04/2016 12:42 AM, Mandy Chung wrote: >> A passing comment: >> >> The test can simply specify @modules jdk.crypto.mscapi and this test will >> only run when jdk.crypto.mscapi is present; in other words, line 45-48 are >> no longer needed. > > Does that also mean line 27 can be removed: > > 27 * @requires os.family == “windows" > I think it should also be removed but you should verify it to make sure jtreg selects this test on windows only (where jdk.crypto.mscap1 is present). Mandy
Re: RFR: 8163126 Wrong @modules in some of jdk/* tests
> On Aug 16, 2016, at 10:17 AM, Alexandre (Shura) Iline > wrote: > > Hi. > > Please review fixes related to module dependencies in a few jdk tests: > http://cr.openjdk.java.net/~shurailine/8163126/webrev.00/index.html Looks okay. I will sponsor this patch for you. Mandy
Re: RFR:8166632: Document how to grant permissions for a module jrt:/ in the image
> On Oct 5, 2016, at 8:18 AM, Sean Mullan wrote: > > On 10/05/2016 10:58 AM, Alan Bateman wrote: >> >> >> On 05/10/2016 15:57, Sean Mullan wrote: >>> The "jrt" URL scheme syntax is new and not widely known yet, so an >>> example has been added to the conf/security/java.policy file to help >>> users show how to grant permissions to a module: >>> >>> http://cr.openjdk.java.net/~mullan/webrevs/8166632/webrev.00/ >> It might be clearer to say "for modules linked into the runtime image" >> rather just "for modules", just in case someone thing this is the way to >> grant permissions to modules on the module path. > > Good point. Will make that change. +1. Mandy
Re: RFR 8167181: Exported elements referring to inaccessible types in jdk.security.jgss
> On Oct 6, 2016, at 9:55 AM, Wang Weijun wrote: > > Please review the code change for jdk9/dev/jdk repo at > > http://cr.openjdk.java.net/~weijun/8167181/webrev.00 This looks good to me. Thanks Mandy
Re: [9] RFR 8165275: Replace the reflective call to the implUpdate method in HandshakeMessage::digestKey
On Oct 4, 2016, at 3:07 PM, Valerie Peng wrote: > > Hi Xuelei, > > Could you please help reviewing this JSSE-related code refactoring? > > Bug: https://bugs.openjdk.java.net/browse/JDK-8165275 > Webrev: http://cr.openjdk.java.net/~valeriep/8165275/webrev.00/ > Looks okay to me. Mandy
Re: RFR: 8168313: Tighten permissions granted to jdk.crypto.pkcs11 module
Looks good to me. Mandy > On Oct 20, 2016, at 8:22 AM, Sean Mullan wrote: > > Please review this change to tighten or remove unnecessary permissions > granted to the jdk.crypto.pkcs11 module: > > http://cr.openjdk.java.net/~mullan/webrevs/8168313/webrev.00/ > https://bugs.openjdk.java.net/browse/JDK-8168313 > > In doing so, I refactored the code a little to not use sun.security.action > APIs and was able to remove a qualified export from java.base. I also moved > the reading of the os.name and os.arch system properties inside a > doPrivileged block just in case the caller(s) do not have permission to read > them. > > --Sean
Re: RFR: 8168851: Tighten permissions granted to the java.smartcardio module
> On Oct 27, 2016, at 10:00 AM, Sean Mullan wrote: > > I accidentally used the wrong bugid. I have updated the Subject line and > links to reflect that this fix is for the java.smartcardio module, not the > jdk.crypto.ucrypto module: > > https://bugs.openjdk.java.net/browse/JDK-8168851 > http://cr.openjdk.java.net/~mullan/webrevs/8168851/webrev.00/ > +1 Mandy
Re: RFR 8168075: Custom system class loader + security manager + malformed policy file = recursive initialization
> On Jan 11, 2017, at 5:34 AM, Adam Petcher wrote: > > Please review the following bug fix: > > http://cr.openjdk.java.net/~apetcher/8168075/webrev.00/ > > This fixes a bug in which a permission check would try to load resources > while the system class loader is being initialized. Resources cannot be > loaded at this time, so this change ensures that the resources are loaded > earlier. This seems a reasonable approach. There is a slight overhead for this change when the resource bundle is never used but it’s not an issue since the initialization of a security manager is not cheap anyway. It may be helpful to add a check to ensure that the resource bundle is loaded either VM.isBooted() or initLevel != 3. It would make it clear of this circular bootstrapping issue. You do this in SecurityManager class initialization. Have you considered doing it in the SecurityManager constructor? As Claes clarified, SM is loaded but not initialized during early VM startup. So no issue to do it in clinit. The alternative is to do it in the constructor as lazy initialization. ResourcesMgr.java Is the doPrivileged necessary? It’s old code written long ago. Perhaps we should file a bug to clean this up later. Otherwise, looks good. Mandy
Re: RFR: 8055206: Update SecurityManager::checkPackageAccess to restrict non-exported JDK packages by default
> On Jan 9, 2017, at 11:25 AM, Sean Mullan wrote: > > Please review this JDK 9 change to make the > SecurityManager::checkPackageAccess and checkPackageDefinition > implementations restrict access to the same set of internal JDK packages as > the module system. > > This overall change will improve security by making these two mechanisms > consistent and reduce the amount of work needed to maintain the > package.access and package.definition security properties going forward. > > JBS issue: https://bugs.openjdk.java.net/browse/JDK-8055206 > JDK webrev: http://cr.openjdk.java.net/~mullan/webrevs/8055206/jdk/webrev.00/ > JAXP webrev: > http://cr.openjdk.java.net/~mullan/webrevs/8055206/jaxp/webrev.00/ > This looks quite good. It’s good that the restricted package list for the system modules is no longer modifiable. This change may impact existing applications that are running with security manager and uses a JDK internal API from the runtime that was not in the package.access list. It may require the application to grant additional “accessClassInPackage.” permissions in addition to using `—-add-exports` to break into encapsulation. Similar to the feedback I suggest for JDK-8168075. We can move this initialization to the holder class and trigger the initialization in the SecurityManager constructor. > : > Several tests also had to be modified to be granted additional permission(s) > to access the newly restricted packages under a SecurityManager. JAXP also > needed a change to grant additional permissions to access internal packages > that are exported to the modules that are dynamically created for use with > XSLT. We will have to see if any dynamic generated bytecode in the field will need access to internal API like XSLT case. It seems something not right for a module that has access to an internal package in another module via qualified exports is required to have “accessClassInPackage.” runtime permission. I believe the “accessClassInPackage.” permission is checked when a deprivileged module loads an internal class in a module defined by the boot loader even if the package of that internal class is exported to it to access. I do think we should follow up and look into the potential solutions to respect the qualified exports. In addition, we should file an issue to look into any possibility to respect `—-add-exports` if specified (perhaps automatically add an entry in the default policy?). I’m okay to follow up these with separate JBS issues. Otherwise looks good. Mandy
Re: RFR 8168075: Custom system class loader + security manager + malformed policy file = recursive initialization
> On Jan 13, 2017, at 7:30 AM, Sean Mullan wrote: > > On 1/12/17 3:53 PM, Mandy Chung wrote: >> >>> On Jan 11, 2017, at 5:34 AM, Adam Petcher >>> wrote: >>> >>> Please review the following bug fix: >>> >>> http://cr.openjdk.java.net/~apetcher/8168075/webrev.00/ >>> >>> This fixes a bug in which a permission check would try to load >>> resources while the system class loader is being initialized. >>> Resources cannot be loaded at this time, so this change ensures >>> that the resources are loaded earlier. >> >> This seems a reasonable approach. There is a slight overhead for >> this change when the resource bundle is never used but it’s not an >> issue since the initialization of a security manager is not cheap >> anyway. >> >> It may be helpful to add a check to ensure that the resource bundle >> is loaded either VM.isBooted() or initLevel != 3. It would make it >> clear of this circular bootstrapping issue. >> >> You do this in SecurityManager class initialization. Have you >> considered doing it in the SecurityManager constructor? As Claes >> clarified, SM is loaded but not initialized during early VM startup. >> So no issue to do it in clinit. The alternative is to do it in the >> constructor as lazy initialization. > > I don't see any particular performance advantage of moving it to the > constructor. The SM will be initialized and instantiated in phase 3 before it > initializes the system classloader. Did you have another concern in mind? > No. There isn’t any performance difference since such code is invoked when a security manager is created. I’m okay to push this as is. Perhaps you can look into it as part of JDK-8055206 since JDK-8055206 has the initialization code too. Mandy
Re: RFR: 8037325: Class.getConstructor() performance regression
> On Jan 12, 2017, at 6:48 AM, Claes Redestad wrote:
>
> Hi,
>
> please review this fix to various performance regressions observed
> as the security model has evolved over the years.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8037325
> Webrev: http://cr.openjdk.java.net/~redestad/8037325/webrev.01
>
Looks good.
Nit: methodName method returns the string representation of the method
signature and so more than the method name. Maybe it should call
“methodToString”? The argumentTypesToString method is only used to print the
method signature. You could merge these two methods if you like.
ReflectUtil.java
262 public static boolean isNonPublicProxyClass(Class cls) {
263 String pkg;
264 return Proxy.isProxyClass(cls) &&
265 ((pkg = cls.getPackageName()) == null ||
!pkg.startsWith(PROXY_PACKAGE));
266 }
Nit: just a personal preference: move Proxy.isProxyClass(cls) check in a
separate if-statement and the declaration pkg can be moved with the assignment.
Mandy
Re: RFR: 8037325: Class.getConstructor() performance regression
> On Jan 16, 2017, at 1:59 PM, Claes Redestad wrote: > > http://cr.openjdk.java.net/~redestad/8037325/webrev.02/ +1 Mandy
Re: RFR: 8055206: Update SecurityManager::checkPackageAccess to restrict non-exported JDK packages by default
> On Jan 18, 2017, at 1:23 PM, Sean Mullan wrote: > >> Similar to the feedback I suggest for JDK-8168075. We can move this >> initialization to the holder class and trigger the initialization in >> the SecurityManager constructor. > > For now, I would prefer to leave it as is. In an earlier revision, I > experimented with delaying the initialization of nonExportedPkgs until > checkPackageAccess was called and ran into some circular permission checking > issues. It could be moving it to the ctor is ok, but since I have every test > passing right now, I'd rather not tinker with it :) Okay with me. > >>> : Several tests also had to be modified to be granted additional >>> permission(s) to access the newly restricted packages under a >>> SecurityManager. JAXP also needed a change to grant additional >>> permissions to access internal packages that are exported to the >>> modules that are dynamically created for use with XSLT. >> >> We will have to see if any dynamic generated bytecode in the field >> will need access to internal API like XSLT case. > > Yes. I have checked several other use cases in the JDK that create dynamic > modules (nashorn, rmi, jmx) and not found any issues. Thanks for the confirmation. Mandy
Review Request: JDK-8173024 Replace direct use of AuthResources resource bundle from jdk.security.auth
Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173024/webrev.00/ sun.security.util.ResourcesMgr::getString(String s, String altBundleName) is one existing mechanism to get the localized string from AuthResources and have sun.security.util.AuthResources resource bundle encapsulated in java.base. jdk.security.auth loads “sun.security.util.AuthResources” resource bundle in some place and uses ResourcesMgr::getString as well. This patch replaces direct loading of AuthResources resource bundle from jdk.security.auth so that jdk.security.auth does not need to access the resource bundle directly. I ran all core tests. Mandy
Re: RFR 8172527: Rename jdk.crypto.token to jdk.crypto.cryptoki
> On Jan 19, 2017, at 11:39 AM, Anthony Scarpino > wrote: > > Hi, > > I need a review to rename the jdk.crypto.token to jdk.crypto.cryptoki. This > is to change what 8171202 had done to the original jdk.crypto.pkcs11 module. > For those not familiar with discussions elsewhere, the term "token" is > confusing and unclear as it can mean many things cryptographically. The use > of cryptoki is more accurate to the original 'pkcs11' name and still > conforming to the new naming standard. > > http://cr.openjdk.java.net/~ascarpino/8172527/webrev-root/ > http://cr.openjdk.java.net/~ascarpino/8172527/webrev-jdk/ The webrev shows deleted/new files for src/jdk.crypto.token source files rather than rename. Did you use hg rename? Changes in other files looks good. Mandy
Re: RFR 8172527: Rename jdk.crypto.token to jdk.crypto.cryptoki
> On Jan 20, 2017, at 10:22 AM, Anthony Scarpino > wrote: > > Good catch.. that'll teach me for trusting the graphical tool to rename a > directory when I used 'Rename'. > > Also I found Brad's issue as it was a new changeset that just showed up in > that file. > > I updated the webrev, which looks better now: > http://cr.openjdk.java.net/~ascarpino/8172527/webrev-jdk.01/ +1 Mandy
Re: Review Request: JDK-8173024 Replace direct use of AuthResources resource bundle from jdk.security.auth
Since AuthResources is the only altBundle, Max suggests to replace getString(String, String) with a method for AuthResources bundle specifically. It’s an alternative I considered too. Here is the revised webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173024/webrev.01/ Mandy > On Jan 18, 2017, at 8:10 PM, Mandy Chung wrote: > > Webrev at: > http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173024/webrev.00/ > > sun.security.util.ResourcesMgr::getString(String s, String altBundleName) is > one existing mechanism to get the localized string from AuthResources and > have sun.security.util.AuthResources resource bundle encapsulated in > java.base. > > jdk.security.auth loads “sun.security.util.AuthResources” resource bundle in > some place and uses ResourcesMgr::getString as well. > > This patch replaces direct loading of AuthResources resource bundle from > jdk.security.auth so that jdk.security.auth does not need to access the > resource bundle directly. > > I ran all core tests. > > Mandy
Re: Review Request: JDK-8173024 Replace direct use of AuthResources resource bundle from jdk.security.auth
AFAIK, no permission check from RB::getBundle loading this resource bundle. The implementation should wrap all security sensitive calls with doPriv. I also mentioned that in [1] I have a simple test that calls new X500Principal(null) and it runs fine with security manager. Mandy [1] http://mail.openjdk.java.net/pipermail/security-dev/2017-January/015436.html > On Jan 21, 2017, at 5:02 PM, Weijun Wang wrote: > > Why isn't the new getAuthResourceString() using AccessController.doPrivileged > anymore? > > Thanks > Max > > On 01/22/2017 05:55 AM, Mandy Chung wrote: >> Since AuthResources is the only altBundle, Max suggests to replace >> getString(String, String) with a method for AuthResources bundle >> specifically. It’s an alternative I considered too. Here is the revised >> webrev: >> >> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173024/webrev.01/ >> >> Mandy >> >>> On Jan 18, 2017, at 8:10 PM, Mandy Chung wrote: >>> >>> Webrev at: >>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173024/webrev.00/ >>> >>> sun.security.util.ResourcesMgr::getString(String s, String altBundleName) >>> is one existing mechanism to get the localized string from AuthResources >>> and have sun.security.util.AuthResources resource bundle encapsulated in >>> java.base. >>> >>> jdk.security.auth loads “sun.security.util.AuthResources” resource bundle >>> in some place and uses ResourcesMgr::getString as well. >>> >>> This patch replaces direct loading of AuthResources resource bundle from >>> jdk.security.auth so that jdk.security.auth does not need to access the >>> resource bundle directly. >>> >>> I ran all core tests. >>> >>> Mandy >>
Re: Review Request: JDK-8173024 Replace direct use of AuthResources resource bundle from jdk.security.auth
> On Jan 21, 2017, at 6:37 PM, Weijun Wang wrote: > > > > On 01/22/2017 09:18 AM, Mandy Chung wrote: >> AFAIK, no permission check from RB::getBundle loading this resource bundle. >> The implementation should wrap all security sensitive calls with doPriv. I >> also mentioned that in [1] > > I see. > > It just feels strange to see getString() and getAuthResourcesString() > implemented so differently in this webrev. Since you think they should be the > same, how about creating a private method that includes the VM.initLevel and > bundles.computeIfAbsent calls? We'll let Adam to decide if getString() can > also call it. > I agree it looks strange but I hope Adam can leverage that. It’s better to leave it for the fix for JDK-8168075. Do you approve this fix? Mandy
Re: Review Request: JDK-8173024 Replace direct use of AuthResources resource bundle from jdk.security.auth
> On Jan 23, 2017, at 6:59 AM, Adam Petcher wrote: > > Comments below. > > On 1/21/2017 11:02 PM, Mandy Chung wrote: >>> On Jan 21, 2017, at 6:37 PM, Weijun Wang >>> <mailto:weijun.w...@oracle.com> wrote: >>> >>> >>> >>> On 01/22/2017 09:18 AM, Mandy Chung wrote: >>>> AFAIK, no permission check from RB::getBundle loading this resource >>>> bundle. The implementation should wrap all security sensitive calls with >>>> doPriv. I also mentioned that in [1] >>> I see. >>> >>> It just feels strange to see getString() and getAuthResourcesString() >>> implemented so differently in this webrev. Since you think they should be >>> the same, how about creating a private method that includes the >>> VM.initLevel and bundles.computeIfAbsent calls? We'll let Adam to decide if >>> getString() can also call it. >>> >> I agree it looks strange but I hope Adam can leverage that. It’s better to >> leave it for the fix for JDK-8168075. > > Thanks. I've updated JDK-8172808 > <https://bugs.openjdk.java.net/browse/JDK-8172808> to indicate that there is > some potential for refactoring here. > > Though it seems like there is an issue with ResourceMgr::getString in your > latest diff. The bundle is loaded, but it is not stored in the map (unless > I'm missing it). So the resource bundle would be loaded for every call to > this method. Good catch. I missed that. The good things is that ResourceBundle is cached after it’s loaded via getBundle method and there should be no loss of performance. I’ll fix it and should probably take JDK-8172808. Mandy
Re: RFR 8168075: Custom system class loader + security manager + malformed policy file = recursive initialization
> On Jan 19, 2017, at 7:28 AM, Adam Petcher wrote: > > My last attempt to solve this problem didn't work because some classes needed > for string formatting were not loaded by init level 3 in some cases. So I had > to backtrack and try a different approach. > > This patch avoids localization and message formatting when the VM is not > booted. In this case, non-localized messages are printed, and simplified > message formatting code is used. Once the VM is loaded, messages are > localized and formatted in the usual way. > > http://cr.openjdk.java.net/~apetcher/8168075/webrev.01/ This change looks fine. This will make sure that MessageFormat will be used only after VM is booted. MessageFormatting.java - missing @bug Nit: test/sun/security/util/Resources/ClassLoader - perhaps renaming “ClassLoader” directory to “customSysLoader” to make it clear. Mandy
Re: RFR 8168075: Custom system class loader + security manager + malformed policy file = recursive initialization
> On Jan 24, 2017, at 11:48 AM, Adam Petcher wrote: > > > On 1/24/2017 2:17 PM, Sean Mullan wrote: >> >> It looks like there is a leftover import for PolicyUtil in >> PolicyParser.java. Otherwise looks good and I can push this fix for you >> after you send me an updated patch/webrev. >> >> --Sean > > Fixed in the latest webrev: > http://cr.openjdk.java.net/~apetcher/8168075/webrev.03/ > I also added the missing @test tag that Mandy requested and I neglected to > include in webrev.02. +1 Mandy
Review Request JDK-8172808: Handle sun.security.util.Resources bundle in ResourcesMgr in the same way as AuthResources
Adam, Sean, Can you review this simple fix: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8172808/webrev.00/ This change updates ResourcesMgr to handle both Resources and AuthResources consistently. This removes doPrivileged block because it is not really needed. This is covered by an existing test, jdk/test/sun/security/util/Resources/Format.java. I ran all core tests to verify as well. Mandy
Re: Review Request JDK-8172808: Handle sun.security.util.Resources bundle in ResourcesMgr in the same way as AuthResources
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8172808/webrev.01/ This includes a simple patch from Adam to fixup the copyright headers in the fix for JDK-8168075. Mandy > On Jan 24, 2017, at 2:25 PM, Mandy Chung wrote: > > Adam, Sean, > > Can you review this simple fix: > http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8172808/webrev.00/ > > This change updates ResourcesMgr to handle both Resources > and AuthResources consistently. This removes doPrivileged > block because it is not really needed. This is covered by > an existing test, jdk/test/sun/security/util/Resources/Format.java. > > I ran all core tests to verify as well. > > Mandy >
Re: RFR: 8145337: [JVMCI] JVMCI initialization with SecurityManager installed fails: java.security.AccessControlException: access denied
(dropping jdk9-dev. security-libs is the appropriate list to review security permission) > On Jan 23, 2017, at 1:56 PM, Doug Simon wrote: > > Both jdk.vm.ci and jdk.vm.compiler require a number of permissions when a > security manager is present. Since neither of these modules is accessible to > application code, it should be ok to give them all permissions. This seems to > be the approach for a number of other modules including > jdk.scripting.nashorn, jdk.dynalink, jdk.jsobject etc. Please review this > small change that configures this proposed permission level. > > http://cr.openjdk.java.net/~dnsimon/8145337/webrev/ > https://bugs.openjdk.java.net/browse/JDK-8145337 jdk.vm.compiler is defined by the application class loader and it’s used by AOT tool. I wonder why it has to run with security manager. You can reference JDK tools such as jdk.compiler and jdk.jlink that are not granted with any permission. Mandy
Re: RFR: 8145337: [JVMCI] JVMCI initialization with SecurityManager installed fails: java.security.AccessControlException: access denied
> On Jan 26, 2017, at 3:13 AM, Doug Simon wrote:
>
>>
>> jdk.vm.compiler is defined by the application class loader and it’s used by
>> AOT tool. I wonder why it has to run with security manager.
>
> Without java.security.AllPermission, the policy for jdk.vm.compiler required
> to get through a bootstrap (i.e., java -server
> -XX:+UnlockExperimentalVMOptions -Djava.security.manager -XX:+BootstrapJVMCI
> -XX:+UseJVMCICompiler -version) is show below (annotated with comments
> denoting the methods requiring the permissions):
>
> :
Are -XX:+BootstrapJVMCI -XX:+UseJVMCICompiler supported to use at runtime?
> There’s no guarantee that this is all the permissions required since not all
> code paths are exercised during bootstrap.
>
>> You can reference JDK tools such as jdk.compiler and jdk.jlink that are not
>> granted with any permission.
>
> Neither of those tools create code and install it in the VM. I don’t think a
> fine grained SecurityManager policy makes sense for a VM compiler since it
> could subvert security by compiling/installing malicious code. That is, a VM
> compiler has to be a trusted component. Keep in mind that user code cannot
> get to jdk.vm.compiler.
My question is not about granting fine-grained permissions vs AllPermissions.
I expect jdk.vm.compiler is used with jdk.aot which does not run with security
manager.
If jdk.vm.compiler is run with VM as JIT and with security manager, the user
can set -Djava.security.policy to a security policy configuring the permission
for jdk.vm.compiler.
grant codeBase "jrt:/jdk.vm.compiler" {
permission java.security.AllPermission;
};
If -XX:+BootstrapJVMCI -XX:+UseJVMCICompiler are supported, the other question
I have is which loader jdk.vm.compiler should be defined?
Mandy
Re: RFR: 8145337: [JVMCI] JVMCI initialization with SecurityManager installed fails: java.security.AccessControlException: access denied
> On Jan 30, 2017, at 10:38 AM, Doug Simon wrote: > > I’ve extended the webrev with that change - please re-review: > > http://cr.openjdk.java.net/~dnsimon/8145337_make/webrev > +1 > Strangely, there was no existing declaration of jdk.vm.compiler in > Modules.gmk. > Default is to be defined by the application class loader. The build will find all modules from the source. There is no need to list all modules. > BTW, I never answered your question: > > "How does JVMCI call out to jdk.vm.compiler? does it load classes using > Class::forName with the system class loader?” > > It uses JVMCIServiceLocator[1] which is a mechanism built on the standard > ServiceLoader. Thanks for the pointer. That confirms my understanding that loads the service providers using the system class loader. Mandy
Re: RFR: 8145337: [JVMCI] JVMCI initialization with SecurityManager installed fails: java.security.AccessControlException: access denied
> On Jan 30, 2017, at 1:36 PM, Doug Simon wrote: > > >> On 30 Jan 2017, at 21:55, Mandy Chung wrote: >> >> >>> On Jan 30, 2017, at 10:38 AM, Doug Simon wrote: >>> >>> I’ve extended the webrev with that change - please re-review: >>> >>> http://cr.openjdk.java.net/~dnsimon/8145337_make/webrev >>> >> >> +1 > > Thanks. Is that a “Reviewed”? > Sorry. I only noticed now that you added this to UPGRADEABLE_MODULE. Please add it only to PLATFORM_MODULES list instead. Making it an upgradeable module is a separate issue. I suggest you reopen JDK-8171448. Specifically, since upgradeable modules are not tied with java.base, our goal for JDK 9 is to eliminate qualified exports from JDK modules to upgradeable modules, e.g. JDK-8170116, JDK-8166745, JDK-8161549. > I think I should get at least one sign-off from the security team. > Hope Sean will review this one. Please send an updated webrev. > Also, since this is effectively making jdk.vm.compiler an upgradeable module, No it does not. > what’s the implication for it being a hash-checked module? When a module M is recorded in the ModuleHashes attribute of java.base, the runtime will check if module M resolved in the graph matches the one tied with java.base when created at build time; if not, it will fail. If an upgradeable module > It seems like these changes effectively achieve what I was requesting with > https://bugs.openjdk.java.net/browse/JDK-8171448. JDK-8145337 is about the security permission. It’s better to separate this review from JDK-8171448. Mandy > > -Doug > >> >>> Strangely, there was no existing declaration of jdk.vm.compiler in >>> Modules.gmk. >>> >> >> Default is to be defined by the application class loader. The build will >> find all modules from the source. There is no need to list all modules. >> >>> BTW, I never answered your question: >>> >>> "How does JVMCI call out to jdk.vm.compiler? does it load classes using >>> Class::forName with the system class loader?” >>> >>> It uses JVMCIServiceLocator[1] which is a mechanism built on the standard >>> ServiceLoader. >> >> Thanks for the pointer. That confirms my understanding that loads the >> service providers using the system class loader. >> >> Mandy >
Re: RFR: 8145337: [JVMCI] JVMCI initialization with SecurityManager installed fails: java.security.AccessControlException: access denied
> On Feb 1, 2017, at 3:07 AM, Doug Simon wrote: > > I’ve reworked the webrev as requested to make jdk.vm.compiler a > non-upgradeable platform module, this allowing it to be mentioned in > default.policy: > > http://cr.openjdk.java.net/~dnsimon/8145337/ Looks good. Mandy
Re: RFR: 8179370: Replace use of , and tags in java.base
Looks okay. Mandy > On Apr 26, 2017, at 5:50 PM, Jonathan Gibbons > wrote: > > Please review these mostly simple changes to replace HTML tags which are not > valid in HTML 5 in public doc comments in java.base. > > As with the previous changes, the changes were done mechanically, using the > following sed script: > > s|||g > s|||g > s|<\\/tt>|<\\/code>|g > s|\(]*\)>\([^<]*\)|\1 style="text-align:center">\2| > s| | > s||| > s| s||| > > The unusual form of the 3rd line was to cover the occurrence in a makefile. > > The 4th line is specific for DataInput.java, and replaces within a > with a style on the element itself. > > The 5th and 6th lines are specific to URLConnection. The use of the table > itself and the ASCII art that follows it are questionable, but the intent > here is just to update the HTML and not to rework the visual appearance of > the generated documentation. > > The changes cover files in the following packages: > > java.base java/io > java.base java/net > java.base java/util > java.base javax/net/ssl > > > JBS: https://bugs.openjdk.java.net/browse/JDK-8179370 > Webrev: http://cr.openjdk.java.net/~jjg/8179370/webrev/ > > -- Jon
Re: RFR: 8179370: Replace use of , and tags in java.base
> On Apr 26, 2017, at 6:49 PM, Jonathan Gibbons
> wrote:
>
> Updated webrev to address Joe's suggestion to try harder to use {@code} as a
> substitute for .
>
> http://cr.openjdk.java.net/~jjg/8179370/webrev.01
Looks good.
Mandy
Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp
Looks like this test execs a new VM for 66 times to exhaust the combination of with and without security manager, with a valid or malformed policy, client & custom loader in unnamed, named, automatic module. I think we could take out the automatic module case as named module would verify it. One refactor you can consider by separating them in several @run actions. The timeout will apply to each action but that does not help shorten the test execution time. Mandy > On May 1, 2017, at 11:59 AM, Brent Christian > wrote: > > Hi, Joe > > The typical run time of this test is ~18s (on my modestly-equipped laptop). > So the test does run within a reasonable amount of time, IMO - under normal > circumstances, anyway. > > The increased timeout is to cover the less seldom test run configurations > involving Xcomp. Judging by when the test was added and the dates of the > failed runs, I think this test has timed out since it was introduced, and > there has not been an Xcomp regression. > > The test checks for a recursive initialization issue (8168075), and so needs > to launch JVMs using various combinations of system classloader/security > manager+policy/various module types. For this reason I think Xcomp hits this > test particularly hard, recompiling the startup code on every VM launch. I > think we want to maintain thorough test coverage here, though. > > Looking more into the failures, the worst case looks to be getting not quite > halfway through the test. We should be able to get away with a more modest > increase to the timeout (600, instead of 1200), and still have the test pass, > if you would prefer. > > Thanks, > -Brent > > On 4/27/17 2:25 PM, joe darcy wrote: >> I understand the interest in having test pass reliably, but I don't >> think giving the test very large timeouts is the preferred way of >> accomplishing that. >> >> For all configurations, the test can now run for up to 20 minutes, up >> from 4 minutes. We want to run the entire test suite, thousands of >> tests, in about 20 minutes. The the timeout factor used for Xcomp run, >> the test would probably now be able to run for over an hour before >> timing out. >> >> I suggest making the test run faster, or seeing if there has been a >> regressions in Xcomp to make test perform more poorly there. >> >> Thanks, >> >> -Joe >> >> >> On 4/27/2017 12:08 PM, Brent Christian wrote: >>> Hi, >>> >>> This test times out under our automated testing configurations that >>> include -Xcomp. >>> >>> Please review my change to increase the timeout for this test. It is >>> sufficient for the test configurations in question to pass on two >>> different local machines (Mac & Linux). >>> >>> diff -r 7c04ab31b4d6 >>> test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java >>> --- a/test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java >>> Wed Apr 26 09:37:23 2017 -0700 >>> +++ b/test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java >>> Thu Apr 27 12:03:33 2017 -0700 >>> @@ -29,7 +29,7 @@ >>> * @library /lib/testlibrary >>> * @modules java.base/jdk.internal.module >>> * @build JarUtils CompilerUtils >>> - * @run main/timeout=240 ClassLoaderTest >>> + * @run main/timeout=1200 ClassLoaderTest >>> */ >>> >>> Thanks, >>> -Brent >>
Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp
> On May 1, 2017, at 12:47 PM, Brent Christian > wrote: > > >> One refactor you can consider by separating >> them in several @run actions. The timeout will apply to each action >> but that does not help shorten the test execution time. > > I had the same thought, and concluded that without a reduction in overall > execution time, the benefit of such a refactor is pretty limited. I think it would make the test easier to read and understand the cases it cover. Mandy
Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp
> On May 9, 2017, at 4:23 PM, Brent Christian > > I've removed the test case for automatic modules, and added a @run action for > each policy file + system classloader configuration. I also removed the code > to compile test sources, using @build instead. > > I also made some (hopefully) clarifying changes to the code. > > The String[] argument to execute() is now separated into args/status/message, > instead of being lumped into one String[], and then having to be split() out > of a single String. Where execute() is called, it's easier to read the args > in order (versus assembled using String.format()). > > I also simplified the code for expected status/output, and made some path > variable names more descriptive. > > Webrev: > http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/ Thanks for the clean up. Does each @run action need 4 mins timeout? Can it restore to the default timeout? Mandy
Re: RFR: [Updated] Update tables in java.base to be HTML5-friendly.
> On May 5, 2017, at 3:52 PM, Jonathan Gibbons > wrote: > > This is an updated review for the changes to improve tables in java.base. > : > Webrevs: > > langtools (the stylesheet): > http://cr.openjdk.java.net/~jjg/8179479-8179592/8179479/webrev.01/ > > jdk (changes to java.base): > http://cr.openjdk.java.net/~jjg/8179479-8179592/8179592/webrev.01/ > > API showing the combined effect of these cahnges: > http://cr.openjdk.java.net/~jjg/8179479-8179592/api.01/java.base-summary.html > The new style class names are better. I reviewed the javadoc changes in java.base. Mostly looks good. A few table without an explicit class and FileSystemProvider should use striped class table. src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java - * + * src/java.base/share/classes/java/nio/charset/Charset.java + * + * + * + * + *
Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp
> On May 11, 2017, at 3:25 PM, Brent Christian
> wrote:
>
> Hi,
>
> I have one more update, with a couple of suggested changes to simplify the
> execute() calls:
>
> * execute() takes a vararg, so explicit String[] creation can be omitted
> (mostly).
>
> * args common to every execute() call are consolidated into a List. (The
> resulting arg reordering should not affect test execution.)
>
> http://cr.openjdk.java.net/~bchristi/8177328/webrev.02/
This looks much cleaner. Thanks for the change.
Minor comment:
95 private final List COMMON_ARGS;
This is an instance field and you can use lower case as the convention.
238 return !s.equals("");
You can consider using String::isEmpty.
No need for an updated webrev.
Thanks
Mandy
Re: SecurityManager.checkPackageAccess for qualified exports
> On May 12, 2017, at 8:01 AM, Sean Mullan wrote: > > On 5/12/17 9:14 AM, Langer, Christoph wrote: >> >> I think the package access check walking down the whole stack is fine and >> should be done here, not just the module access check. One thing to mention is that the application class loader’s loadClass method calls SecurityManager::checkPackageAccess which is stack-based permission check. When a class is being loaded by the application class loader, package access permission is checked. >> However, frames originating out of a module that the package was exported to >> should have the permission to access the package. Such that when I would run >> in a privileged section there, I would get package access. And if I wouldn't >> run privileged then all the calling frames would be checked and the check >> might not be passed. Wouldn't that be the right way? > > Yes, I think something like this is worth considering but needs to be > prototyped and carefully reviewed before we would be able to consider it. I > can file an issue to track this, but in my opinion it is too late for JDK 9. We discussed this and agree to improve it in a future release. In particular when a module M1 exports qualifiedly to M2, it’d be good to skip that the package access check. Mandy
Re: RFR [9]: 8181295: Document that SecurityManager::checkPackageAccess may be called by the VM
> On Jun 16, 2017, at 8:00 AM, Sean Mullan wrote: > > Please review this clarification to the SecurityManager::checkPackageAccess > method to note that the method may be called by the Virtual Machine when > loading classes: > > http://cr.openjdk.java.net/~mullan/webrevs/8181295/webrev.00/ > > A small correction was also made to the checkPackageDefinition method to note > that it may be called by the defineClass (and not the loadClass) method of > class loaders. checkPackageDefinition is always a question for me and it’s not called in the JDK implementation. Is there any test verifying that (i.e. called from defineClass)? I’m okay to change “is” to “may” in checkPackageDefinition in this patch. I can’t validate this spec change. I suggest to separate this from JDK-8181295 and follow up in a future release. Mandy
Re: RFR [9]: 8181295: Document that SecurityManager::checkPackageAccess may be called by the VM
> On Jun 16, 2017, at 1:25 PM, Sean Mullan wrote: > > On 6/16/17 11:13 AM, Mandy Chung wrote: >>> On Jun 16, 2017, at 8:00 AM, Sean Mullan wrote: >>> >>> Please review this clarification to the SecurityManager::checkPackageAccess >>> method to note that the method may be called by the Virtual Machine when >>> loading classes: >>> >>> http://cr.openjdk.java.net/~mullan/webrevs/8181295/webrev.00/ >>> >>> A small correction was also made to the checkPackageDefinition method to >>> note that it may be called by the defineClass (and not the loadClass) >>> method of class loaders. >> checkPackageDefinition is always a question for me and it’s not called in >> the JDK implementation. Is there any test verifying that (i.e. called from >> defineClass)? >> I’m okay to change “is” to “may” in checkPackageDefinition in this patch. I >> can’t validate this spec change. I suggest to separate this from >> JDK-8181295 and follow up in a future release. > > Ok, that's fine. Instead of changing the wording, I would prefer to revert > the change to checkPackageDefinition and file a new issue to address that > separately in a subsequent release as it is not as critical and not > specifically related to this issue. That’s fine with me. Approved. I don’t need an updated webrev. Mandy
Re: RFR 8182118: Package summary is missing in jdk.security.auth module
> On Jun 18, 2017, at 8:33 PM, Weijun Wang wrote:
>
> Hi All
>
> Please take a review at
>
> http://cr.openjdk.java.net/~weijun/8182118/webrev.00/
>
> Basically, a description line is added into package-info.java of each of
> these packages:
>
> - com/sun/security/auth:
>
> Contains the implementation of {@code java.security.Principal}.
>
> - com/sun/security/auth/module:
>
> Contains the implementation of {@code javax.security.auth.spi.LoginModule}.
>
> - com/sun/security/auth/login:
>
> Contains the implementation of {@code
> javax.security.auth.login.Configuration}.
>
> - com/sun/security/auth/callback:
>
> Contains the implementation of {@code javax.security.auth.callback.Callback}.
>
What about “Provides the implementation of ….”
I suggest to use @link to the type.
> with @since 1.4.
>
> I thought about using {@link java.security.Principal} but seems it's not
> supported in package-info.java.
java/lang/package-info.java and many package summary use @link.
>
> BTW, is this bug meant for JDK 9? I just read the mail from Mark saying only
> P1 fixes will be allowed from now on.
If you push it your Monday, you should be able to make jdk-9+175 integration
(6/22 GAC). Otherwise P1 fixes only.
Mandy
Re: RFR 8182118: Package summary is missing in jdk.security.auth module
Looks fine to me. Mandy > On Jun 19, 2017, at 6:17 AM, Weijun Wang wrote: > > Updated at http://cr.openjdk.java.net/~weijun/8182118/webrev.02/. > > --Max > > On 06/19/2017 08:23 PM, Sean Mullan wrote: >> On 6/19/17 8:17 AM, Weijun Wang wrote: There is more than one, so this should be "implementations". >>> >>> In fact, I originally used "implementations" (without "the") and "an >>> implementation", but then I saw the module-info.java for the module saying >>> "Contains the implementation of the javax.security.auth.* interfaces" and >>> thought "the implementation" is always correct. >> I don't see where it uses the word "Contains". >> I would probably just tweak that to say "Provides implementations of the >> javax.security.auth.* interfaces and various authentication modules." >> This would make the wording more consistent in the module and packages. >> --Sean
Re: RFR: JDK-8185758: jdk.smartcardio has broken docs for exceptions
> On Aug 2, 2017, at 4:21 PM, Jonathan Gibbons
> wrote:
>
> (I'm not sure if there is a better list for this request, but the request is
> so simple, I'm hoping it will be sufficient.
>
> Please review this very simple fix to replace two uses of ...
> with {@code...}.
>
> The underlying problem being fixed is incorrect handling of the first
> sentence in a couple of cases, due to the presence of a '?' in the code
> expression. The doclet was treating the ? as terminating the first sentence,
> leading to confusing output, and incorrect HTML being generated.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8185758
> Webrev: http://cr.openjdk.java.net/~jjg/8185758/webrev.00/
Looks good.
Mandy
Re: RFR 8184744 : Replace finalizer in crypto classes with Cleaner
Thanks for doing this. It’s good to see JDK finalizes being replaced. It looks good to me. I wonder if there is any existing security utility class that may be a good place for this zero-ing byte array method to share. Sean and other may have opinion. Mandy > On Aug 3, 2017, at 1:01 PM, Roger Riggs wrote: > > Found a race in the tests between the Cleaner clearing the arrays and the > test waiting for the key to be freed. > Revised to continuously check the arrays and exit when they are cleared. If > not cleared the test will timeout. > > Updated the webrev: > http://cr.openjdk.java.net/~rriggs/webrev-crypt-finalize-8184744/ > > Thanks, Roger > > On 8/3/2017 9:58 AM, Roger Riggs wrote: >> Please review replacing finalize with java.lang.ref.Cleaner in a few simple >> cases. >> >> Webrev: >> http://cr.openjdk.java.net/~rriggs/webrev-crypt-finalize-8184744/ >> >> Thanks, Roger >> >> >
Re: RFR 8186931: jdk.security.jarsigner package is missing package summary
+1
Mandy
On 9/5/17 5:16 PM, Weijun Wang wrote:
The package info for com.sun.security.jgss in jdk.security.jgss is also
missing. The updated change looks like this (I omit the copyright header):
src/jdk.jartool/share/classes/jdk/security/jarsigner/package-info.java:
+/**
+ * This package defines APIs for signing jar files.
+ */
+package jdk.security.jarsigner;
src/jdk.jartool/share/classes/module-info.java:
+ * This module also defines APIs for signing JAR files.
src/jdk.security.jgss/share/classes/com/sun/security/jgss/package-info.java:
+/**
+ * This package defines classes and interfaces for the JDK extensions
+ * to the GSS-API.
+ */
package com.sun.security.jgss;
src/jdk.security.jgss/share/classes/module-info.java:
/**
- * Defines Java extensions to the GSS-API and an implementation of the SASL
+ * Defines JDK extensions to the GSS-API and an implementation of the SASL
* GSSAPI mechanism.
Thanks
Max
On Aug 31, 2017, at 11:10 PM, Sean Mullan wrote:
On 8/31/17 4:49 AM, Weijun Wang wrote:
/**
* This package contains the {@link jdk.security.jarsigner.JarSigner} API,
* which backs the signing function of the {@code jarsigner} tool.
*/
I think you should say something about that this API can be used to sign JAR
files. The fact that it is also used by jarsigner seems less important to
mention in the package description. I suggest changing this to:
"This package contains the {@link jdk.security.jarsigner.JarSigner} API which can be
used to sign jar files."
--Sean
Re: StackOverflowError - Java 9 Build 181
FYI. jdk.javaws is granted with AllPermissions in
conf/security/javaws.policy. Maybe javaws.policy is not augmented to
the security policy at runtime?
Mandy
On 9/20/17 12:45 PM, Sean Mullan wrote:
Tom,
Try adding the following lines to the lib/security/default.policy file
in your JDK installation:
grant codeBase "jrt:/jdk.javaws" {
permission java.security.AllPermission;
};
I have a hunch that permissions are not being granted to the
jdk.javaws module before it needs them. If that fixes the issue (or
you don't see it for a few days), I'll followup and file a bug.
Thanks,
Sean
On 9/19/17 5:55 PM, Tom Hood wrote:
No luck so far reproducing this problem. The two times it happened to
me yesterday have both been with Java 9 build 181 and the application
has been idle for awhile. I login to our application, execute various
features of the application, go to a meeting, return, and then see
the java console repeatedly displaying the stack overflow exception.
Maybe meetings are bad for Java 9? :-) I think there are some
background threads in our application that are waking up periodically
and doing "stuff". I don't know what that "stuff" is yet, but that
would be my guess at where I will find the code that triggered the
overflow.
Assuming I can get our application to the point where I can reproduce
the stack overflow, are there particular Java 9 builds that made
significant changes to security-relevant code that you'd like me to try?
Keep in mind that our app runs on a network not connected to the
internet. As it is, I manually typed in the stack trace, so if
there's a lot of output I'll have to print it and go through an
approval process to show it to you via a scanned pdf. I will
continue testing of our app with the security debug turned on so that
I'll have the output if it happens again. I also have the logging
and tracing enabled in the java control panel.
-- Tom
On Tue, Sep 19, 2017 at 12:13 PM, Sean Mullan > wrote:
Cross-posting to security-dev as this is more relevant to that list
and bcc-ing core-libs-dev.
I think this might be an issue with the JavaWebStart SecurityManager
not being granted the proper permissions. It is possible that the
deployment policy files are not being loaded or there is some other
subtle bootstrapping issue. It should not result in a recursive loop
of course, but there may be a workaround.
In the meantime, can you send me more information, preferably a test
case and a log file with -Djava.security.debug=all enabled? (The
latter will help analyze the recursion and see what security checks
are failing and for which ProtectionDomains). Also, have you tested
this on builds earlier than b181?
Thanks,
Sean
On 9/19/17 2:53 PM, Tom Hood wrote:
I should add that we have not modified or overridden any policy
files.
Also, we are not using a custom security manager.
On Tue, Sep 19, 2017 at 11:52 AM, Tom Hood mailto:tom.w.h...@gmail.com>> wrote:
Hi,
I hit an infinite recursion loop probably related to
PolicyFile that
exists in Java 9 build 181 for windows 64-bit. It might be
related to
JDK-8077418
>
I haven't tracked down what is causing our webstart app to
hit this
problem yet, but I thought I would let you know sooner than
later. Also,
it probably is not a problem for our particular application
as I should be
able to set the security manager to null which I think/hope
will bypass
this issue. I will try today to reproduce it in our app so
I can confirm
if setting security manager to null will work for us.
The stack looks like the following: (with many repeat stacks
omitted)
Exception in thread "AWT-EventQueue-2"
java.lang.StackOverflowError
at
java.base/java.security.AccessController.doPrivileged(Native
Method)
at
java.base/sun.security.provider.PolicyFile.getPermissions(Po
licyFile.java:1135)
at
java.base/sun.security.provider.PolicyFile.getPermissions(Po
licyFile.java:1082)
at
java.base/sun.security.provider.PolicyFile.implies(PolicyFil
e.java:1038)
at
java.base/java.security.provider.ProtectionDomain.implies(Pr
otectionDomain.java:323)
at
java.base/java.security.provider.ProtectionDomain.impliesWit
hAltFilePerm(ProtectionDomain.java:355)
at
java.base/java.security.provider.AccessControlContext.checkP
ermission(AccessControlContext.java:450)
at
java.base/j
Re: RFR: 8186535: Remove deprecated pre-1.2 SecurityManager methods and fields
On 11/22/17 6:37 AM, Sean Mullan wrote: Please review this change to remove the pre-JDK 1.2 SecurityManager methods that have been deprecated since JDK 1.2 and marked for removal in JDK 9. These methods are fragile, error-prone and have been obsolete since the SecurityManager was revamped in JDK 1.2. The methods to be removed are: getInCheck, classDepth, classLoaderDepth, currentClassLoader, currentLoadedClass, inClass, and inClassLoader. In addition, the deprecated and error-prone checkMemberAccess method (which was deprecated in JDK 8 and marked for removal in JDK 9) has been changed to throw SecurityException if the caller has not been granted AllPermission. This makes the method less likely it will be used incorrectly while still allowing some more time before it is removed. http://cr.openjdk.java.net/~mullan/webrevs/8186535/webrev.00/ src/java.desktop/share/classes/sun/applet/AppletSecurity.java 111 private static final StackWalker walker = 112 StackWalker.getInstance(RETAIN_CLASS_REFERENCE); This call will do a stack-based permission check. So it needs to be wrapped with doPrivileged. Otherwise, looks fine. Just to mention this: AppletSecurity does not really need the currentClassLoader method. AppletSecurity::currentAppletClassLoader could be reimplemented to use StackWalker to walk the stack once (replacing the call to currentClassLoader and getClassContext) to find AppletClassLoader. OTOH it does not worth making more change since applets are going away. Mandy
Re: RFR 8194251: Deadlock between UsageTracker and System.getProperty() when using a malformed security policy
This looks fine to me. Mandy On 2/5/18 11:26 AM, Adam Petcher wrote: Please review the following change: JBS: https://bugs.openjdk.java.net/browse/JDK-8194251 Webrev: http://cr.openjdk.java.net/~apetcher/8194251/webrev.00/ We ran into a problem related to loading locale data when a security policy file is malformed. The parse error is localized and printed, which requires the locale data to be loaded, which triggers a security policy check, which results in deadlock or infinite recursion. This change removes localization from all messages during policy file parsing and loading. I believe that this behavior is acceptable according to our localization requirements. I removed the code that tried to determine whether localization would succeed, because making it work reliably would be difficult. Now the client of LocalizedMessage will need to explicitly state whether the message should be localized or not.
Re: RFR: JDK-8200178 Remove mapfiles for JDK native libraries
This is a very good change and no more mapfile to maintain!!
Please do file JBS issues for the component teams to clean up their
exports.
Mandy
On 3/23/18 7:30 AM, Erik Joelsson wrote:
I have looked at the build changes and they look good.
Will you file followups for each component team to look over their
exported symbols, at least for the libraries with
$(EXPORT_ALL_SYMBOLS)? It sure looks like there is some technical debt
laying around here.
/Erik
On 2018-03-23 06:56, Magnus Ihse Bursie wrote:
With modern compilers, we can use compiler directives (such as
_attribute__((visibility("default"))), or __declspec(dllexport)) to
control symbol visibility, directly in the source code. This has
historically not been present on all compilers, so we had to resort
to using mapfiles (also known as linker scripts).
This is no longer the case. Now all compilers we use support symbol
visibility directives, in one form or another. We should start using
this. Since this has been the only way to control symbol visibility
on Windows, for most of the shared code, we already have proper
JNIEXPORT decorations in place.
If we fix the remaining platform-specific files to have proper
JNIEXPORT tagging, then we can finally get rid of mapfiles.
This fix removed mapfiles for all JDK libraries. It does not touch
hotspot libraries nor JDK executables; they will have to wait for a
future fix -- this was complex enough. This change will not have any
impact on macosx, since we do not use mapfiles there, but instead
export all symbols. (This is not a good idea, but I'll address that
separately.) This change will also have a minimal impact on Windows.
The only reason Windows is impacted at all, is that some changes
needed by Solaris and Linux were simpler to fix for all platforms.
I have strived for this change to have no impact on the actual
generated code. Unfortunately, this was not possible to fully
achieve. I do not believe that these changes will have any actual
impact on the product, though. I will present the differences more in
detail further down. Those who are not interested can probably skip
that.
The patch has passed tier1 testing and is currently running tier2 and
tier3. Since the running code is more or less (see caveat below)
unmodified, I don't expect any testing issues.
Bug: https://bugs.openjdk.java.net/browse/JDK-8200178
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8200178-remove-mapfiles/webrev.01
Details on changes:
Most of the source code changes are (unsurprisingly) in java.base and
java.desktop. Remaining changes are in jdk.crypto.ucrypto,
jdk.hotspot.agent, jdk.jdi and jdk.jdwp.agent.
Source code changes does almost to 100% consists in decorating an
exported function with JNIEXPORT. I have also followed the
long-standing convention of adding JNICALL. This is a no-op on
non-Windows platforms, so for most of the changes this is purely
cosmetic (and possibly adding in robustness, should the function ever
be used on Windows in the future). I have also followed the stylistic
convention of putting "JNIEXPORT JNICALL" on a separate
line. For some functions, however, this might cause a change in
calling convention on Windows. Since this can not apply to exported
functions on Windows (otherwise they would already have had
JNIEXPORT), I do not think this matters anything.
A few libraries did not have a mapfile, on Linux and/or Solaris. This
actually meant that all symbols were exported. It is highly unclear
if this was known and intended by the original make rule writer. I
have emulated this by adding the flag $(EXPORT_ALL_SYMBOLS) to these
libraries. Hopefully, we can remove this flag and fix proper exported
symbols in the future.
I have run the complete build using COMPARE_BUILD, and made a
thourough analysis of the differences for Linux and Solaris. All
native libraries have symbol differences, but most of them are
trivial and/or harmless. As a result, most libraries have disasm
differences as well, but these too seem trivial and harmless. The
differences in symbols that are common to all libraries include:
* Internal symbols such as __bss_start, _edata, _end and _fini are
now global. (They are imported as such from the compiler
libraries/archives, and we have no linker script to override this
behavior).
* The versioning tag SUNWprivate_1.1 is not included, and thus
neither the .gnu.version_d symbol.
* There are a few differences in the symbol and/or mangling of some
local functions. I'm not sure what's causing this,
but it's unlikely to have any effect on the product.
Another common source for change in symbols is due to previous
platform differences. For instance, if we had "JNIEXPORT int JNICALL
do_foo() { ... }", but do_foo was not in the mapfile, the symbol was
exported on Windows but not on Linux and Solaris. (Presumable since
it was not needed there, even though it was compiled for those
platforms as well.) Now, with t
Re: [11] RFR: 8193032: Remove terminally deprecated SecurityManager APIs
On 3/27/18 11:15 PM, Sean Mullan wrote: Please remove this change to remove several SecurityManager methods that have been marked for removal since Java SE 9: checkTopLevelWindow, checkSystemClipboardAccess, checkAwtEventQueueAccess, and checkMemberAccess. These methods no longer have any benefit, and removing them will follow through on the the plan to remove them. We believe the compatibility risk is fairly low - we have only found a very small number of custom SecurityManager implementations that are overriding the methods and using @Override or calling the methods directly that will need to change their code. See the CSR for more info. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8193032/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8200185 This looks good to me. Mandy
Re: RFR: 8202419: Avoid creating Permission constants early
On 4/30/18 8:25 PM, Claes Redestad wrote: Hi, moving a couple of local permission constants to sun.security.util.SecurityConstants ensures we delay and avoid loading permission classes very early during bootstrap. Delaying load is profitable since it's happening early enough (before System.initPhase1) to contribute to delaying the initialization of VM subsystems. Webrev: http://cr.openjdk.java.net/~redestad/8202419/open.00/ Looks okay to me. I agree with Alan and Sean w.r.t. to the comment in ACCESS_PERMISSION that would be better to keep it in AccessibleObject::checkPermission and remove line 131 in ReflectionFactory. Mandy
Re: RFR (JDK 12): 6899533: SecureClassLoader and URLClassLoader have unnecessary check for createClassLoader permission
On 9/6/18 12:29 PM, Sean Mullan wrote: Please review this change to remove code that is no longer necessary now that pre-JDK 1.2 SecurityManager methods are no longer supported [1]. In addition, the initialized flag and associated code has been removed from SecureClassLoader as this was only necessary to prevent finalizer attacks prior to JDK 6. webrev: http://cr.openjdk.java.net/~mullan/webrevs/6899533/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6899533 Looks good. Happy to see this is cleaned up. Mandy
Re: RFR: JDK-8210274: Source Launcher should work with a security manager
On 9/11/18 12:34 PM, Alan Bateman wrote: What are the implications for uses of javax.tools and com.sun.tools.javac.Main in code running with a security manager? Maybe that is a separate project but I would have expected to see privileged blocks in places that need permissions. The intent was to stay clear (in this changeset) of all other ways of invoking javac, meaning javax.tools, com.sun.tools.javac.Main and spi.ToolProvider. While there are relatively few ways to invoke javac, these ways all permit the use of annotation processors and other callbacks, and so we would need privileged blocks in all places where callbacks could re-enter javac. That is what I was wondering about it. I don't see a queue at the door of developers looking to run javac with a security manager but at the same time it is possible to configure many app servers with JSP support to run with a security manager. I assume they must have historically granted at least some permissions to "tools.jar" for this to work. Probably. Also they might also wrap their call to javac with doPrivileged. If we were to drop the support for a security manager from the source launcher, would you still consider it worthwhile to update the policy file to enumerate the privileges required to run javac? Setting aside the updates for the Source Launcher tests, I think the work to improve all the other tests to function better when a security manager is involved is also worth while. It took a lot of work in JDK 9 to identify the minimum permissions needed by several modules. The java.xml.bind and java.xml.ws modules took a lot of effort because of the callbacks and missing privileged blocks. It does take a lot of effort and testing to be confident. So my personal view (and not my call) is that it's probably not high priority to do the same for jdk.compiler at this time. I see this changeset is to get the source launcher to work with security manager so that launching in class file mode and source mode can specify the same runtime options. You are right that it'd be a lot of effort to get jdk.compiler to work with callbacks and missing privileged blocks. The minimum permissions are good for source launcher support. If we were to drop the support for a security manager from the source launcher, I think jdk.compiler would need AllPermission due to callback. Mandy
Re: [12] Review Request: 8210692 The "com.sun.awt.SecurityWarning" class can be dropped
On 9/13/18 2:43 PM, Sergey Bylokhov wrote: Hello. Please review fix for jdk12. Bug: https://bugs.openjdk.java.net/browse/JDK-8210692 Webrev: http://cr.openjdk.java.net/~serb/8210692/webrev.00 CSR: https://bugs.openjdk.java.net/browse/JDK-8210693 Thus change looks okay to me. This class is not exported and so I have updated the CSR of "Implementation" scope rather than JDK. Mandy The client code has a "com.sun.awt.SecurityWarning" class which at some point in the past, JDK 6u10, was used as a kind of "public" API. During development of jdk9 it was considered that this API is not supported and may be removed in the future: JDK-8051640. In jdk11 this class was marked as "forRemoval = true" (see JDK-8205588) and can be dropped in 12. In the fix these things were removed: - com/sun/awt/SecurityWarning.java - The test for SecurityWarning class - Test groups, because SecurityWarning is the latest class in "com/sun/awt" package - The reference to "com/sun/awt" in the "default.policy" (cc) core-libs-dev: I am not sure who is responsible to review the change in "default.policy".
Re: RFR (12): 8191053: Provide a mechanism to make system's security manager immutable
On 9/13/18 1:02 PM, Sean Mullan wrote: This is a SecurityManager related change which warrants some additional details for its motivation. The current System.setSecurityManager() API allows a SecurityManager to be set at run-time. However, because of this mutability, it incurs a performance overhead even for applications that never call it and do not enable a SecurityManager dynamically, which is probably the majority of applications. : webrev: http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8203316 JBS: https://bugs.openjdk.java.net/browse/JDK-8191053 This is a welcoming change as many applications run without security manager and they will benefit the performance improvement. This patch makes the private System::security field @Stable hich is the first installment of perf optimization. I hope to see further optimization can be done for example speed up of doPrivileged block in the no-security manager case. This patch looks good. It may be good to add one test case to launch with -Djava.security.manager and -Djdk.allowSecurityManager=false to show that no security manager is installed; essential -Djava.security.manager is ignored. Maybe @implNote should mention such case where -Djava.security.manager is ignored if -Djdk.allowSecurityManager=false. Mandy
Re: RFR (12): 8191053: Provide a mechanism to make system's security manager immutable
On 9/13/18 4:50 PM, Stuart Marks wrote: Hi Sean, Looks sensible to me. On 9/13/18 1:02 PM, Sean Mullan wrote: 2. A new JDK-specific system property to disallow the setting of the security manager at run-time: jdk.allowSecurityManager If set to false, it allows the run-time to optimize the code and improve performance when it is known that an application will never run with a SecurityManager. To support this behavior, the System.setSecurityManager() API has been updated such that it can throw an UnsupportedOperationException if it does not allow a security manager to be set dynamically. I guess the default value is true? The behavior makes sense, though the name I think is misleading. It seems not to disallow a security manager, but to disallow the capability to *set* the security manager. Maybe "jdk.allowSetSecurityManager" ? When -Djdk.allowSecurityManager is set at startup, no security manager is allowed. Most cases a security manager is started via -Djava.security.manager on the command-line. This name also prepares for the future to potentially flip the default (no security manager by default) and allow a security manager at runtime. Mandy
Re: [12] Review Request: 8210692 The "com.sun.awt.SecurityWarning" class can be dropped
On 9/16/18 12:48 AM, Alan Bateman wrote: On 15/09/2018 22:00, Philip Race wrote: It was exported in the past .. and it was publicly documented .. http://www.oracle.com/technetwork/articles/javase/appletwarning-135102.html .. so I think Sergey was correct in his "JDK" scope. Implementation would be for something entirely internal. I think Sergey's changes are okay. The main issue with com.sun.awt API is that it tried to be both an exported and internal/unsupported API at the same time (it's javadoc reads "This class is an implementation detail and only meant for limited use outside of the core platform"). This doesn't work for modules. In addition the design principles in JEP 200 make it clear that standard modules should not export a non-standard package to all modules. If there has been extensive use of this API then I could imagine it being refactored and moved into a JDK-specific module but there was little evidence of usage. So where we ended up in JDK 9 is that the API is not exported. This means, as Mandy hinted, you can't compile against this API (at least not without --add-exports to the compiler). Existing code using this API will continue to run on JDK 9, 10, 11 or until the java.desktop module is fully encapsulated or the SecurityWarning class is removed. Sergey got there first. Yes. Since it was decided not to export com.sun.awt API (JDK-8051640), they are no longer supported. Is there a CSR filed officially declaring com.sun.awt APIs are no longer supported? Since it's already deprecated for removal, it may be worth to have a release note for JDK-8205588 if we didn't officially declare it unsupported. Mandy
Re: RFR (12): 8191053: Provide a mechanism to make system's security manager immutable
On 10/2/18 8:34 AM, Sean Mullan wrote: Hello, Thanks for all the comments so far, and the interesting discussions about the future of the SecurityManager. We will definitely return to those discussions in the near future, but for now I have a second webrev ready for review for this enhancement: http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.01/ : 2. After further thought, I took Bernd's suggestion [1] and instead of adding a new property to disallow the setting of a SecurityManager at runtime, have added new tokens to the existing "java.security.manager" system property, named "=disallow", and "=allow" to toggle this behavior. The "=" is to avoid any potential clashes with custom SM classes with those names. I think this is a cleaner approach. There are a couple of new paragraphs in the SecurityManager class description describing the "java.security.manager" property and how the new tokens work. I'm not a fan of using double == which is not obvious to catch the typo. I think the `==` syntax may not be commonly known either (I suspect it's seldom for a user to override java.security.policy rather than augmenting it). Have you considered using simple token `disallow` and `allow` (or all-caps)? The possibility of a custom security manager class named `disallow` and `allow` should be low. Mandy
Re: RFR (12): 8191053: Provide a mechanism to make system's security manager immutable
On 10/2/18 10:14 AM, Sean Mullan wrote: On 10/2/18 1:05 PM, Mandy Chung wrote: I'm not a fan of using double == which is not obvious to catch the typo. I think the `==` syntax may not be commonly known either (I suspect it's seldom for a user to override java.security.policy rather than augmenting it). Have you considered using simple token `disallow` and `allow` (or all-caps)? I am fine with that as well. The possibility of a custom security manager class named `disallow` and `allow` should be low. I agree. I could imagine there might be a custom SM class named "Disallow" but that should still work, right? Yes since the property value is case-sensitive. Mandy
Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged
On 11/1/18 1:18 AM, Vladimir Ivanov wrote:
I think it's a good idea, but I believe it would require a CSR
request. Do you mind if I file a separate issue for
jdk.internal.vm.annotation.Hidden?
Sure.
Most of the annotations in jdk.internal.vm.annotation were originally
located in java.lang.invoke. Not sure CSR will be required in this
particular case.
@Hidden is internal and no CSR is needed.
FYI. JDK-8212620 is the RFE to consider providing a standard mechanism
to hide frames from stack trace.
Mandy
Best regards,
Vladimir Ivanov
On 10/31/18 6:11 PM, Vladimir Ivanov wrote:
Dean,
src/java.base/share/classes/java/security/AccessController.java:
+ /**
+ * Internal marker for hidden implementation frames.
+ */
+ /*non-public*/
+ @Target(ElementType.METHOD)
+ @Retention(RetentionPolicy.RUNTIME)
+ @interface Hidden {
+ }
You declare @Hidden, but then map it to _method_Hidden along with
@Hidden from java.lang.invoke.LambdaForm.
What do you think about moving LambdaForm.Hidden to
jdk.internal.vm.annotation instead and share among all usages?
Best regards,
Vladimir Ivanov
On 31/10/2018 15:23, dean.l...@oracle.com wrote:
https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1
This change implements AccessController.doPrivileged in Java. This
gives a performance improvement while also being useful to Project
Loom by removing the Java --> native --> Java transition. One
reason doPrivileged has historically been in native is because of
the need to guarantee the cleanup of the privileged context when
doPrivileged returns. To do that in Java, the information that
AccessController.getContext needs is pushed onto the Java stack as
arguments to a method that getContext will recognize during its
stack walk. This allows us to remove the native privileged stack
while guaranteeing that the privileged context goes away when the
method returns.
Tested with tier1-tier3 hotspot and jdk tests and JCK
api/java_security tests. For the first few rounds of testing, I
kept the old native privileged stack and compared the results of
the old and new implementations for each getContext call, which did
catch some early bugs. The changes were also examined by internal
security experts and run through additional internal security tests.
The improvement on this [1] doPrivileged microbenchmark is
approximate 50x.
There is no attempt to optimize getContext() or security permission
checks in this change, however, this is intended to be a first step
towards other possible improvements, for example those proposed
here [2].
dl
[1]
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java
[2]
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html
Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged
On 11/1/18 2:34 PM, dean.l...@oracle.com wrote: @Hidden is internal and no CSR is needed. FYI. JDK-8212620 is the RFE to consider providing a standard mechanism to hide frames from stack trace. OK, I already filed JDK-8213234 but I think I should just close it as a duplicate of JDK-8212620. JDK-8212620 likely does not make JDK 12. I assume Vladimir suggests to do the rename with your fix (or a follow up). Mandy
Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged
Hi Dean, I reviewed webrev.4 version. It looks good. Happy to see moving the doPrivileged support to Java and the performance improvement. On 10/31/18 3:23 PM, dean.l...@oracle.com wrote: https://bugs.openjdk.java.net/browse/JDK-8212605 http://cr.openjdk.java.net/~dlong/8212605/webrev.1 This change implements AccessController.doPrivileged in Java. This gives a performance improvement while also being useful to Project Loom by removing the Java --> native --> Java transition. One reason doPrivileged has historically been in native is because of the need to guarantee the cleanup of the privileged context when doPrivileged returns. To do that in Java, the information that AccessController.getContext needs is pushed onto the Java stack as arguments to a method that getContext will recognize during its stack walk. This allows us to remove the native privileged stack while guaranteeing that the privileged context goes away when the method returns. Tested with tier1-tier3 hotspot and jdk tests and JCK api/java_security tests. For the first few rounds of testing, I kept the old native privileged stack and compared the results of the old and new implementations for each getContext call, which did catch some early bugs. The changes were also examined by internal security experts and run through additional internal security tests. The improvement on this [1] doPrivileged microbenchmark is approximate 50x. There is no attempt to optimize getContext() or security permission checks in this change, however, this is intended to be a first step towards other possible improvements, for example those proposed here [2]. FYI. Sean and I also did some experiment to replace JVM_GetStackAccessControlContext with StackWalker some time ago. Another potential area to move the code from VM to Java for the future as David explored and probably involves performance work in the stack walker. Mandy
Re: 12 RFR(M) 8214583: AccessController.getContext may return wrong value after JDK-8212605
This looks okay to me. Mandy On 12/14/18 4:59 PM, dean.l...@oracle.com wrote: https://bugs.openjdk.java.net/browse/JDK-8214583 http://cr.openjdk.java.net/~dlong/8214583/webrev This change includes two new regression test that demonstrate the problem, and a fix that allows the tests to pass. The problem happens when the JIT compiler's escape analysis eliminates the allocation of the AccessControlContext object passed to doPrivileged. The compiler thinks this is safe because it does not see that the object "escapes". However, getContext needs to be able to find the object using a stack walk, so we need a way to tell the compiler that it does indeed escape. To do this we pass the value to a native method that does nothing. Microbenchmark results: jdk12-b18: Benchmark Mode Cnt Score Error Units DoPrivileged.test avgt 25 255.626 ± 6.446 ns/op DoPrivileged.testInline avgt 25 250.968 ± 4.975 ns/op jdk12-b19: Benchmark Mode Cnt Score Error Units DoPrivileged.test avgt 25 5.689 ± 0.001 ns/op DoPrivileged.testInline avgt 25 2.765 ± 0.001 ns/op this fix: Benchmark Mode Cnt Score Error Units DoPrivileged.test avgt 25 5.020 ± 0.001 ns/op DoPrivileged.testInline avgt 25 2.774 ± 0.025 ns/op dl
Re: [12] RFR(S) 8216151: [Graal] Module jdk.internal.vm.compiler.management has not been granted accessClassInPackage.org.graalvm.compiler.debug
On 1/14/19 9:39 AM, Vladimir Kozlov wrote: Thank you, Alan On 1/14/19 2:27 AM, Alan Bateman wrote: On 13/01/2019 02:46, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/8216151/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8216151 Have to update default.policy after changes in jdk.internal.vm.compiler.management files done by JDK-8199755: "Update Graal". Ran CheckAccessClassInPackagePermissions.java test. cc'ing security-dev as that is where is the security policy file is maintained. One thing is double check is that code in jdk.internal.vm.compiler.management really needs to access members of classes in the listed packages. I ask because the module definition doesn't export some of these packages to jdk.internal.vm.compiler.management so they aren't accessible even when not running with a security manager. I verified that all listed packages are used by compiler.management and I listed only needed in default.policy. I used CheckAccessClassInPackagePermissions.java test to find which permissions are needed. I reviewed the change and the list matches the list of qualified exports from jdk.internal.vm.compiler to jdk.internal.vm.compiler.management. The security team has been looking into removing the private VM call out to ClassLoader::checkPackageAccess. When that's removed, we would not need to maintain these accessClassInPackage permission to access any new qualified exports. Mandy
Re: CSR Review Request, JDK-8218932 Remove the internal package com.sun.net.ssl
On 2/13/19 1:51 PM, Xuelei Fan wrote: Hi, Could I get the CSR reviewed: https://bugs.openjdk.java.net/browse/JDK-8218932 The internal package com.sun.net.ssl had been deprecated since JDK 1.4, and was not exported in the java.base module since JDK 9. Application cannot use them directly now. It should be safe to remove them in JDK 13. If you are using the internal package for some reason, please let me know the compatibility impact by the end of Feb 20, 2019. For those who are not aware of the jdeps tool: jdeps -jdkinternals will flag static references to JDK internal APIs and output some suggestion if a replacement API is known like this: JDK Internal API Suggested Replacement - com.sun.net.ssl.HostnameVerifier Use javax.net.ssl @since 1.4 com.sun.net.ssl.internal.ssl.ProviderUse java.security.Security.getProvider(provider-name) @since 1.3 Run jdeps on your libraries to find out any use of JDK internal APIs. Mandy
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Hi Vladimir,
Indeed these are test issues that the tests will need to grant permissions
to jdk.internal.vm.compiler as the default policy grants.
Thanks for going extra miles to fix the tests.
My suggestion may be a bit general. What I intend to say the custom
security policy should extend the default policy unless it intentionally
excludes configuring permissions for specific modules. I review the
the patch but the test doesn't clearly tell what the test intends to
do w.r.t. security configuration.
We should avoid inadvertently granting permissions that the test expects
to disallow. A better solution is to limit granting permissions just for
`jdk.internal.vm.compiler` module rather than all.
Attached is ModulePolicy class that allows you to get the Policy for
a specific module. It can be put in the test library that these tests
can use them.
So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and
implies method will call the returned ModulePolicy if present.
test/lib/jdk/test/lib/security is one existing testlibrary for security
related stuff. You can consider putting ModulePolicy.java there.
This is one idea.
Mandy
On 6/19/19 6:03 PM, Vladimir Kozlov wrote:
http://cr.openjdk.java.net/~kvn/8185139/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8185139
For Graal to work we have to give Graal module all permissions which
is specified in default policy [1].
Unfortunately this cause problem for Graal running tests which
overwrite default policy.
I discussed this with Mandy and she suggested that such tests should
also check default policy. I implemented her suggestion. Note, this is
not only Graal problem. There were already similar fixes before [2].
I also updated Graal's problem list. Several tests were left on
problem list (with different bug id) because they would not run with
Java Graal (for example, they use --limit-modules flag which prevents
loading Graal module). We will enable such tests again when libgraal
is supported.
I ran testing which execute these tests with Graal. It shows only
known problems which are not related to these changes.
Thanks,
Vladimir
[1]
http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156
[2] https://bugs.openjdk.java.net/browse/JDK-8189291
import java.lang.module.*;
import java.net.*;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.*;
import java.security.*;
public class ModulePolicy extends Policy {
private static final Policy DEFAULT_POLICY = Policy.getPolicy();
private static final Map modulePolicies =
ModuleLayer.boot()
.configuration().modules().stream()
.map(rm -> new ModulePolicy(rm))
.collect(Collectors.toMap(ModulePolicy::name,
Function.identity()));
public static Optional get(String name) {
return Optional.ofNullable(modulePolicies.get(name));
}
final String name;
final URL codeSourceUrl;
final PermissionCollection permissions;
private ModulePolicy(ResolvedModule rm) {
URL url = createURL(rm.reference().location().orElseThrow());
CodeSource cs = new CodeSource(url, (CodeSigner[]) null);
this.name = rm.name();
this.codeSourceUrl = url;
this.permissions = DEFAULT_POLICY.getPermissions(cs);
}
public String name() {
return name;
}
private URL createURL(URI uri) {
URL url = null;
try {
url = uri.toURL();
} catch (MalformedURLException | IllegalArgumentException e) {
}
return url;
}
@Override
public PermissionCollection getPermissions(CodeSource cs) {
return codeSourceUrl.equals(cs.getLocation()) ? permissions : new
Permissions();
}
@Override
public boolean implies(ProtectionDomain domain, Permission perm) {
return permissions.implies(perm);
}
}
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
The Policy API does not assume the presence of the default policy for
the runtime to use.
jdk.internal.vm.compiler already uses doPrivileged. The module ends up
with no permission as the test policy does not consult the default policy.
Mandy
On 6/20/19 6:32 AM, Roger Riggs wrote:
Hi,
If this were java.base, we would use doPrivilege to ignore the policy
and place specific limits.
Encumbering the default policy with conditions needed by a trusted
subsystem seems
like distributing what should be a local implementation issue.
$.02, Roger
On 6/20/19 2:23 AM, Mandy Chung wrote:
Hi Vladimir,
Indeed these are test issues that the tests will need to grant
permissions
to jdk.internal.vm.compiler as the default policy grants.
Thanks for going extra miles to fix the tests.
My suggestion may be a bit general. What I intend to say the custom
security policy should extend the default policy unless it intentionally
excludes configuring permissions for specific modules. I review the
the patch but the test doesn't clearly tell what the test intends to
do w.r.t. security configuration.
We should avoid inadvertently granting permissions that the test expects
to disallow. A better solution is to limit granting permissions just
for
`jdk.internal.vm.compiler` module rather than all.
Attached is ModulePolicy class that allows you to get the Policy for
a specific module. It can be put in the test library that these tests
can use them.
So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and
implies method will call the returned ModulePolicy if present.
test/lib/jdk/test/lib/security is one existing testlibrary for security
related stuff. You can consider putting ModulePolicy.java there.
This is one idea.
Mandy
On 6/19/19 6:03 PM, Vladimir Kozlov wrote:
http://cr.openjdk.java.net/~kvn/8185139/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8185139
For Graal to work we have to give Graal module all permissions which
is specified in default policy [1].
Unfortunately this cause problem for Graal running tests which
overwrite default policy.
I discussed this with Mandy and she suggested that such tests should
also check default policy. I implemented her suggestion. Note, this
is not only Graal problem. There were already similar fixes before [2].
I also updated Graal's problem list. Several tests were left on
problem list (with different bug id) because they would not run with
Java Graal (for example, they use --limit-modules flag which
prevents loading Graal module). We will enable such tests again when
libgraal is supported.
I ran testing which execute these tests with Graal. It shows only
known problems which are not related to these changes.
Thanks,
Vladimir
[1]
http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156
[2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
On 6/20/19 1:40 PM, Sean Mullan wrote:
Sorry, I'm just catching up and looking at this now.
The one thing I don't like about these tests that use their own Policy
implementation is that the permissions that are granted inside the
test are granted to all code, and not just the test, which may lead to
cases where permissions that should be granted to JDK modules in
default.policy may be missed.
When the test wants a fine-control on turning on security manager
programmatically and test various security permission combinations,
I prefer to use a custom Policy implementation.
Otherwise, I have to maintain multiple different test.policy files to
test different permission combination which will be prone to editing
error.
I think we should provide a test library to help building a custom Policy
implementation that can take the default policy, preferrably the policy
for the resolved modules such that the test can focus its logic of
security configuration and does not need to code with the system
modules in mind.
That's what I started with ModulePolicy and could potentially build up
test library for configuring security permissions programmatically.
Mandy
In tests that use policy files, we should grant permissions to only
the test, for example:
grant codebase "file:${test.classes}/.../-" {
permission ...;
};
However, in looking through our policy files, there are many that are
not doing that. Something to fix, so I'll file a bug.
This could also be fixed in these tests by inspecting the CodeSource
of the ProtectionDomain. Or better yet, they should just use the jtreg
java.security.policy option and a policy file and avoid the need to
create a Policy implementation.
Thanks,
Sean
On 6/20/19 11:04 AM, Mandy Chung wrote:
The Policy API does not assume the presence of the default policy for
the runtime to use.
jdk.internal.vm.compiler already uses doPrivileged. The module ends up
with no permission as the test policy does not consult the default
policy.
Mandy
On 6/20/19 6:32 AM, Roger Riggs wrote:
Hi,
If this were java.base, we would use doPrivilege to ignore the
policy and place specific limits.
Encumbering the default policy with conditions needed by a trusted
subsystem seems
like distributing what should be a local implementation issue.
$.02, Roger
On 6/20/19 2:23 AM, Mandy Chung wrote:
Hi Vladimir,
Indeed these are test issues that the tests will need to grant
permissions
to jdk.internal.vm.compiler as the default policy grants.
Thanks for going extra miles to fix the tests.
My suggestion may be a bit general. What I intend to say the custom
security policy should extend the default policy unless it
intentionally
excludes configuring permissions for specific modules. I review the
the patch but the test doesn't clearly tell what the test intends to
do w.r.t. security configuration.
We should avoid inadvertently granting permissions that the test
expects
to disallow. A better solution is to limit granting permissions
just for
`jdk.internal.vm.compiler` module rather than all.
Attached is ModulePolicy class that allows you to get the Policy for
a specific module. It can be put in the test library that these tests
can use them.
So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and
implies method will call the returned ModulePolicy if present.
test/lib/jdk/test/lib/security is one existing testlibrary for
security
related stuff. You can consider putting ModulePolicy.java there.
This is one idea.
Mandy
On 6/19/19 6:03 PM, Vladimir Kozlov wrote:
http://cr.openjdk.java.net/~kvn/8185139/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8185139
For Graal to work we have to give Graal module all permissions
which is specified in default policy [1].
Unfortunately this cause problem for Graal running tests which
overwrite default policy.
I discussed this with Mandy and she suggested that such tests
should also check default policy. I implemented her suggestion.
Note, this is not only Graal problem. There were already similar
fixes before [2].
I also updated Graal's problem list. Several tests were left on
problem list (with different bug id) because they would not run
with Java Graal (for example, they use --limit-modules flag which
prevents loading Graal module). We will enable such tests again
when libgraal is supported.
I ran testing which execute these tests with Graal. It shows only
known problems which are not related to these changes.
Thanks,
Vladimir
[1]
http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156
[2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal
Hi Vladimir,
As long as the owner of the test review the patch and confirm the
test policy intends to extend the default policy, those test change
will be fine.
test/jdk/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java
417 DEFAULT_POLICY.implies(domain, permission)) return true; Nit:
separating this to a new if-case after 426 to make it distinct.
I reviewedtest/jdk/java/lang/Class/getDeclaredField/* and
test/jdk/java/lang/invoke/* patch and they look fine.
My suggestion is to minimize the risk of your patch. Since Daniel
has reviewed the logging test change (which is the bulk of this patch),
I have no issue if the reviewers approve this patch. I will file
a separate RFE for the test library idea.
Mandy
On 6/20/19 11:05 AM, Vladimir Kozlov wrote:
Hi Mandy
I am not sure about this suggestion. Graal module may not be present
in the JDK (on SPARC for example).
And I don't want pollute general tests with Graal specific code:
ModulePolicy.get("jdk.internal.vm.compiler").
If you or other have concerns about some tests looking on default
policy I can keep them on problem list to run them later only with
libgraal.
I found only 2 closed tests in which I had doubt about using default
policy. I kept them on problem list.
Other tests, as I understand, manipulate permissions for test classes.
They don't extend system classes so default policy should not affect
test class permissions.
Thanks,
Vladimir
On 6/19/19 11:23 PM, Mandy Chung wrote:
Hi Vladimir,
Indeed these are test issues that the tests will need to grant
permissions
to jdk.internal.vm.compiler as the default policy grants.
Thanks for going extra miles to fix the tests.
My suggestion may be a bit general. What I intend to say the custom
security policy should extend the default policy unless it intentionally
excludes configuring permissions for specific modules. I review the
the patch but the test doesn't clearly tell what the test intends to
do w.r.t. security configuration.
We should avoid inadvertently granting permissions that the test expects
to disallow. A better solution is to limit granting permissions just
for
`jdk.internal.vm.compiler` module rather than all.
Attached is ModulePolicy class that allows you to get the Policy for
a specific module. It can be put in the test library that these tests
can use them.
So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and
implies method will call the returned ModulePolicy if present.
test/lib/jdk/test/lib/security is one existing testlibrary for security
related stuff. You can consider putting ModulePolicy.java there.
This is one idea.
Mandy
On 6/19/19 6:03 PM, Vladimir Kozlov wrote:
http://cr.openjdk.java.net/~kvn/8185139/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8185139
For Graal to work we have to give Graal module all permissions which
is specified in default policy [1].
Unfortunately this cause problem for Graal running tests which
overwrite default policy.
I discussed this with Mandy and she suggested that such tests should
also check default policy. I implemented her suggestion. Note, this
is not only Graal problem. There were already similar fixes before [2].
I also updated Graal's problem list. Several tests were left on
problem list (with different bug id) because they would not run with
Java Graal (for example, they use --limit-modules flag which
prevents loading Graal module). We will enable such tests again when
libgraal is supported.
I ran testing which execute these tests with Graal. It shows only
known problems which are not related to these changes.
Thanks,
Vladimir
[1]
http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156
[2] https://bugs.openjdk.java.net/browse/JDK-8189291
Re: RFR: 8191138: Remove deprecated java.security.acl APIs
On 7/25/19 9:15 AM, Sean Mullan wrote: Please review this change to remove the deprecated java.security.acl APIs. These APIs have long had a warning in the javadocs (since at least JDK 1.3.1 and possibly earlier) indicating that they were superseded by other APIs. They were initially deprecated in JDK 9 and marked for removal in JDK 10, so enough time has passed to allow for removal. Several external projects that were using the APIs have also either resolved the issues or have a plan to remove the dependencies. CSR: https://bugs.openjdk.java.net/browse/JDK-8217101 Issue: https://bugs.openjdk.java.net/browse/JDK-8191138 webrev: http://cr.openjdk.java.net/~mullan/webrevs/8191138/webrev.00/ +1 It's good to know that external projects have either removed or plan to remove their dependency on java.security.acl API. Mandy
Re: Fix for Javadoc errors in java.base
Looks fine. Thanks Mandy On 8/18/20 10:02 AM, Julia Boes wrote: Hi, The two changes below still need to be reviewed. Any takers? Cheers, Julia --- old/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java2020-08-14 23:55:41.953638446 +0530 +++ new/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java2020-08-14 23:55:41.445619497 +0530 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012, 2013, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -208,7 +208,7 @@ * * @return a CallSite, which, when invoked, will return an instance of the * functional interface - * @throws ReflectiveOperationException + * @throws LambdaConversionException */ abstract CallSite buildCallSite() throws LambdaConversionException; --- old/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java2020-08-14 23:55:42.977677096 +0530 +++ new/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java2020-08-14 23:55:42.473658062 +0530 @@ -200,7 +200,6 @@ * * @return a CallSite, which, when invoked, will return an instance of the * functional interface - * @throws ReflectiveOperationException * @throws LambdaConversionException If properly formed functional interface * is not found */
Re: RFR: 8253208: Move CDS related code to a separate class [v2]
On Mon, 21 Sep 2020 18:17:55 GMT, Yumin Qi wrote: >> With more CDS related code added to VM, it is time to move CDS code to a >> separate class. CDS is the new class which is >> specific to CDS. >> Tests: tier1-4 > > Yumin Qi has updated the pull request incrementally with one additional > commit since the last revision: > > 8253208: Move CDS related code to a separate class This patch looks okay. Please add the javadoc for `CDS::getRandomSeedForDumping` and `CDS::defineArchiveModules` for completeness. - PR: https://git.openjdk.java.net/jdk/pull/261
Re: RFR: 8253208: Move CDS related code to a separate class [v3]
On Mon, 21 Sep 2020 22:24:15 GMT, Yumin Qi wrote: >> With more CDS related code added to VM, it is time to move CDS code to a >> separate class. CDS is the new class which is >> specific to CDS. >> Tests: tier1-4 > > Yumin Qi has updated the pull request incrementally with one additional > commit since the last revision: > > 8253208: Move CDS related code to a separate class Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/261
Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag
On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons wrote: > The change is (just) to remove legacy usages of a JDK-private custom tag. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/814
