Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v2]
On Fri, 25 Sep 2020 00:45:09 GMT, Weijun Wang wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updated test case to use DerUtils
>
> src/java.base/share/classes/sun/security/x509/AlgorithmId.java line 214:
>
>> 212: || algid.equals(SHA512withECDSA_oid)) {
>> 213: // RFC 4055 3.3: when an RSASSA-PSS key does not require
>> 214: // parameter validation, field is absent.
>
> Would you like to add more comments on other RFCs here?
Added the comments for RFC reference.
-
PR: https://git.openjdk.java.net/jdk/pull/312
Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v3]
> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the > parameters field instead of encoding a > Null tag. Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: Added comment for RFC - Changes: - all: https://git.openjdk.java.net/jdk/pull/312/files - new: https://git.openjdk.java.net/jdk/pull/312/files/edc71d03..4f4e3688 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=312=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=312=01-02 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/312.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/312/head:pull/312 PR: https://git.openjdk.java.net/jdk/pull/312
Integrated: 8235710: Remove the legacy elliptic curves
On Mon, 21 Sep 2020 21:10:58 GMT, Anthony Scarpino wrote: > This change removes the native elliptic curves library code; as well as, and > calls to that code, tests, and files > associated with those libraries. The makefiles have been changed to remove > from all source builds of the ec code. The > SunEC system property is removed and java.security configurations changed to > reflect the removed curves. This will > remove the following elliptic curves from SunEC: secp112r1, secp112r2, > secp128r1, secp128r2, secp160k1, secp160r1, > secp160r2, secp192k1, secp192r1, secp224k1, secp224r1, secp256k1, sect113r1, > sect113r2, sect131r1, sect131r2, > sect163k1, sect163r1, sect163r2, sect193r1, sect193r2, sect233k1, sect233r1, > sect239k1, sect283k1, sect283r1, > sect409k1, sect409r1, sect571k1, sect571r1, X9.62 c2tnb191v1, X9.62 > c2tnb191v2, X9.62 c2tnb191v3, X9.62 c2tnb239v1, > X9.62 c2tnb239v2, X9.62 c2tnb239v3, X9.62 c2tnb359v1, X9.62 c2tnb431r1, X9.62 > prime192v2, X9.62 prime192v3, X9.62 > prime239v1, X9.62 prime239v2, X9.62 prime239v3, brainpoolP256r1 > brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 This pull request has now been integrated. Changeset: 0b83fc01 Author:Anthony Scarpino URL: https://git.openjdk.java.net/jdk/commit/0b83fc01 Stats: 20155 lines in 77 files changed: 28 ins; 20048 del; 79 mod 8235710: Remove the legacy elliptic curves Reviewed-by: xuelei, erikj - PR: https://git.openjdk.java.net/jdk/pull/289
Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v2]
On Thu, 24 Sep 2020 23:49:08 GMT, Hai-May Chao wrote:
>> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the
>> parameters field instead of encoding a
>> Null tag.
>
> Hai-May Chao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updated test case to use DerUtils
src/java.base/share/classes/sun/security/x509/AlgorithmId.java line 214:
> 212: || algid.equals(SHA512withECDSA_oid)) {
> 213: // RFC 4055 3.3: when an RSASSA-PSS key does not require
> 214: // parameter validation, field is absent.
Would you like to add more comments on other RFCs here?
-
PR: https://git.openjdk.java.net/jdk/pull/312
Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v2]
> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the > parameters field instead of encoding a > Null tag. Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: Updated test case to use DerUtils - Changes: - all: https://git.openjdk.java.net/jdk/pull/312/files - new: https://git.openjdk.java.net/jdk/pull/312/files/f52268a7..edc71d03 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=312=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=312=00-01 Stats: 189 lines in 1 file changed: 35 ins; 132 del; 22 mod Patch: https://git.openjdk.java.net/jdk/pull/312.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/312/head:pull/312 PR: https://git.openjdk.java.net/jdk/pull/312
Re: RFR: 8235710: Remove the legacy elliptic curves [v3]
On Thu, 24 Sep 2020 21:37:14 GMT, Weijun Wang wrote: >> jdk.disabled.namedCurves is commented out and I don't think it's good for >> every operation disabled algorithms call to >> check an empty property. The description for the disabledAlgorithm >> properties say you have to use "include", so I >> don't think it is necessary to we keep it active.. > > I just think this is an unnecessary behavior change. After all, the purpose > of `jdk.disabled.namedCurves` is to be > included in other disabledAlgorithms properties. > No strong opinion on this. Please decide yourself. I understand what you are saying. The property only existed because there were so many curves that would have overwhelmed the disabledAlgorithms. I also don't like making this a permanent addition to the disabledAlgorithm properties. It's possible we may remove the property in the future as it's likely unnecessary going forward. - PR: https://git.openjdk.java.net/jdk/pull/289
Re: RFR: 8235710: Remove the legacy elliptic curves [v3]
On Thu, 24 Sep 2020 21:15:34 GMT, Anthony Scarpino wrote: >> src/java.base/share/conf/security/java.security line 636: >> >>> 634: # >>> 635: jdk.certpath.disabledAlgorithms=MD2, MD5, SHA1 jdkCA & usage >>> TLSServer, \ >>> 636: RSA keySize < 1024, DSA keySize < 1024, EC keySize < 224 >> >> `jdk.disabled.namedCurves` still exists. If someone decides to add a curve >> there, shouldn't it be also disabled here? > > jdk.disabled.namedCurves is commented out and I don't think it's good for > every operation disabled algorithms call to > check an empty property. The description for the disabledAlgorithm > properties say you have to use "include", so I > don't think it is necessary to we keep it active.. I just think this is an unnecessary behavior change. After all, the purpose of `jdk.disabled.namedCurves` is to be included in other disabledAlgorithms properties. No strong opinion on this. Please decide yourself. - PR: https://git.openjdk.java.net/jdk/pull/289
Re: RFR: 8235710: Remove the legacy elliptic curves [v3]
On Thu, 24 Sep 2020 19:48:45 GMT, Weijun Wang wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> change exception for ec keyagreement >> fix supportedcurves in SunEC > > src/java.base/share/conf/security/java.security line 636: > >> 634: # >> 635: jdk.certpath.disabledAlgorithms=MD2, MD5, SHA1 jdkCA & usage TLSServer, >> \ >> 636: RSA keySize < 1024, DSA keySize < 1024, EC keySize < 224 > > `jdk.disabled.namedCurves` still exists. If someone decides to add a curve > there, shouldn't it be also disabled here? jdk.disabled.namedCurves is commented out and I don't think it's good for every operation disabled algorithms call to check an empty property. The description for the disabledAlgorithm properties say you have to use "include", so I don't think it is necessary to we keep it active.. - PR: https://git.openjdk.java.net/jdk/pull/289
Re: RFR: 8235710: Remove the legacy elliptic curves [v3]
On Wed, 23 Sep 2020 23:38:03 GMT, Anthony Scarpino wrote: >> This change removes the native elliptic curves library code; as well as, and >> calls to that code, tests, and files >> associated with those libraries. The makefiles have been changed to remove >> from all source builds of the ec code. The >> SunEC system property is removed and java.security configurations changed to >> reflect the removed curves. This will >> remove the following elliptic curves from SunEC: secp112r1, secp112r2, >> secp128r1, secp128r2, secp160k1, secp160r1, >> secp160r2, secp192k1, secp192r1, secp224k1, secp224r1, secp256k1, sect113r1, >> sect113r2, sect131r1, sect131r2, >> sect163k1, sect163r1, sect163r2, sect193r1, sect193r2, sect233k1, sect233r1, >> sect239k1, sect283k1, sect283r1, >> sect409k1, sect409r1, sect571k1, sect571r1, X9.62 c2tnb191v1, X9.62 >> c2tnb191v2, X9.62 c2tnb191v3, X9.62 c2tnb239v1, >> X9.62 c2tnb239v2, X9.62 c2tnb239v3, X9.62 c2tnb359v1, X9.62 c2tnb431r1, >> X9.62 prime192v2, X9.62 prime192v3, X9.62 >> prime239v1, X9.62 prime239v2, X9.62 prime239v3, brainpoolP256r1 >> brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 > > Anthony Scarpino has updated the pull request incrementally with one > additional commit since the last revision: > > change exception for ec keyagreement > fix supportedcurves in SunEC src/java.base/share/conf/security/java.security line 636: > 634: # > 635: jdk.certpath.disabledAlgorithms=MD2, MD5, SHA1 jdkCA & usage TLSServer, \ > 636: RSA keySize < 1024, DSA keySize < 1024, EC keySize < 224 `jdk.disabled.namedCurves` still exists. If someone decides to add a curve there, shouldn't it be also disabled here? - PR: https://git.openjdk.java.net/jdk/pull/289
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException
On Wed, 23 Sep 2020 15:06:55 GMT, Jaikiran Pai wrote:
> Can I please get a review and a sponsor for a fix for
> https://bugs.openjdk.java.net/browse/JDK-8242882?
>
> As noted in that JBS issue, if the size of the Manifest entry in the jar
> happens to be very large (such that it exceeds
> the `Integer.MAX_VALUE`), then the current code in `JarFile#getBytes` can
> lead to a `NegativeArraySizeException`. This
> is due to the: if (len != -1 && len <= 65535) block which evaluates to
> `true` when the size of the manifest entry is
> larger than `Integer.MAX_VALUE`. As a result, this then ends up calling the
> code which can lead to the
> `NegativeArraySizeException`. The commit in this PR fixes that issue by
> changing those `if/else` blocks to prevent
> this issue and instead use a code path that leads to the
> `InputStream#readAllBytes()` which internally has the
> necessary checks to throw the expected `OutOfMemoryError`. This commit also
> includes a jtreg test case which
> reproduces the issue and verifies the fix.
Changes requested by bchristi (Reviewer).
src/java.base/share/classes/java/util/jar/JarFile.java line 791:
> 789: private byte[] getBytes(ZipEntry ze) throws IOException {
> 790: try (InputStream is = super.getInputStream(ze)) {
> 791: int len = (int)ze.getSize();
I think the main issue is the casting of the 'long' value from
ZipEntry.getSize() into an 'int'. I think checking if
the size is > the maximum array size and throwing an OOME here might be a
sufficient fix on its own.
-
PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
On 24/09/2020 09:59, Kim Barrett wrote: If possible, my preference would be to avoid the pragma cruft and write the code in such a way that gcc8/9 without asan doesn't warn, and gcc10 doesn't warn with or without asan. I've kind of lost track in the discussion of all the variants whether that's actually feasible. I agree, the program cruft in NetworkInterface.c is unmaintainable and not the right place for it. -Alan.
Re: RFR(S): 8252407: Build failure with gcc-8+ and asan
> On Sep 23, 2020, at 2:10 AM, Eric Liu wrote: > > Hi Kim, > > Sorry for the delay. > > This patch removes a redundant string copy in NetworkInterface.c to avoid > string-truncation > warning. Other warnings we talked before, which are unable to completely fix > in different version > of gcc, I have to use pragma to suppress them as a workaround. > > This patch now could compile with gcc-7, gcc-8, gcc-9, gcc-10 both with or > without asan. > > [TESTS] > Jtreg: hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1. > No new failure found. > > http://cr.openjdk.java.net/~qfeng/ericliu/jdk/stringop_trunc/webrev.01/ > https://bugs.openjdk.java.net/browse/JDK-8252407 If possible, my preference would be to avoid the pragma cruft and write the code in such a way that gcc8/9 without asan doesn't warn, and gcc10 doesn't warn with or without asan. I've kind of lost track in the discussion of all the variants whether that's actually feasible.
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException
Hello Brent, Thank you for sponsoring this change. In the meantime, I triggered the pre-submit GitHub action job to run the "tier1" tests for a duplicate branch of this PR. That completed successfully https://github.com/jaikiran/jdk/actions/runs/269960940. I'll wait for the reviews, before initiating any /integrate command. -Jaikiran On 23/09/20 10:21 pm, Brent Christian wrote: > On Wed, 23 Sep 2020 15:12:58 GMT, Jaikiran Pai wrote: > >>> Can I please get a review and a sponsor for a fix for >>> https://bugs.openjdk.java.net/browse/JDK-8242882? >>> >>> As noted in that JBS issue, if the size of the Manifest entry in the jar >>> happens to be very large (such that it exceeds >>> the `Integer.MAX_VALUE`), then the current code in `JarFile#getBytes` can >>> lead to a `NegativeArraySizeException`. This >>> is due to the: if (len != -1 && len <= 65535) block which evaluates to >>> `true` when the size of the manifest entry is >>> larger than `Integer.MAX_VALUE`. As a result, this then ends up calling the >>> code which can lead to the >>> `NegativeArraySizeException`. The commit in this PR fixes that issue by >>> changing those `if/else` blocks to prevent >>> this issue and instead use a code path that leads to the >>> `InputStream#readAllBytes()` which internally has the >>> necessary checks to throw the expected `OutOfMemoryError`. This commit >>> also includes a jtreg test case which >>> reproduces the issue and verifies the fix. >> I had created a copy of this branch in my personal fork and included the >> `submit.yml` workflow as noted in this recent >> mail here[1] to try and run the `tier1` testing for this change. But it >> looks like that has failed for unrelated >> reasons[2]. So I'll go ahead and trigger the `tier1` tests locally instead. >> [1] >> https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html >> [2] >> https://github.com/jaikiran/jdk/actions/runs/268948812 > Hi, Jaikiran. I can sponsor this change. > > - > > PR: https://git.openjdk.java.net/jdk/pull/323
