RFR: 8258915: Temporary buffer cleanup

classic Classic list List threaded Threaded
50 messages Options
123
Reply | Threaded
Open this post in threaded view
|

RFR: 8258915: Temporary buffer cleanup

Weijun Wang-2
Clean up temporary byte array, char array, and keyspec around keys and passwords.

No new regression test.

-------------

Commit messages:
 - 8258915: More temporary buffer cleanup

Changes: https://git.openjdk.java.net/jdk/pull/2070/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2070&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8258915
  Stats: 1571 lines in 51 files changed: 731 ins; 428 del; 412 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2070.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2070/head:pull/2070

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup

Rajan Halade-2
On Wed, 13 Jan 2021 21:32:07 GMT, Weijun Wang <[hidden email]> wrote:

> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>
> No new regression test.

please add noreg label to the JBS bug.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup

Weijun Wang-2
On Wed, 13 Jan 2021 22:19:00 GMT, Rajan Halade <[hidden email]> wrote:

> please add noreg label to the JBS bug.

Added. Thanks.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v2]

Weijun Wang-2
In reply to this post by Weijun Wang-2
> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>
> No new regression test.

Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:

 - rsa
 - Merge
 - 8258915: More temporary buffer cleanup
   
   8258915: More temporary buffer cleanup

-------------

Changes: https://git.openjdk.java.net/jdk/pull/2070/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2070&range=01
  Stats: 1666 lines in 53 files changed: 795 ins; 429 del; 442 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2070.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2070/head:pull/2070

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup

Weijun Wang-2
In reply to this post by Weijun Wang-2
On Thu, 14 Jan 2021 02:52:08 GMT, Weijun Wang <[hidden email]> wrote:

>> please add noreg label to the JBS bug.
>
>> please add noreg label to the JBS bug.
>
> Added. Thanks.

Just pushed a new commit. First I merged into the RSA PKCS #1 code change from @valeriepeng, and then reapplied the cleanup to `RSAKeyFactory`. Most new code change is inside `RSAPrivateCrtKeyImpl`. I also updated `DerValue` and `DerOutpuStream` a little to avoid unnecessary array copying.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v2]

Valerie Peng-2
In reply to this post by Weijun Wang-2
On Thu, 21 Jan 2021 16:23:21 GMT, Weijun Wang <[hidden email]> wrote:

>> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>>
>> No new regression test.
>
> Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - rsa
>  - Merge
>  - 8258915: More temporary buffer cleanup
>    
>    8258915: More temporary buffer cleanup

src/java.base/share/classes/com/sun/crypto/provider/PBKDF2Core.java line 163:

> 161:                     }
> 162:                     Arrays.fill(encoding, (byte)0);
> 163:                     spec.clearPassword();

nit: move to inside the if-check block above?

src/java.base/share/classes/com/sun/crypto/provider/PBKDF2HmacSHA1Factory.java line 163:

> 161:                     }
> 162:                     Arrays.fill(encoding, (byte)0);
> 163:                     spec.clearPassword();

same nit: move to inside of the if-check block.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v2]

Valerie Peng-2
In reply to this post by Weijun Wang-2
On Thu, 21 Jan 2021 16:23:21 GMT, Weijun Wang <[hidden email]> wrote:

>> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>>
>> No new regression test.
>
> Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - rsa
>  - Merge
>  - 8258915: More temporary buffer cleanup
>    
>    8258915: More temporary buffer cleanup

src/java.base/share/classes/com/sun/crypto/provider/PrivateKeyInfo.java line 98:

> 96:
> 97:     public void clear() {
> 98:         Arrays.fill(privkey, (byte)0);

check for null just in case?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v2]

Valerie Peng-2
In reply to this post by Weijun Wang-2
On Thu, 21 Jan 2021 16:23:21 GMT, Weijun Wang <[hidden email]> wrote:

>> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>>
>> No new regression test.
>
> Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - rsa
>  - Merge
>  - 8258915: More temporary buffer cleanup
>    
>    8258915: More temporary buffer cleanup

src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 234:

> 232:                 encodedKey = out.toByteArray();
> 233:                 // Copy the actual bytes
> 234:                 System.arraycopy(key, 0, encodedKey, encodedKey.length - key.length, key.length);

I think this can now be updated with the newer DerValue.wrap(....) then DerValue.clear() approach?

src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 676:

> 674:                 // effectively once required.
> 675:                 secretKeyInfo.putOctetString(new byte[encoded.length]);
> 676:                 pkcs8.write(DerValue.tag_Sequence, secretKeyInfo);

Same as earlier comment - can switch to DerValue.wrap() and then clear() approach?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v2]

Valerie Peng-2
In reply to this post by Weijun Wang-2
On Thu, 21 Jan 2021 16:23:21 GMT, Weijun Wang <[hidden email]> wrote:

>> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>>
>> No new regression test.
>
> Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - rsa
>  - Merge
>  - 8258915: More temporary buffer cleanup
>    
>    8258915: More temporary buffer cleanup

src/java.base/share/classes/sun/security/rsa/RSAPrivateCrtKeyImpl.java line 233:

> 231:             DerValue val = DerValue.wrap(DerValue.tag_Sequence, out);
> 232:             key = val.toByteArray();
> 233:             val.clear();

Update RSAPrivateKeyImpl.java also for consistency sake?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v2]

Weijun Wang-2
In reply to this post by Valerie Peng-2
On Fri, 22 Jan 2021 07:18:22 GMT, Valerie Peng <[hidden email]> wrote:

>> Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>>
>>  - rsa
>>  - Merge
>>  - 8258915: More temporary buffer cleanup
>>    
>>    8258915: More temporary buffer cleanup
>
> src/java.base/share/classes/com/sun/crypto/provider/PrivateKeyInfo.java line 98:
>
>> 96:
>> 97:     public void clear() {
>> 98:         Arrays.fill(privkey, (byte)0);
>
> check for null just in case?

`val.data.getOctetString()` on line 82 should never return null, so I think a null check is not necessary here.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v2]

Weijun Wang-2
In reply to this post by Valerie Peng-2
On Fri, 22 Jan 2021 07:44:41 GMT, Valerie Peng <[hidden email]> wrote:

>> Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>>
>>  - rsa
>>  - Merge
>>  - 8258915: More temporary buffer cleanup
>>    
>>    8258915: More temporary buffer cleanup
>
> src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 234:
>
>> 232:                 encodedKey = out.toByteArray();
>> 233:                 // Copy the actual bytes
>> 234:                 System.arraycopy(key, 0, encodedKey, encodedKey.length - key.length, key.length);
>
> I think this can now be updated with the newer DerValue.wrap(....) then DerValue.clear() approach?

Good idea!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v2]

Valerie Peng-2
In reply to this post by Weijun Wang-2
On Fri, 22 Jan 2021 14:35:46 GMT, Weijun Wang <[hidden email]> wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/PrivateKeyInfo.java line 98:
>>
>>> 96:
>>> 97:     public void clear() {
>>> 98:         Arrays.fill(privkey, (byte)0);
>>
>> check for null just in case?
>
> `val.data.getOctetString()` on line 82 should never return null, so I think a null check is not necessary here.

Sure.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v2]

Weijun Wang-2
In reply to this post by Valerie Peng-2
On Fri, 22 Jan 2021 08:18:15 GMT, Valerie Peng <[hidden email]> wrote:

>> Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>>
>>  - rsa
>>  - Merge
>>  - 8258915: More temporary buffer cleanup
>>    
>>    8258915: More temporary buffer cleanup
>
> src/java.base/share/classes/sun/security/rsa/RSAPrivateCrtKeyImpl.java line 233:
>
>> 231:             DerValue val = DerValue.wrap(DerValue.tag_Sequence, out);
>> 232:             key = val.toByteArray();
>> 233:             val.clear();
>
> Update RSAPrivateKeyImpl.java also for consistency sake?

Good idea.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v3]

Weijun Wang-2
In reply to this post by Weijun Wang-2
> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>
> No new regression test.

Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:

  more wrap, less copy

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2070/files
  - new: https://git.openjdk.java.net/jdk/pull/2070/files/7129df6a..b7216061

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2070&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2070&range=01-02

  Stats: 50 lines in 7 files changed: 12 ins; 23 del; 15 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2070.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2070/head:pull/2070

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup

Weijun Wang-2
In reply to this post by Weijun Wang-2
On Thu, 21 Jan 2021 16:25:13 GMT, Weijun Wang <[hidden email]> wrote:

>>> please add noreg label to the JBS bug.
>>
>> Added. Thanks.
>
> Just pushed a new commit. First I merged into the RSA PKCS #1 code change from @valeriepeng, and then reapplied the cleanup to `RSAKeyFactory`. Most new code change is inside `RSAPrivateCrtKeyImpl`. I also updated `DerValue` and `DerOutpuStream` a little to avoid unnecessary array copying.

New commit. However, I was writing test to detect leak in `RSAPrivateKeySpec` conversion and add more key->spec->key for other algorithms and find more leaks. Will fix in another commit.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v4]

Weijun Wang-2
In reply to this post by Weijun Wang-2
> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>
> No new regression test.

Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:

  keyfactory operations on own keyspec

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2070/files
  - new: https://git.openjdk.java.net/jdk/pull/2070/files/b7216061..9b17b1a2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2070&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2070&range=02-03

  Stats: 57 lines in 7 files changed: 34 ins; 2 del; 21 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2070.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2070/head:pull/2070

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup

Weijun Wang-2
In reply to this post by Weijun Wang-2
On Fri, 22 Jan 2021 15:39:53 GMT, Weijun Wang <[hidden email]> wrote:

>> Just pushed a new commit. First I merged into the RSA PKCS #1 code change from @valeriepeng, and then reapplied the cleanup to `RSAKeyFactory`. Most new code change is inside `RSAPrivateCrtKeyImpl`. I also updated `DerValue` and `DerOutpuStream` a little to avoid unnecessary array copying.
>
> New commit. However, I was writing test to detect leak in `RSAPrivateKeySpec` conversion and add more key->spec->key for other algorithms and find more leaks. Will fix in another commit.

Another new commit. Last time I only fixed KeySpec on PKCS8 format. This time it's for an algorithm's _own_ spec.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v3]

Valerie Peng-2
In reply to this post by Weijun Wang-2
On Fri, 22 Jan 2021 15:43:05 GMT, Weijun Wang <[hidden email]> wrote:

>> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>>
>> No new regression test.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>
>   more wrap, less copy

src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 221:

> 219:         if (encodedKey == null) {
> 220:             try {
> 221:                 DerOutputStream tmp = new DerOutputStream();

What is the criteria of using the default constructor vs the one with a initial size? Here is using the default, are we sure about the key (line 224 below) will always fit?

src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 664:

> 662:
> 663:                 // Encode secret key in a PKCS#8
> 664:                 DerOutputStream secretKeyInfo = new DerOutputStream();

Same, using default constructor here and we write encodedKey into it at line 670 below.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v4]

Valerie Peng-2
In reply to this post by Weijun Wang-2
On Fri, 22 Jan 2021 21:28:53 GMT, Weijun Wang <[hidden email]> wrote:

>> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>>
>> No new regression test.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>
>   keyfactory operations on own keyspec

src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java line 116:

> 114:             encode();
> 115:         } catch (IOException e) {
> 116:             throw new ProviderException("Cannot produce ASN.1 encoding", e);

Supposedly the IOException should never happen? Otherwise the Arrays.fill(...) call may not happen.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258915: Temporary buffer cleanup [v4]

Valerie Peng-2
In reply to this post by Weijun Wang-2
On Fri, 22 Jan 2021 21:28:53 GMT, Weijun Wang <[hidden email]> wrote:

>> Clean up temporary byte array, char array, and keyspec around keys and passwords.
>>
>> No new regression test.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>
>   keyfactory operations on own keyspec

src/jdk.crypto.ec/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 133:

> 131:             out.putInteger(1); // version 1
> 132:             out.putOctetString(sOctets);
> 133:             Arrays.fill(sOctets, (byte)0);

The same handling should apply to line 106 above inside makeEncoding(byte[])?

src/jdk.crypto.ec/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 136:

> 134:             DerValue val = DerValue.wrap(DerValue.tag_Sequence, out);
> 135:             key = val.toByteArray();
> 136:             val.clear();

Same handling should apply to line 107-109 above inside makeEncoding(byte[])?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2070
123