Re: RFR: 8255557: Decouple GCM from CipherCore [v4]
On Fri, 21 May 2021 02:03:56 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 1515:
>
>> 1513: // the dst buffer is too short. Context will be restored
>> so the
>> 1514: // method can be called again with the proper sized dst
>> buffer.
>> 1515: if (len > dst.remaining()) {
>
> We should check the dst capacity in the beginning of the method, if its
> capacity is too small, i.e. less than the overall length - tag length, don't
> proceed further.
> As in the doFinal() w/ byte[] arguments, I am not sure if the save/restore is
> really necessary.
> Maybe add a test if it's not already covered by existing tests if time
> permits.
I think it can be moved up without doing any extra work
I'm pretty sure there is a test because I remember Sean C working on a bug with
restoring when dst was too small.
> src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java
> line 221:
>
>> 219:
>> 220: store("AES/GCM", KnownOIDs.AES_128$GCM$NoPadding,
>> 221: "AES/GCM/NoPadding");
>
> In SunJCE class, the registration is done using "AES/GCM/NoPadding" instead
> of "AES/GCM". I am not sure why is this store() call using AES/GCM is
> necessary?
I will double check this, but I believe this change and the need for AESGCM in
SunJCE are linked together
-
PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v4]
On Fri, 21 May 2021 01:58:43 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix perf problem by reorganizing doLastBlock() > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 1510: > >> 1508: >> 1509: // 'len' contains the length in ibuffer and src >> 1510: checkDataLength(len); > > Is this really useful given 'processed' is 0 and there is only one argument > 'len'. Should always pass? Are you asking if its necessary at all to check the length when decrypting? That's a good question given it's encryption. Maybe decryption doesn't need this check, only encryption. If we do want to check it, then len could be greater than MAX_BUF_SIZE, then the exception would be thrown - PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v4]
On Fri, 21 May 2021 01:57:08 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix perf problem by reorganizing doLastBlock() > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 1503: > >> 1501: tag.put(ct); >> 1502: tag.flip(); >> 1503: // Limit is how much of the ibuffer has been chopped >> off. > > The comment seems incorrect? Limit is not how much the ibuffer has been > chopped off, but rather what has not been chopped off, no? I think it's which direction you are looking at it from, when I wrote this I was thinking how much of the ibuffer was chopped off into tag.. i can reword that because its ambiguous. - PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v4]
On Wed, 19 May 2021 21:18:15 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix perf problem by reorganizing doLastBlock() > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 628: > >> 626: } >> 627: >> 628: int mergeBlock(byte[] buffer, int bufOfs, byte[] in, int inOfs, > > Can be made 'static'? No javadoc for this one, maybe move the javadoc for the > other one up and use "Methods" in the javadoc to cover both? I just copied it and made a slight change to explain the difference - PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v4]
On Wed, 19 May 2021 21:18:27 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix perf problem by reorganizing doLastBlock() > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 640: > >> 638: * @return number of bytes used from 'in' >> 639: */ >> 640: int mergeBlock(byte[] buffer, int bufOfs, int bufLen, byte[] in, > > Can be made 'static'? mergeBlock contains blockSize which isn't static > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 648: > >> 646: >> 647: System.arraycopy(buffer, bufOfs, block, 0, bufLen); >> 648: int inUsed = Math.min(block.length - bufLen, > > Seems equivalent to `int inUsed = Math.min((block.length - bufLen), inLen);`? I think you're right.. and my main test seems to be ok with it too - PR: https://git.openjdk.java.net/jdk/pull/4072
Re: [External] : Re: JEP411: Missing use-case: Monitoring / restricting libraries
On 18/05/2021 10:09 pm, Ron Pressler wrote: On 18 May 2021, at 12:27, Peter Firmstone wrote: However I disagree that the Principle of least privilege is wrong headed, I think you've been discussing sandbox concepts with the experts and they're going to tell you that's a bad idea. But the two aren't the same, one is access control, the other is attempted complete leak proof isolation, which is a very difficult thing to do that grows exponentially with your API surface area, I'm sure in years to come crackers will find vulnerabilities in VM's too. Sandboxing would work if you removed most of the library code and features and limited memory and threads, but I don't think anyone is seriously considering that. Java for applets should have been it's own stripped down limited JVM. When you say the “principle of least privilege” you seem to imply that it is at *all* cost. Many believe that the attempt to assign different privileges to different *pieces of code* (not different people) is not effective from a cost/benefit perspective. In other words, the effort would have a bigger impact on security if directed elsewhere. After all, the same principle would imply that we’d need to afford different permissions at a method granularity rather than class granularity. No, actually to date, it's been a very affordable option, probably because the burden has been on Openjdk developers to date, to which I'm very grateful. It doesn't impact performance and it allows me to reduce the attack surface of my programs, especially in regard to dependencies. https://debricked.com/blog/2021/01/02/vulnerabilities-in-dependencies/ What sort of cost, you mean development cost, the cost of understanding doPrivileged? Both, but mostly the former. Yes everything is a compromise and there are trade-offs. The way I see it, the cheapest way to maintain security is to find others who share a common pain point is to maintain a copy of OpenJDK focused on security. We can audit upstream changes and add them later, with the security cost penalty, without throwing out the huge amount of effort performed by JDK developers over the years, there's no way we can re-implement it economically, so this appears the most sensible option. At least to me, it's just a simple lambda expression, it's not difficult. It think people are making this more complex than it needs to be due to a proliferation of Permissions, but even that I manage with tooling. There’s no point to argue over this. It’s been tried for a couple of decades, people don’t want it, experts say it’s not cost-effective, no popular platform after Java and .NET has chosen to invest in it, and .NET has dropped it already. Even if you convinced me that the effort of attempting to afford different permissions to different code in the same process was an effective use of resources, if you could have changed the habits of millions of developers you would have done it by now. You can carry on the argument over whether this is a good or bad idea on security forums and at security conferences, but that it didn’t pan out, adoption-wise, in practice is an empirical fact (that it's worked great *for you* is completely irrelevant). Even good ideas can fail to gain traction, and we shouldn’t throw good money after bad. These are just opinions, it's nice to have them, but they're not helping. I think you'll find usages of SecurityManager api's are far more widespread than your opinion suggests, but that's just my opinion too lol. You've probably have never used it, so you don't really understand it, I can't argue with ignorance and you can't understand the benefit, if you are only asking others for their opinions, you are likely surrounded by people with the same opinion, so you're suffering from confirmation bias without any practical application, as is anyone else that doesn't use it. https://berteig.com/coaching/confirmation-bias/ But it's such a shame to lose access control as collateral damage. Most applications that do access control today do not use the Security Manager for that. When you say “access control” you mean something very specific, which is not what others mean when they say access control. In particular, even doAs is proposed to remain. — Ron Yes, I think we should keep it simple, and keep calling it the principle of least privilege, to avoid confusion, it's very beneficial for reducing the consequences of perimeter security failure, even if your experts think otherwise. -- Regards, Peter Firmstone
Re: RFR: 8255557: Decouple GCM from CipherCore [v3]
On Fri, 21 May 2021 02:51:05 GMT, Anthony Scarpino
wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
>> line 402:
>>
>>> 400: }
>>> 401: try {
>>> 402: ArrayUtil.nullAndBoundsCheck(input, inputOffset, inputLen);
>>
>> Why is only this ArrayUtil.nullAndBoundsCheck(...) present in this
>> engineDoFinal(...)? There are other engineUpdate/engineDoFinal() calls which
>> also have input array, offset, and length. Shouldn't this check be added
>> there as well? If the crypto engine check is separated out into a separate
>> method, e.g. checkEngine(), then you don't have to explicitly release the
>> crypto engine (as on line 405) and can just call checkEngine() after all the
>> input validation has passed.
>
> yeah these checks are a bit all over the place.. I'll rework them
So I think I only need to add a check to the engineDoFinal() that did not have
a check before.
-
PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v4]
On Wed, 19 May 2021 20:21:23 GMT, Anthony Scarpino
wrote:
>> Hi,
>>
>> I need a review of this rather large change to GCM. GCM will no longer use
>> CipherCore, and AESCrypt to handle it's buffers and other objects. It is
>> also a major code redesign limits the amount of data copies and make some
>> performance-based decisions.
>>
>> Thanks
>>
>> Tony
>
> Anthony Scarpino has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix perf problem by reorganizing doLastBlock()
Here are comments on the remaining src changes. Will continue looking at the
tests.
Thanks,
Valerie
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
628:
> 626: }
> 627:
> 628: int mergeBlock(byte[] buffer, int bufOfs, byte[] in, int inOfs,
Can be made 'static'? No javadoc for this one, maybe move the javadoc for the
other one up and use "Methods" in the javadoc to cover both?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
637:
> 635: * The method takes two buffers to create one block of data
> 636: *
> 637: * This in only called when buffer length is less that a
> blockSize
nit: typo in->is, that->than
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
640:
> 638: * @return number of bytes used from 'in'
> 639: */
> 640: int mergeBlock(byte[] buffer, int bufOfs, int bufLen, byte[] in,
Can be made 'static'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
648:
> 646:
> 647: System.arraycopy(buffer, bufOfs, block, 0, bufLen);
> 648: int inUsed = Math.min(block.length - bufLen,
Seems equivalent to `int inUsed = Math.min((block.length - bufLen), inLen);`?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
741:
> 739: } else {
> 740: // If the remaining in buffer + data does
> not fill a
> 741: // block, complete the gctr operation
This comment seems more suited for the else block below at line 753. In
addition, the criteria for completing the gctr operation should be whether src
still has remaining bytes left. It's possible that the (src.remaining() ==
blockSize - over) and happen to fill up the block, right? The current flow for
this particular case would be an op.update(...) then continue down to line
770-778, maybe you can adjust the if-check on line748 so this would go through
line 754-759 and return, i.e. the else block?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
752:
> 750: if (dst != null) {
> 751: dst.put(block, 0, blockSize);
> 752: }
not counting this blockSize value into "processed"?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
752:
> 750: if (dst != null) {
> 751: dst.put(block, 0, blockSize);
> 752: }
Why not just do `op.update(block, 0, blockSize, dst); `?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
757:
> 755: if (dst != null) {
> 756: dst.put(block, 0, Math.min(block.length,
> len));
> 757: }
Would this method work correctly if dst is null? Shouldn't this be checked in
the beginning of this method?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
805:
> 803: /**
> 804: * This segments large data into smaller chunks so hotspot will
> start
> 805: * using CTR and GHASH intrinsics sooner. This is a problem for
> app
nit: typo: CTR->GCTR? This is to improve performance rather than a problem, no?
Same comment applies to the other throttleData() method above.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
874:
> 872: } else if (!src.isDirect() && !dst.isDirect()) {
> 873: // if src is read only, then we need a copy
> 874: if (!src.isReadOnly()) {
Do you mean if (src.hasArray() && dst.hasArray())? Even if src is not read
only, but array()/arrayOffset() methods are optional and can only be safely
invoked after the hasArray() call.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
908:
> 906: * If an intermediate array is needed, the whole original out
> array
> 907: * length is allocated because it's simpler than keep the same
> offset
> 908: * and hope the expected output is
The comment seems incomplete? Perhaps you mean "if an intermediate array is
needed, it will be allocated using the whole original out array length, so that
th
Re: RFR: 8255557: Decouple GCM from CipherCore [v2]
On Mon, 17 May 2021 21:58:48 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - review comment updates
>> - Fixed the lack of overlap detection with GCMEncrypt.update()
>
> src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 54:
>
>> 52: * @since 1.8
>> 53: */
>> 54: final class GCTR extends CounterMode implements GCM {
>
> Not sure if this really needs to implements GCM?
Well, I see how you use this GCM interface now.
-
PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v3]
On Wed, 19 May 2021 19:20:20 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> cleanup
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 402:
>
>> 400: }
>> 401: try {
>> 402: ArrayUtil.nullAndBoundsCheck(input, inputOffset, inputLen);
>
> Why is only this ArrayUtil.nullAndBoundsCheck(...) present in this
> engineDoFinal(...)? There are other engineUpdate/engineDoFinal() calls which
> also have input array, offset, and length. Shouldn't this check be added
> there as well? If the crypto engine check is separated out into a separate
> method, e.g. checkEngine(), then you don't have to explicitly release the
> crypto engine (as on line 405) and can just call checkEngine() after all the
> input validation has passed.
yeah these checks are a bit all over the place.. I'll rework them
-
PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v3]
On Tue, 18 May 2021 22:23:11 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> cleanup > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 225: > >> 223: } >> 224: >> 225: SecureRandom random = null; > > Is this field used? I didn't find any assignment. If it's needed, can you > please move it to where the rest of fields are? As part of your earlier comment, I realized it should be used when the user specifies a random. I also moved it up to the top - PR: https://git.openjdk.java.net/jdk/pull/4072
RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K
The code change refactors classes that have a `SuppressWarnings("removal")`
annotation that covers more than 50KB of code. The big annotation is often
quite faraway from the actual deprecated API usage it is suppressing, and with
the annotation covering too big a portion it's easy to call other deprecated
methods without being noticed.
-
Depends on: https://git.openjdk.java.net/jdk/pull/4073
Commit messages:
- 8267521: Post JEP 411 refactoring: maximum covering > 50K
Changes: https://git.openjdk.java.net/jdk/pull/4138/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8267521
Stats: 226 lines in 18 files changed: 142 ins; 29 del; 55 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: [External] : Re: JEP411: Missing use-case: Monitoring / restricting libraries
If there are those of us who wanted to maintain a fork of Java 17,
focused on security, we could backport new features after they've been
reviewed for security.
Would we be welcomed to do that here? Otherwise is it something we
should do on GitHub?
Cheers,
Peter.
On 21/05/2021 11:25 am, David Black wrote:
On Thu, 20 May 2021 at 21:27, Andrew Dinn wrote:
On 18/05/2021 23:06, David Black wrote:
I don't think that this thinking is unique but it might not be worth
the "cost" to Oracle to maintain something that seemingly for various
reasons Oracle has little interest in maintaining (we're not in
applet-land anymore). I would like to encourage proposals that mean
that people who want to do 4, who implement further security hardening
where others seemingly shy away, can continue to do 4.
Please don't do that. The cost Ron is talking about is not to "Oracle".
It is a cost to the OpenJDK project as a whole.
Sorry about that, that is a good point.
Likewise, the lack of project team interest in maintaining the security
manager and self-evident interest in applying resources to providing
other, more valuable Java capabilities is not simply restricted to
"Oracle" project members.
regards,
Andrew Dinn
---
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
--
Regards,
Peter Firmstone
0498 286 363
Zeus Project Services Pty Ltd.
Re: [External] : Re: JEP411: Missing use-case: Monitoring / restricting libraries
On Thu, 20 May 2021 at 21:27, Andrew Dinn wrote:
>
> On 18/05/2021 23:06, David Black wrote:
> > I don't think that this thinking is unique but it might not be worth
> > the "cost" to Oracle to maintain something that seemingly for various
> > reasons Oracle has little interest in maintaining (we're not in
> > applet-land anymore). I would like to encourage proposals that mean
> > that people who want to do 4, who implement further security hardening
> > where others seemingly shy away, can continue to do 4.
> Please don't do that. The cost Ron is talking about is not to "Oracle".
> It is a cost to the OpenJDK project as a whole.
Sorry about that, that is a good point.
> Likewise, the lack of project team interest in maintaining the security
> manager and self-evident interest in applying resources to providing
> other, more valuable Java capabilities is not simply restricted to
> "Oracle" project members.
>
> regards,
>
>
> Andrew Dinn
> ---
> Red Hat Distinguished Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill
>
--
David Black / Security Engineer.
Re: RFR: 8255557: Decouple GCM from CipherCore [v3]
On Tue, 18 May 2021 22:46:58 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> cleanup
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 168:
>
>> 166: // return tag length in bytes
>> 167: int getTagLen() {
>> 168: return this.tagLenBytes;
>
> Doesn't seem too useful if all it does is just returning the 'tagLenBytes'
> field? With your current code, tagLenBytes is initialized with 16 and is set
> in init(). When a GCMParameterSpec is not specified, it uses the tagLenBytes
> value from earlier init() instead of a fixed default. This seems incorrect?
Agreed.. I was probably trying to follow the getIv() idea, but never
implemented it elsewhere
-
PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v3]
On Tue, 18 May 2021 21:58:24 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> cleanup
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 149:
>
>> 147: if (spec == null) {
>> 148: throw new InvalidKeyException("No GCMParameterSpec
>> specified");
>> 149: }
>
> This seems redundant as it's already been checked in engineInit() before
> calling this method. Line 154 directly calls spec.getTLen() without checking
> spec != null.
That does appear to be unnecessary. engineInit() make sure there is a spec
exists.
-
PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v3]
On Tue, 18 May 2021 20:33:22 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> cleanup
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 133:
>
>> 131: throw new InvalidKeyException("The key must be " +
>> 132: keySize + " bytes");
>> 133: }
>
> Set the keyValue to all 0s before throwing exception, i.e. try-finally.
If the key is not valid and never used, I don't see why it needs to be cleared.
-
PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v3]
On Tue, 18 May 2021 18:52:06 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> cleanup > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 75: > >> 73: private GCMEngine engine; >> 74: >> 75: private boolean encryption = true; > > Move these non-static fields to where they belong? Do you mean below the static references? I never noticed that as a style requirement - PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v4]
On Tue, 18 May 2021 21:56:38 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 59:
>
>> 57: final class GaloisCounterMode extends FeedbackCipher {
>> 58:
>> 59: static int DEFAULT_TAG_LEN = AES_BLOCK_SIZE;
>
> Hmm, for the same reason as the DEFAULT_IV_LEN, you will probably need a
> DEFAULT_TAG_LEN. Otherwise, how do you know what the default tag length is
> after several Cipher.init() calls?
Yes, I can see where running init() on a previously used object can use the old
tag
-
PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v3]
On Wed, 19 May 2021 18:17:27 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> cleanup > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 259: > >> 257: @Override >> 258: protected void engineInit(int opmode, Key key, >> 259: AlgorithmParameterSpec params, SecureRandom random) > > The specified "random" is not saved to internal "random". Perhaps an > oversight? Yeah, I changed this code long ago, i suspect I didn't realize engineGetParameters() used it - PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8255557: Decouple GCM from CipherCore [v3]
On Wed, 19 May 2021 18:15:26 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> cleanup
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 279:
>
>> 277: if (iv.length == 0) {
>> 278: throw new InvalidAlgorithmParameterException("IV is
>> empty");
>> 279: }
>
> Why separating the validation of iv and tag length in separate methods, i.e.
> engineInit() vs init()? Consider consolidating them?
I think I had a reason for this where I didn't know what the exact error was.
I'd rather keep them separate so it throws the right message for the error.
It's not a performance critical area.
-
PR: https://git.openjdk.java.net/jdk/pull/4072
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java This isn't a review of the code changes, although I did take a quick look at the manual changes, and they look fine. I did a local build of the PR branch on Windows, Mac, and Linux, and then did a build / test of JavaFX using that locally built JDK to find all the places where I need to add `-Djava.security.manager=allow` to the JavaFX tests to fix [JDK-8264140](https://bugs.openjdk.java.net/browse/JDK-8264140). It's working as expected. - Marked as reviewed by kcr (Author). PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v24]
On Wed, 28 Apr 2021 08:20:05 GMT, Chris Hegarty wrote: >> src/java.base/share/classes/sun/nio/ch/IOUtil.java line 466: >> >>> 464: } >>> 465: >>> 466: private static final JavaNioAccess NIO_ACCESS = >>> SharedSecrets.getJavaNioAccess(); >> >> It might be cleaner to move to acquire/release methods to their own >> supporting class as it's not really IOUtil. > > I went back and forth on this a number of times already. I think where we > landed is a reasonable place, given the current shape of the code. > > Scope is a private property of Buffer, and as such should be consulted > anywhere where a buffer's address is being accessed. In fact, a prior > prototype would not allow access to the underlying address value without the > caller passing a valid handle for the buffer view's scope. It's hard to find > the sweet-spot here between code reuse and safety, but the high-order bit is > that the code accessing the address is auditable and testable to avoid > accessing memory unsafely. Maybe there is a better alternative implementation > code structure (at the cost of some duplication), but it is not obvious to me > what that is (and I have given it some thought). Suggestions welcome. > > Note, there is a little more follow-on work to be done in this area, if we > are to expand support to other non-TCP channel implementations. Maybe > investigation into possible code refactorings could be done as part of that? Can you create a follow-on issue to re-visit the changes to IOUtil? The changes in this area that are in this PR will need to re-worked so that it more cleanly separate the synchronous and asynchronous usages. - PR: https://git.openjdk.java.net/jdk/pull/3699
Integrated: 8261880: Change nested classes in java.base to static nested classes where possible
On Tue, 16 Feb 2021 14:30:58 GMT, Сергей Цыпанов wrote: > Non-static classes hold a link to their parent classes, which in many cases > can be avoided. This pull request has now been integrated. Changeset: 9425d3de Author:Sergey Tsypanov Committer: Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/9425d3de83fe8f4caddef03ffa3f4dd4de58f236 Stats: 15 lines in 11 files changed: 0 ins; 0 del; 15 mod 8261880: Change nested classes in java.base to static nested classes where possible Reviewed-by: redestad - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:38:03 GMT, Сергей Цыпанов wrote: >> Non-static classes hold a link to their parent classes, which in many cases >> can be avoided. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8261880: Remove static from declarations of Holder nested classes Just for information there is similar issues in `javax.imageio.metadata.IIOMetadataFormatImpl` class in the `java.desktop` module. - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v24]
On Thu, 20 May 2021 13:05:22 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-412 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/412 > > Maurizio Cimadamore has updated the pull request with a new target base due > to a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 35 additional > commits since the last revision: > > - Add sealed modifiers in morally sealed API interfaces > - Merge branch 'master' into JEP-412 > - Fix VaList test >Remove unused code in Utils > - Merge pull request #11 from JornVernee/JEP-412-MXCSR > >Add MXCSR save and restore to upcall stubs for non-windows platforms > - Add MXCSR save and restore to upcall stubs for non-windows platforms > - Merge branch 'master' into JEP-412 > - Fix issue with bounded arena allocator > - Rename is_entry_blob to is_optimized_entry_blob >Rename as_entry_blob to as_optimized_entry_blob >Fix misc typos and indentation issues > - Take care of remaining references to "Panama" > - * Replace is_panama_entry_frame() with is_optimized_entry_frame() >* Replace EntryBlob with OptimizedEntryBlob > - ... and 25 more: > https://git.openjdk.java.net/jdk/compare/354e6edf...e927c235 Latest javadoc: http://cr.openjdk.java.net/~mcimadamore/JEP-412/v3/javadoc/jdk/incubator/foreign/package-summary.html Latest specdiff: http://cr.openjdk.java.net/~mcimadamore/JEP-412/v3/specdiff/overview-summary.html - PR: https://git.openjdk.java.net/jdk/pull/3699
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v24]
On Thu, 20 May 2021 13:00:14 GMT, Maurizio Cimadamore
wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due
>> to a merge or a rebase. The incremental webrev excludes the unrelated
>> changes brought in by the merge/rebase. The pull request contains 35
>> additional commits since the last revision:
>>
>> - Add sealed modifiers in morally sealed API interfaces
>> - Merge branch 'master' into JEP-412
>> - Fix VaList test
>>Remove unused code in Utils
>> - Merge pull request #11 from JornVernee/JEP-412-MXCSR
>>
>>Add MXCSR save and restore to upcall stubs for non-windows platforms
>> - Add MXCSR save and restore to upcall stubs for non-windows platforms
>> - Merge branch 'master' into JEP-412
>> - Fix issue with bounded arena allocator
>> - Rename is_entry_blob to is_optimized_entry_blob
>>Rename as_entry_blob to as_optimized_entry_blob
>>Fix misc typos and indentation issues
>> - Take care of remaining references to "Panama"
>> - * Replace is_panama_entry_frame() with is_optimized_entry_frame()
>>* Replace EntryBlob with OptimizedEntryBlob
>> - ... and 25 more:
>> https://git.openjdk.java.net/jdk/compare/26a8cf83...e927c235
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayout.java
> line 200:
>
>> 198: * Implementations of this interface are immutable, thread-safe and > href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based.
>> 199: */
>> 200: public sealed interface MemoryLayout extends Constable permits
>> AbstractLayout, SequenceLayout, GroupLayout, PaddingLayout, ValueLayout {
>
> In principle we could just permit AbstractLayout and be done. In practice,
> the javadoc comes out better if we list all the concrete API interfaces which
> extend MemoryLayout, as the `permit` list will be displayed in the javadoc.
At least the internal class name is hidden in the javadoc:
-
PR: https://git.openjdk.java.net/jdk/pull/3699
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Thu, 20 May 2021 10:42:49 GMT, Claes Redestad wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8261880: Remove static from declarations of Holder nested classes > > Marked as reviewed by redestad (Reviewer). @cl4es now you can sponsor :) - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v24]
On Thu, 20 May 2021 13:00:15 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-412 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 35 additional
> commits since the last revision:
>
> - Add sealed modifiers in morally sealed API interfaces
> - Merge branch 'master' into JEP-412
> - Fix VaList test
>Remove unused code in Utils
> - Merge pull request #11 from JornVernee/JEP-412-MXCSR
>
>Add MXCSR save and restore to upcall stubs for non-windows platforms
> - Add MXCSR save and restore to upcall stubs for non-windows platforms
> - Merge branch 'master' into JEP-412
> - Fix issue with bounded arena allocator
> - Rename is_entry_blob to is_optimized_entry_blob
>Rename as_entry_blob to as_optimized_entry_blob
>Fix misc typos and indentation issues
> - Take care of remaining references to "Panama"
> - * Replace is_panama_entry_frame() with is_optimized_entry_frame()
>* Replace EntryBlob with OptimizedEntryBlob
> - ... and 25 more:
> https://git.openjdk.java.net/jdk/compare/629f67e6...e927c235
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayout.java
line 200:
> 198: * Implementations of this interface are immutable, thread-safe and href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based.
> 199: */
> 200: public sealed interface MemoryLayout extends Constable permits
> AbstractLayout, SequenceLayout, GroupLayout, PaddingLayout, ValueLayout {
In principle we could just permit AbstractLayout and be done. In practice, the
javadoc comes out better if we list all the concrete API interfaces which
extend MemoryLayout, as the `permit` list will be displayed in the javadoc.
-
PR: https://git.openjdk.java.net/jdk/pull/3699
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v24]
> This PR contains the API and implementation changes for JEP-412 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/412 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 35 additional commits since the last revision: - Add sealed modifiers in morally sealed API interfaces - Merge branch 'master' into JEP-412 - Fix VaList test Remove unused code in Utils - Merge pull request #11 from JornVernee/JEP-412-MXCSR Add MXCSR save and restore to upcall stubs for non-windows platforms - Add MXCSR save and restore to upcall stubs for non-windows platforms - Merge branch 'master' into JEP-412 - Fix issue with bounded arena allocator - Rename is_entry_blob to is_optimized_entry_blob Rename as_entry_blob to as_optimized_entry_blob Fix misc typos and indentation issues - Take care of remaining references to "Panama" - * Replace is_panama_entry_frame() with is_optimized_entry_frame() * Replace EntryBlob with OptimizedEntryBlob - ... and 25 more: https://git.openjdk.java.net/jdk/compare/788875f9...e927c235 - Changes: - all: https://git.openjdk.java.net/jdk/pull/3699/files - new: https://git.openjdk.java.net/jdk/pull/3699/files/f5668ffc..e927c235 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3699&range=23 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3699&range=22-23 Stats: 7087 lines in 360 files changed: 4302 ins; 1796 del; 989 mod Patch: https://git.openjdk.java.net/jdk/pull/3699.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3699/head:pull/3699 PR: https://git.openjdk.java.net/jdk/pull/3699
Re: [External] : Re: JEP411: Missing use-case: Monitoring / restricting libraries
On 18/05/2021 23:06, David Black wrote:
I don't think that this thinking is unique but it might not be worth
the "cost" to Oracle to maintain something that seemingly for various
reasons Oracle has little interest in maintaining (we're not in
applet-land anymore). I would like to encourage proposals that mean
that people who want to do 4, who implement further security hardening
where others seemingly shy away, can continue to do 4.
Please don't do that. The cost Ron is talking about is not to "Oracle".
It is a cost to the OpenJDK project as a whole.
Likewise, the lack of project team interest in maintaining the security
manager and self-evident interest in applying resources to providing
other, more valuable Java capabilities is not simply restricted to
"Oracle" project members.
regards,
Andrew Dinn
---
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill
Re: [External] : Re: JEP411: Missing use-case: Monitoring / restricting libraries
On 18/05/2021 10:21 am, Ron Pressler wrote: On 18 May 2021, at 01:11, Peter Firmstone wrote: Your ideas are great in theory, in practice, the problem with your Agent proposal is every JVM release needs to be reviewed, and we have to review Java's internal implementation code, and understand it in order to instrument it. Absolutely. But that is exactly the work OpenJDK maintainers are required to do today to support something most people want better alternatives for at the expense of those better alternatives and other work. The Principle of Least Privilege reduces the consequences resulting from attacks that penetrate external security defenses, whether by social engineering, failure to sanitize input, protocols, or platform vulnerabilities. After many years of pain, JDK development work has not been wasted, I for one am grateful for the development efforts and thoughts put into securing the JVM. It is expensive and now your employer has decided they no longer wish to carry this expense, pushing it downstream. Java has a proven and well tested security API, it's flaws are generally well understood. At sometime in the future, Java's internal security defenses will be removed, and new code will no longer perform permission checks, so any breaches of external defenses will result in a JVM being Pwned. After considering the proposals for new Security API, I have decided that the Java 1.8 to Java 1.17 versions will be the most suitable as it has the best well developed and understood security architecture, requiring little further development work. The new proposals arising from this JEP present a significant development upgrade cost, and these new API's that we need to use for building new security architecture around, won't be battle hardened, are experimental and haven't passed the test of time. My assessment is the cost to upgrade is too high and far too great a risk in this instance, the least cost and safest option is to stay with well proven, deployment tested, high performance, hardened existing API's. I wish the OpenJDK project success in their future endeavors. We will try to support later Java versions in Service implementations, similar to other languages, however these will not be able to consume other services. This may enable users to take advantage of later language features. However we will always require the use of Java 1.8 to 1.17 as service consumers. I am grateful for the efforts, which appear to be thought of as wasted efforts or bad money, however this time spent addressing security issues, will leave the series of Java versions from 1.8 to 1.17 as some of the most secure versions of any software platform, as an asset of significant value to those who value security. Thank you. Peter Firmstone.
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:38:03 GMT, Сергей Цыпанов wrote: >> Non-static classes hold a link to their parent classes, which in many cases >> can be avoided. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8261880: Remove static from declarations of Holder nested classes Marked as reviewed by redestad (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 04:22:32 GMT, Phil Race wrote: >> That is unfortunate, but nonetheless I think required to be done. >> We have acknowledeged this can't reasonably be called an RFE, so the JEP is >> introducing bugs/technical debt/call it what you will. This should generally >> be part of a sandbox for the JEP and fixed before integration of the JEP. >> From my point of view it is a blocker. > > The JEP isn't PTT for 17 so there's plenty of time isn't there ? There are 827 files in this patch. Phil is right that adding SW at the class level is introducing technical debt but if addressing that requires refactoring and re-testing of AWT or font code then it would be better to have someone from that area working on. Is there any reason why this can't be going on now on awt-dev/2d-dev and integrated immediately after JEP 411? I don't think we should put Max through the wringer here as there are too many areas to cover. - PR: https://git.openjdk.java.net/jdk/pull/4073
