Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Fri, 16 Oct 2020 02:30:55 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/sun/security/pkcs/PKCS7.java line 818:
>>
>>> 816: DerOutputStream derSigAlg = new DerOutputStream();
>>> 817: sigAlgID.derEncode(derSigAlg);
>>> 818: derAlgs.writeImplicit((byte)0xA1, derSigAlg);
>>
>> Are you sure that this context specific tag value is implicit? In RFC 6211,
>> some other ASN.1 definition uses IMPLICIT
>> keyword after the [x] which seems to suggest that the default is explicit
>> unless specified. Besides, the layman's guide
>> sec2.3 also states "The keyword [class number] alone is the same as explicit
>> tagging, except when the "module" in which
>> the ASN.1 type is defined has implicit tagging by default." So, it seems
>> that explicit tagging should be the default?
>
> In the formal definition at https://tools.ietf.org/html/rfc6211#appendix-A,
> you can see `DEFINITIONS IMPLICIT TAGS`
> covers from BEGIN to END. Those explicit IMPLICIT tags you see are CMS ASN.1
> definitions, and it looks in its own RFC
> at https://tools.ietf.org/html/rfc5652#section-12, IMPLICIT and EXPLICIT are
> always written out. I can confirm both
> OpenSSL and BC use IMPLICIT.
Ah, I see. There is a line about implicit tags as you pointed out. Good~
>> src/java.base/share/classes/sun/security/pkcs/PKCS7.java line 172:
>>
>>> 170: throws IOException
>>> 171: {
>>> 172: ContentInfo block = new ContentInfo(derin, oldStyle);
>>
>> With this change, i.e. using a local variable instead of setting the field
>> 'contentInfo', the 'contentInfo' field seems
>> to left unset when contentType equals to
>> ContentInfo.NETSCAPE_CERT_SEQUENCE_OID?
>
> I'll see what the best code is, but I don't like the way contentInfo is
> assigned twice, once as the whole block and
> once as the content inside. I'd rather add a `contentInfo = block` in its
> else if block.
Right, I also dislike the double assignment. Just making sure that contentInfo
is set somewhere.
-
PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Fri, 16 Oct 2020 02:34:35 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/sun/security/pkcs/SignerInfo.java line 549:
>>
>>> 547: return encAlg;
>>> 548: default:
>>> 549: String digAlg = digAlgId.getName().replace("-", "");
>>
>> This may be incorrect if the digest algorithm is in the SHA3 family. Maybe
>> we should check and apply this conversion
>> only when digest algorithm starts with "SHA-".
>
> Good suggestion. I'll also try some tests.
In fact, since now I directly write the signature algorithm into the
`SignerInfo.digestEncryptionAlgorithmId` field,
the code above is not used at all. The `makeSigAlg` method directly returns the
`encAlgId` argument if it has "with"
inside.
I'll fix it anyway. I've confirmed that if I still write only the key algorithm
there (Ex: "EC") then the verification
process will see a problem without your suggested change.
-
PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Fri, 16 Oct 2020 02:15:08 GMT, Valerie Peng wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> signing time, jarsigner -directsign, and digest algorithm check > > test/lib/jdk/test/lib/security/timestamp/TsaSigner.java line 221: > >> 219: new X500Name(issuerName), >> 220: signerEntry.cert.getSerialNumber(), >> 221: >> AlgorithmId.get(SignatureUtil.extractDigestAlgFromDwithE(sigAlgo)), > > So, sigAlgo would never be RSASSA-PSS, EDDSA, ED25519, or ED448? There were no getDigestAlgInPkcs7SignerInfo in my initial version. I'll use this new method instead. I'll probably need to try if the new algorithms really work here. - PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Thu, 15 Oct 2020 20:42:30 GMT, Valerie Peng wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> signing time, jarsigner -directsign, and digest algorithm check
>
> src/java.base/share/classes/sun/security/pkcs/SignerInfo.java line 549:
>
>> 547: return encAlg;
>> 548: default:
>> 549: String digAlg = digAlgId.getName().replace("-", "");
>
> This may be incorrect if the digest algorithm is in the SHA3 family. Maybe we
> should check and apply this conversion
> only when digest algorithm starts with "SHA-".
Good suggestion. I'll also try some tests.
-
PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Thu, 15 Oct 2020 02:03:13 GMT, Valerie Peng wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> signing time, jarsigner -directsign, and digest algorithm check > > src/java.base/share/classes/sun/security/pkcs/PKCS7.java line 818: > >> 816: DerOutputStream derSigAlg = new DerOutputStream(); >> 817: sigAlgID.derEncode(derSigAlg); >> 818: derAlgs.writeImplicit((byte)0xA1, derSigAlg); > > Are you sure that this context specific tag value is implicit? In RFC 6211, > some other ASN.1 definition uses IMPLICIT > keyword after the [x] which seems to suggest that the default is explicit > unless specified. Besides, the layman's guide > sec2.3 also states "The keyword [class number] alone is the same as explicit > tagging, except when the "module" in which > the ASN.1 type is defined has implicit tagging by default." So, it seems that > explicit tagging should be the default? In the formal definition at https://tools.ietf.org/html/rfc6211#appendix-A, you can see `DEFINITIONS IMPLICIT TAGS` covers from BEGIN to END. Those explicit IMPLICIT tags you see are CMS ASN.1 definitions, and it looks in its own RFC at https://tools.ietf.org/html/rfc5652#section-12, IMPLICIT and EXPLICIT are always written out. I can confirm both OpenSSL and BC use IMPLICIT. - PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Wed, 14 Oct 2020 19:18:04 GMT, Valerie Peng wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> signing time, jarsigner -directsign, and digest algorithm check
>
> src/java.base/share/classes/sun/security/pkcs/PKCS7.java line 172:
>
>> 170: throws IOException
>> 171: {
>> 172: ContentInfo block = new ContentInfo(derin, oldStyle);
>
> With this change, i.e. using a local variable instead of setting the field
> 'contentInfo', the 'contentInfo' field seems
> to left unset when contentType equals to
> ContentInfo.NETSCAPE_CERT_SEQUENCE_OID?
I'll see what the best code is, but I don't like the way contentInfo is
assigned twice, once as the whole block and
once as the content inside. I'd rather add a `contentInfo = block` in its else
if block.
-
PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Wed, 14 Oct 2020 05:31:33 GMT, Valerie Peng wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> signing time, jarsigner -directsign, and digest algorithm check > > src/java.base/share/classes/sun/security/util/SignatureUtil.java line 218: > >> 216: * >> 217: * @param signer Signature object that tells you RSASA-PSS params >> 218: * @param sigalg Signature algorithm tells you who with who > > who with who? Maybe I was thinking about SHA1withRSA, but here it can be the new algorithms. Just remove the confusing words. - PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Wed, 14 Oct 2020 03:51:23 GMT, Weijun Wang wrote: >> Major points in CSR at https://bugs.openjdk.java.net/browse/JDK-8245274: >> >> - new sigalg "RSASSA-PSS", "EdDSA", "Ed25519" and "Ed448" can be used in >> jarsigner >> >> - The ".RSA" and ".EC" block extension types (PKCS #7 SignedData inside a >> signed JAR) are reused for new signature >> algorithms >> >> - A new JarSigner property "directsign" >> >> - Updating the jarsigner tool doc >> >> Major code changes: >> >> - Always use the signature algorithm directly as >> SignerInfo::signatureAlgorithm. We used to use the encryption algorithm >> there like RSA, DSA, and EC. Now it's always SHA1withRSA or RSASSA-PSS. >> >> - Move signature related utilities methods from AlgorithmId.java to >> SignatureUtil.java >> >> - Add new SignatureUtil methods fromKey() and fromSignature() to simplify >> creating Signature and getting its AlgorithmId >> >> - Use the new methods in PKCS10, X509CertImpl, and X509CRLImpl signing >> >> - Add a new (and intuitive, IMHO) PKCS7::generateNewSignedData capable of >> all old and new signature algorithms >> >> - Mark all -altsign related code deprecated and they can be removed once >> ContentSigner is removed > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > signing time, jarsigner -directsign, and digest algorithm check test/lib/jdk/test/lib/security/timestamp/TsaSigner.java line 221: > 219: new X500Name(issuerName), > 220: signerEntry.cert.getSerialNumber(), > 221: > AlgorithmId.get(SignatureUtil.extractDigestAlgFromDwithE(sigAlgo)), So, sigAlgo would never be RSASSA-PSS, EDDSA, ED25519, or ED448? - PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Wed, 14 Oct 2020 04:03:46 GMT, Valerie Peng wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> signing time, jarsigner -directsign, and digest algorithm check
>
> src/java.base/share/classes/sun/security/util/KnownOIDs.java line 147:
>
>> 145: SHAKE128("2.16.840.1.101.3.4.2.11"),
>> 146: SHAKE256("2.16.840.1.101.3.4.2.12"),
>> 147: SHAKE256_LEN("2.16.840.1.101.3.4.2.18", "SHAKE256-LEN"),
>
> Can we move this down a little? The ordering within the section is based on
> the oid value. It's easier to check for
> unlisted/unsupported oid value this way. Why not also add SHAKE128_LEN?
I'll add one.
-
PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Wed, 14 Oct 2020 03:51:23 GMT, Weijun Wang wrote:
>> Major points in CSR at https://bugs.openjdk.java.net/browse/JDK-8245274:
>>
>> - new sigalg "RSASSA-PSS", "EdDSA", "Ed25519" and "Ed448" can be used in
>> jarsigner
>>
>> - The ".RSA" and ".EC" block extension types (PKCS #7 SignedData inside a
>> signed JAR) are reused for new signature
>> algorithms
>>
>> - A new JarSigner property "directsign"
>>
>> - Updating the jarsigner tool doc
>>
>> Major code changes:
>>
>> - Always use the signature algorithm directly as
>> SignerInfo::signatureAlgorithm. We used to use the encryption algorithm
>> there like RSA, DSA, and EC. Now it's always SHA1withRSA or RSASSA-PSS.
>>
>> - Move signature related utilities methods from AlgorithmId.java to
>> SignatureUtil.java
>>
>> - Add new SignatureUtil methods fromKey() and fromSignature() to simplify
>> creating Signature and getting its AlgorithmId
>>
>> - Use the new methods in PKCS10, X509CertImpl, and X509CRLImpl signing
>>
>> - Add a new (and intuitive, IMHO) PKCS7::generateNewSignedData capable of
>> all old and new signature algorithms
>>
>> - Mark all -altsign related code deprecated and they can be removed once
>> ContentSigner is removed
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> signing time, jarsigner -directsign, and digest algorithm check
src/java.base/share/classes/sun/security/pkcs/PKCS7.java line 172:
> 170: throws IOException
> 171: {
> 172: ContentInfo block = new ContentInfo(derin, oldStyle);
With this change, i.e. using a local variable instead of setting the field
'contentInfo', the 'contentInfo' field seems
to left unset when contentType equals to ContentInfo.NETSCAPE_CERT_SEQUENCE_OID?
src/java.base/share/classes/sun/security/pkcs/PKCS7.java line 818:
> 816: DerOutputStream derSigAlg = new DerOutputStream();
> 817: sigAlgID.derEncode(derSigAlg);
> 818: derAlgs.writeImplicit((byte)0xA1, derSigAlg);
Are you sure that this context specific tag value is implicit? In RFC 6211,
some other ASN.1 definition uses IMPLICIT
keyword after the [x] which seems to suggest that the default is explicit
unless specified. Besides, the layman's guide
sec2.3 also states "The keyword [class number] alone is the same as explicit
tagging, except when the "module" in which
the ASN.1 type is defined has implicit tagging by default." So, it seems that
explicit tagging should be the default?
src/java.base/share/classes/sun/security/pkcs/SignerInfo.java line 508:
> 506: * name and the encryption algorithm name inside a PKCS7 SignerInfo.
> 507: *
> 508: * For old style PKCS7 files where we use RSA, DSA, EC asencAlgId
asencAlgId => as encAlgId
src/java.base/share/classes/sun/security/pkcs/SignerInfo.java line 549:
> 547: return encAlg;
> 548: default:
> 549: String digAlg = digAlgId.getName().replace("-", "");
This may be incorrect if the digest algorithm is in the SHA3 family. Maybe we
should check and apply this conversion
only when digest algorithm starts with "SHA-".
src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java line 858:
> 856: byte[] content = baos.toByteArray();
> 857:
> 858: // Use new method is directSign is false or it's a modern
is => if
-
PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Thu, 15 Oct 2020 18:15:35 GMT, Vicente Romero wrote: > this one has nothing to do with javac so the `compiler` label should be > removed @vicente-romero-oracle Sorry for the noise, I should have removed it earlier. All files in the `jdk.jartool` module are under `compiler` in https://github.com/openjdk/skara/blob/master/config/mailinglist/rules/jdk.json#L120. You can make it more specific. - PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Tue, 13 Oct 2020 13:29:39 GMT, Weijun Wang wrote: >> Add support for [RFC 6211: Cryptographic Message Syntax (CMS) Algorithm >> Identifier Protection >> Attribute](https://tools.ietf.org/html/rfc6211) to protect against algorithm >> substitution attacks. This attribute is >> signed and it contains copies of digestAlgorithm and signatureAlgorithm >> which are unprotected in SignerInfo. Before >> this enhancement, the two algorithms can be implied from the signature >> itself (i.e. if you change any of them the >> signature size would not match or the key will not decrypt). However, with >> the introduction of RSASSA-PSS, the >> signature algorithm can be modified and it still looks like the signature is >> valid. This particular case is [described >> in the RFC](https://tools.ietf.org/html/rfc6211#page-5): >>signatureAlgorithm has been protected by implication in the past. >> The use of an RSA public key implied that the RSA v1.5 signature >> algorithm was being used. The hash algorithm and this fact could >> be checked by the internal padding defined. This is no longer >> true with the addition of the RSA-PSS signature algorithms. > > A force push to fix the RFC number typo in the latest commit. No content > update. this one has nothing to do with javac so the `compiler` label should be removed - PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Wed, 14 Oct 2020 03:51:23 GMT, Weijun Wang wrote:
>> Major points in CSR at https://bugs.openjdk.java.net/browse/JDK-8245274:
>>
>> - new sigalg "RSASSA-PSS", "EdDSA", "Ed25519" and "Ed448" can be used in
>> jarsigner
>>
>> - The ".RSA" and ".EC" block extension types (PKCS #7 SignedData inside a
>> signed JAR) are reused for new signature
>> algorithms
>>
>> - A new JarSigner property "directsign"
>>
>> - Updating the jarsigner tool doc
>>
>> Major code changes:
>>
>> - Always use the signature algorithm directly as
>> SignerInfo::signatureAlgorithm. We used to use the encryption algorithm
>> there like RSA, DSA, and EC. Now it's always SHA1withRSA or RSASSA-PSS.
>>
>> - Move signature related utilities methods from AlgorithmId.java to
>> SignatureUtil.java
>>
>> - Add new SignatureUtil methods fromKey() and fromSignature() to simplify
>> creating Signature and getting its AlgorithmId
>>
>> - Use the new methods in PKCS10, X509CertImpl, and X509CRLImpl signing
>>
>> - Add a new (and intuitive, IMHO) PKCS7::generateNewSignedData capable of
>> all old and new signature algorithms
>>
>> - Mark all -altsign related code deprecated and they can be removed once
>> ContentSigner is removed
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> signing time, jarsigner -directsign, and digest algorithm check
src/java.base/share/classes/sun/security/util/SignatureUtil.java line 210:
> 208: new DerValue((byte) 2, new byte[]{2, 0})); //
> int 512
> 209: } catch (IOException | NoSuchAlgorithmException e) {
> 210: throw new AssertionError("Shoudl not happen", e);
Shoudl => Should
src/java.base/share/classes/sun/security/util/SignatureUtil.java line 215:
> 213: }
> 214: /**
> 215: * Determines the digestEncryptionAlgorithmId in PKCS& SignerInfo.
&=>7
src/java.base/share/classes/sun/security/util/SignatureUtil.java line 217:
> 215: * Determines the digestEncryptionAlgorithmId in PKCS& SignerInfo.
> 216: *
> 217: * @param signer Signature object that tells you RSASA-PSS params
RSASA=>RSASSA
src/java.base/share/classes/sun/security/util/SignatureUtil.java line 221:
> 219: * @param privateKey key tells you EdDSA params
> 220: * @param directsign Ed448 uses different digest algs depending on
> this
> 221: * @return the digest alg
alg => algid
src/java.base/share/classes/sun/security/util/SignatureUtil.java line 218:
> 216: *
> 217: * @param signer Signature object that tells you RSASA-PSS params
> 218: * @param sigalg Signature algorithm tells you who with who
who with who?
-
PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
On Wed, 14 Oct 2020 03:51:23 GMT, Weijun Wang wrote:
>> Major points in CSR at https://bugs.openjdk.java.net/browse/JDK-8245274:
>>
>> - new sigalg "RSASSA-PSS", "EdDSA", "Ed25519" and "Ed448" can be used in
>> jarsigner
>>
>> - The ".RSA" and ".EC" block extension types (PKCS #7 SignedData inside a
>> signed JAR) are reused for new signature
>> algorithms
>>
>> - A new JarSigner property "directsign"
>>
>> - Updating the jarsigner tool doc
>>
>> Major code changes:
>>
>> - Always use the signature algorithm directly as
>> SignerInfo::signatureAlgorithm. We used to use the encryption algorithm
>> there like RSA, DSA, and EC. Now it's always SHA1withRSA or RSASSA-PSS.
>>
>> - Move signature related utilities methods from AlgorithmId.java to
>> SignatureUtil.java
>>
>> - Add new SignatureUtil methods fromKey() and fromSignature() to simplify
>> creating Signature and getting its AlgorithmId
>>
>> - Use the new methods in PKCS10, X509CertImpl, and X509CRLImpl signing
>>
>> - Add a new (and intuitive, IMHO) PKCS7::generateNewSignedData capable of
>> all old and new signature algorithms
>>
>> - Mark all -altsign related code deprecated and they can be removed once
>> ContentSigner is removed
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> signing time, jarsigner -directsign, and digest algorithm check
src/java.base/share/classes/sun/security/util/KnownOIDs.java line 147:
> 145: SHAKE128("2.16.840.1.101.3.4.2.11"),
> 146: SHAKE256("2.16.840.1.101.3.4.2.12"),
> 147: SHAKE256_LEN("2.16.840.1.101.3.4.2.18", "SHAKE256-LEN"),
Can we move this down a little? The ordering within the section is based on the
oid value. It's easier to check for
unlisted/unsupported oid value this way. Why not also add SHAKE128_LEN?
-
PR: https://git.openjdk.java.net/jdk/pull/322
Re: RFR: 8242068: Signed JAR support for RSASSA-PSS and EdDSA [v7]
> Major points in CSR at https://bugs.openjdk.java.net/browse/JDK-8245274: > > - new sigalg "RSASSA-PSS", "EdDSA", "Ed25519" and "Ed448" can be used in > jarsigner > > - The ".RSA" and ".EC" block extension types (PKCS #7 SignedData inside a > signed JAR) are reused for new signature > algorithms > > - A new JarSigner property "directsign" > > - Updating the jarsigner tool doc > > Major code changes: > > - Always use the signature algorithm directly as > SignerInfo::signatureAlgorithm. We used to use the encryption algorithm > there like RSA, DSA, and EC. Now it's always SHA1withRSA or RSASSA-PSS. > > - Move signature related utilities methods from AlgorithmId.java to > SignatureUtil.java > > - Add new SignatureUtil methods fromKey() and fromSignature() to simplify > creating Signature and getting its AlgorithmId > > - Use the new methods in PKCS10, X509CertImpl, and X509CRLImpl signing > > - Add a new (and intuitive, IMHO) PKCS7::generateNewSignedData capable of all > old and new signature algorithms > > - Mark all -altsign related code deprecated and they can be removed once > ContentSigner is removed Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: signing time, jarsigner -directsign, and digest algorithm check - Changes: - all: https://git.openjdk.java.net/jdk/pull/322/files - new: https://git.openjdk.java.net/jdk/pull/322/files/ffaae532..734fd034 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=322&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=322&range=05-06 Stats: 53 lines in 5 files changed: 42 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/322.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/322/head:pull/322 PR: https://git.openjdk.java.net/jdk/pull/322
