Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8242068: Signed JAR support for RSASSA-PSS and EdDSA #322

Closed
wants to merge 8 commits into from

Conversation

@wangweij
Copy link
Contributor

@wangweij wangweij commented Sep 23, 2020

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (3/3 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8242068: Signed JAR support for RSASSA-PSS and EdDSA

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/322/head:pull/322
$ git checkout pull/322

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 23, 2020

👋 Welcome back weijun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 23, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2020

@wangweij The following labels will be automatically applied to this pull request: compiler core-libs security.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 23, 2020

@openjdk
Copy link

@openjdk openjdk bot commented Sep 28, 2020

@wangweij this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8242068
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
test/lib/jdk/test/lib/util/JarUtils.java Outdated Show resolved Hide resolved
@wangweij wangweij force-pushed the wangweij:8242068 branch from c05af70 to 5d455ba Oct 4, 2020
@openjdk openjdk bot removed the merge-conflict label Oct 4, 2020
@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 4, 2020

Note: I force pushed a new commit to correct a typo in the summary line.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 9, 2020

/csr

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 9, 2020

/label remove core-libs

@openjdk openjdk bot added the csr label Oct 9, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 9, 2020

@wangweij this pull request will not be integrated until the CSR request JDK-8245274 for issue JDK-8242068 has been approved.

@openjdk openjdk bot removed the core-libs label Oct 9, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 9, 2020

@wangweij
The core-libs label was successfully removed.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 13, 2020

Add support for RFC 6211: Cryptographic Message Syntax (CMS) Algorithm Identifier Protection Attribute 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:

   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.
@wangweij wangweij force-pushed the wangweij:8242068 branch from 8151390 to ffaae53 Oct 13, 2020
@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 13, 2020

A force push to fix the RFC number typo in the latest commit. No content update.

* @param params (optional) parameters
* @return an AlgorithmParameterSpec object
* @throws ProviderException
*/

This comment has been minimized.

@valeriepeng

valeriepeng Oct 14, 2020

Well, I am a bit unsure about your changes to this method. With your change, it returns default parameter spec (instead of null) when the specified AlgorithmParameters object is null. This may not be desirable for all cases? Existing callers would have to check for (params != null) before calling this method. The javadoc description also seems a bit strange with the to-be-converted AlgorithmParameters object being optional. Maybe add a separate method like getParamSpecWithDefault on top of this method or add a separate boolean argument useDefault?

This comment has been minimized.

@wangweij

wangweij Oct 16, 2020
Author Contributor

I cannot remember why I need to return a default. The only default we current have is for RSASSA-PSS, and in all RSASSA-PSS AlgorithmId for signature I see there is always the params. (When it's for a key the params can be missing). All 3 callers of this method is on a signature AlgorithmId so the params should not be null. I'll remove the default return value and do more testing.

This comment has been minimized.

@valeriepeng

valeriepeng Oct 16, 2020

Sounds good. RSASSA-PSS sig algorithm id always have params, it's required.

* Determines the digestEncryptionAlgorithmId in PKCS& SignerInfo.
*
* @param signer Signature object that tells you RSASA-PSS params
* @param sigalg Signature algorithm tells you who with who

This comment has been minimized.

This comment has been minimized.

@wangweij

wangweij Oct 16, 2020
Author Contributor

Maybe I was thinking about SHA1withRSA, but here it can be the new algorithms. Just remove the confusing words.

@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Oct 15, 2020

/label remove compiler

@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Oct 15, 2020

this one has nothing to do with javac so the compiler label should be removed

@openjdk openjdk bot removed the compiler label Oct 15, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 15, 2020

@vicente-romero-oracle
The compiler label was successfully removed.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 15, 2020

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.

contentInfo = new ContentInfo(derin, oldStyle);
contentType = contentInfo.contentType;
DerValue content = contentInfo.getContent();
ContentInfo block = new ContentInfo(derin, oldStyle);

This comment has been minimized.

@valeriepeng

valeriepeng Oct 15, 2020

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?

This comment has been minimized.

@wangweij

wangweij Oct 16, 2020
Author Contributor

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.

This comment has been minimized.

@valeriepeng

valeriepeng Oct 16, 2020

Right, I also dislike the double assignment. Just making sure that contentInfo is set somewhere.

digAlgID.derEncode(derAlgs);
DerOutputStream derSigAlg = new DerOutputStream();
sigAlgID.derEncode(derSigAlg);
derAlgs.writeImplicit((byte)0xA1, derSigAlg);

This comment has been minimized.

@valeriepeng

valeriepeng Oct 15, 2020

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?

This comment has been minimized.

@wangweij

wangweij Oct 16, 2020
Author Contributor

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.

This comment has been minimized.

@valeriepeng

valeriepeng Oct 16, 2020

Ah, I see. There is a line about implicit tags as you pointed out. Good~

}
return encAlg;
default:
String digAlg = digAlgId.getName().replace("-", "");

This comment has been minimized.

@valeriepeng

valeriepeng Oct 15, 2020

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-".

This comment has been minimized.

@wangweij

wangweij Oct 16, 2020
Author Contributor

Good suggestion. I'll also try some tests.

This comment has been minimized.

@wangweij

wangweij Oct 16, 2020
Author Contributor

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.

@@ -125,7 +125,9 @@ public static void main(String[] args) throws Exception {
iae(()->b1.setProperty("internalsf", "Hello"));
npe(()->b1.setProperty("sectionsonly", null));
iae(()->b1.setProperty("sectionsonly", "OK"));
npe(()->b1.setProperty("altsigner", null));

This comment has been minimized.

@valeriepeng

valeriepeng Oct 16, 2020

Is 'altsigner' support removed? But I saw it being used in later part of this test. The javadoc for JarSigner.Builder only lists a subset of above properties. Are those not in javadoc internal and can be removed any time, just curious? Nit: maybe add 8242068 to @bug line for existing regression tests?

This comment has been minimized.

@wangweij

wangweij Oct 16, 2020
Author Contributor

It's deprecated. I remember we are already thinking about deprecating the altSign mechanism when creating the JarSigner API and therefore we didn't add them to the spec from the beginning.

Bug id added.

@@ -221,8 +218,7 @@ private TsaParam parseRequestParam() throws Exception {
SignerInfo signerInfo = new SignerInfo(
new X500Name(issuerName),
signerEntry.cert.getSerialNumber(),
AlgorithmId.get(
AlgorithmId.getDigAlgFromSigAlg(sigAlgo)),
AlgorithmId.get(SignatureUtil.extractDigestAlgFromDwithE(sigAlgo)),

This comment has been minimized.

@valeriepeng

valeriepeng Oct 16, 2020

So, sigAlgo would never be RSASSA-PSS, EDDSA, ED25519, or ED448?

This comment has been minimized.

@wangweij

wangweij Oct 16, 2020
Author Contributor

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.

@openjdk openjdk bot removed the csr label Oct 21, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2020

@wangweij This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8242068: Signed JAR support for RSASSA-PSS and EdDSA

Reviewed-by: valeriep

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 291 new commits pushed to the master branch:

  • 6bd05b1: 8255074: sun.nio.fs.WindowsPath::getPathForWin32Calls synchronizes on String object
  • 9e9f5e6: 8017179: [macosx] list1 and list2 vistble item isn't desired
  • 2ee2b4a: 8231454: File lock in Windows on a loaded jar due to a leak in Introspector::getBeanInfo
  • 42a6ead: 8254884: Make sure jvm does not crash with Arm SVE and Vector API
  • e5870cf: 8252133: The java/awt/GraphicsDevice/DisplayModes/CycleDMImage.java fails if metal pipeline is active
  • afc967f: 8254783: jpackage fails on Windows when application name differs from installer name
  • 3ccf487: 8253019: Enhanced JPEG decoding
  • cfb02d4: 8250861: Crash in MinINode::Ideal(PhaseGVN*, bool)
  • 0d35235: 8249927: Specify limits of jdk.serialProxyInterfaceLimit
  • d6cef99: 8245417: Improve certificate chain handling
  • ... and 281 more: https://git.openjdk.java.net/jdk/compare/3c4e824aa5fd99337759fa5000f5673ab2430750...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 21, 2020
@wangweij
Copy link
Contributor Author

@wangweij wangweij commented Oct 21, 2020

/integrate

@openjdk openjdk bot closed this Oct 21, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 21, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2020

@wangweij Since your change was applied there have been 300 commits pushed to the master branch:

  • e559bd2: 8254889: name_and_sig_as_C_string usages in frame coding without ResourceMark
  • da97ab5: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer
  • 7e26404: 8255000: C2: Unify IGVN processing when loop opts are over
  • 27230fa: 8255026: C2: Miscellaneous cleanups in Compile and PhaseIdealLoop code
  • c107178: 8253964: [Graal] UnschedulableGraphTest#test01fails with expected:<4> but was:<3>
  • bd45191: 8255065: Zero: accessor_entry misses the IRIW case
  • 2a06335: 8254785: compiler/graalunit/HotspotTest.java failed with "missing Graal intrinsics for: java/lang/StringLatin1.indexOfChar([BIII)I"
  • 1b7ddeb: 8254976: Re-enable swing jtreg tests which were broken due to samevm mode
  • 2e510e0: 8255043: Incorrectly styled copyright text
  • 6bd05b1: 8255074: sun.nio.fs.WindowsPath::getPathForWin32Calls synchronizes on String object
  • ... and 290 more: https://git.openjdk.java.net/jdk/compare/3c4e824aa5fd99337759fa5000f5673ab2430750...master

Your commit was automatically rebased without conflicts.

Pushed as commit 839f01d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@wangweij wangweij deleted the wangweij:8242068 branch Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants