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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 8 commits into from
Closed

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

wants to merge 8 commits into from
+1,311 −737

Conversation

wangweij
Copy link
Contributor

@wangweij wangweij commented Sep 23, 2020 •
edited by openjdk bot
Loading

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 null initial cache in ClassValueMap #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

Sorry, something went wrong.

@bridgekeeper
Copy link

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 Pull request is ready for review label Sep 23, 2020
@openjdk
Copy link

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.

@openjdk openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Sep 23, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 23, 2020 •
edited
Loading

Webrevs

@openjdk
Copy link

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

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Sep 28, 2020
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Oct 4, 2020
@wangweij
Copy link
Contributor Author

wangweij commented Oct 4, 2020 •
edited
Loading

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

@wangweij
Copy link
Contributor Author

wangweij commented Oct 9, 2020

/csr

@wangweij
Copy link
Contributor Author

wangweij commented Oct 9, 2020

/label remove core-libs

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Oct 9, 2020
@openjdk
Copy link

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 core-libs-dev@openjdk.org label Oct 9, 2020
@openjdk
Copy link

openjdk bot commented Oct 9, 2020

@wangweij
The core-libs label was successfully removed.

@wangweij
Copy link
Contributor Author

wangweij commented Oct 13, 2020 •
edited
Loading

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
Copy link
Contributor Author

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

valeriepeng
valeriepeng reviewed Oct 14, 2020
View reviewed changes
src/java.base/share/classes/sun/security/util/SignatureUtil.java
* @param params (optional) parameters
* @return an AlgorithmParameterSpec object
* @throws ProviderException
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

valeriepeng
valeriepeng reviewed Oct 14, 2020
View reviewed changes
src/java.base/share/classes/sun/security/util/SignatureUtil.java Outdated
* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who with who?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/label remove compiler

@vicente-romero-oracle
Copy link
Contributor

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

@openjdk openjdk bot removed the compiler compiler-dev@openjdk.org label Oct 15, 2020
@openjdk
Copy link

openjdk bot commented Oct 15, 2020

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

@wangweij
Copy link
Contributor Author

wangweij commented Oct 15, 2020 •
edited
Loading

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.

valeriepeng
valeriepeng reviewed Oct 15, 2020
View reviewed changes
src/java.base/share/classes/sun/security/pkcs/PKCS7.java
contentInfo = new ContentInfo(derin, oldStyle);
contentType = contentInfo.contentType;
DerValue content = contentInfo.getContent();
ContentInfo block = new ContentInfo(derin, oldStyle);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/java.base/share/classes/sun/security/pkcs/PKCS7.java
digAlgID.derEncode(derAlgs);
DerOutputStream derSigAlg = new DerOutputStream();
sigAlgID.derEncode(derSigAlg);
derAlgs.writeImplicit((byte)0xA1, derSigAlg);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/java.base/share/classes/sun/security/pkcs/SignerInfo.java Outdated
}
return encAlg;
default:
String digAlg = digAlgId.getName().replace("-", "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. I'll also try some tests.

Copy link
Contributor Author

@wangweij wangweij Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

valeriepeng
valeriepeng reviewed Oct 16, 2020
View reviewed changes
test/jdk/jdk/security/jarsigner/Spec.java
@@ -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));
Copy link
Contributor

@valeriepeng valeriepeng Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

valeriepeng
valeriepeng reviewed Oct 16, 2020
View reviewed changes
test/lib/jdk/test/lib/security/timestamp/TsaSigner.java Outdated
@@ -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)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Pull request needs approved CSR before integration label Oct 21, 2020
@openjdk
Copy link

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 Pull request is ready to be integrated label Oct 21, 2020
@wangweij
Copy link
Contributor Author

/integrate

@openjdk openjdk bot closed this Oct 21, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 21, 2020
@openjdk
Copy link

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 8242068 branch October 21, 2020 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlanBateman AlanBateman

@valeriepeng valeriepeng

Assignees
No one assigned
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wangweij @vicente-romero-oracle @AlanBateman @valeriepeng