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

Upgrade CXF to latest version to get rid of CVE-2022-46364 and #197

Closed
wants to merge 2 commits into from
Closed

Upgrade CXF to latest version to get rid of CVE-2022-46364 and #197

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
824db31
Upgrade CXF to latest version to get rid of CVE-2022-46364 and
post-svejk Dec 20, 2022
aa25f5c
Also set custom asymmetric signature for the BASIC_128_GCM_SHA_256 suite
post-svejk Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@

<peppol.sbdh.version>1.0.0</peppol.sbdh.version>

<cxf.version>3.3.8</cxf.version>
<wss4j.version>2.2.7</wss4j.version>
<cxf.version>3.5.5</cxf.version>
<wss4j.version>2.4.1</wss4j.version>
<neethi.version>3.2.0</neethi.version>
<soap.version>1.4.0</soap.version>

Expand Down
7 changes: 4 additions & 3 deletions src/main/java/network/oxalis/as4/util/OxalisAlgorithmSuiteLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,14 @@ public static class OxalisAlgorithmSuite extends AlgorithmSuite {
128, 128, 128, 256, 1024, 4096
)
);
ALGORITHM_SUITE_TYPES.get(BASIC_128_GCM_SHA_256_MGF_SHA_256).setMGFAlgo(MGF_SHA256);
ALGORITHM_SUITE_TYPES.get(BASIC_128_GCM_SHA_256_MGF_SHA_256).setEncryptionDigest(SPConstants.SHA256);
AlgorithmSuiteType algorithmSuiteType = ALGORITHM_SUITE_TYPES.get(BASIC_128_GCM_SHA_256_MGF_SHA_256);
algorithmSuiteType.setMGFAlgo(MGF_SHA256);
algorithmSuiteType.setEncryptionDigest(SPConstants.SHA256);
algorithmSuiteType.setAsymmetricSignature("http://www.w3.org/2001/04/xmldsig-more#rsa-sha256");
}

OxalisAlgorithmSuite(final SPConstants.SPVersion version, final Policy nestedPolicy) {
super(version, nestedPolicy);
this.setAsymmetricSignature("http://www.w3.org/2001/04/xmldsig-more#rsa-sha256");
Copy link

Choose a reason for hiding this comment

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

I do not understand this change - does it relate to the topic of PR?

Copy link
Author

Choose a reason for hiding this comment

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

The API of WSS4J have changed. The method setAsymmetricSignature appears to have been moved from the top level class AlgorithmSuite to the nested class AlgorithmSuiteType.

Copy link

Choose a reason for hiding this comment

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

OK, good! But could it be that AsymmetricSignature is not set to "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256" for
BASIC_128_GCM_SHA_256
but only for
BASIC_128_GCM_SHA_256_MGF_SHA_256?

In previous version, it was defined on a OxalisAlgorithmSuite level, in a new version - on AlgorithmSuiteType, but in

Oxalis-AS4/src/main/java/network/oxalis/as4/util/OxalisAlgorithmSuiteLoader.java

Line 76 in 824db31

algorithmSuiteType.setAsymmetricSignature("http://www.w3.org/2001/04/xmldsig-more#rsa-sha256");
it is set only for BASIC_128_GCM_SHA_256_MGF_SHA_256, right?

May be we miss similar line for BASIC_128_GCM_SHA_256?

If we make mistake here, it will be quite difficult to find it out...

Copy link
Author

Choose a reason for hiding this comment

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

Yes. The asymmetric signature is now only set for the second custom AlgorithmSuiteType, BASIC_128_GCM_SHA_256_MGF_SHA_256. It might well be that it is needed for the first one, too.

I put it on the second suite type, since it already had a couple of customizations, guessing that the first one might not need customisation. But I don't know these WSS4J APIs, I just did the bare minimum to get it to compile against newer CXF and WSS4J. And it appears to be working in our test environment, but that might be a false positive, if some default asymmetric signature with weaker security is used instead...

Do you think its safer to assign the asymmetric signature on both?

Copy link
Author

Choose a reason for hiding this comment

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

From the WSS4J code, the algorithm suite gets a default asymmetric signature, when we don't specify it. So it's probably safest to set it on both suites. I have added that in the last commit.

Copy link

Choose a reason for hiding this comment

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

Thank you! Yes, this looks safer - better to keep the same logic as much as possible during upgrades.
I think it is ok now - although I would write it like before, without extracting to a variable algorithmSuiteType for only one of suites:

ALGORITHM_SUITE_TYPES.get(BASIC_128_GCM_SHA_256_MGF_SHA_256).setMGFAlgo(MGF_SHA256);
ALGORITHM_SUITE_TYPES.get(BASIC_128_GCM_SHA_256_MGF_SHA_256).setEncryptionDigest(SPConstants.SHA256);
ALGORITHM_SUITE_TYPES.get(BASIC_128_GCM_SHA_256_MGF_SHA_256)
	.setAsymmetricSignature("http://www.w3.org/2001/04/xmldsig-more#rsa-sha256");

ALGORITHM_SUITE_TYPES.get(BASIC_128_GCM_SHA_256)
	.setAsymmetricSignature("http://www.w3.org/2001/04/xmldsig-more#rsa-sha256");

In this case, the commit diff would clearly highlight, that you just added .setAsymmetricSignature("http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"); to both suite types because it is removed from Suite.
But it is just a question of styling :)

Anyway, let's see @aaron-kumar review too :)

Copy link
Author

Choose a reason for hiding this comment

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

Ok, seems like @aaron-kumar did a superset of these changes in the latest release, and also added code to adjust the signature algorithm.

}

@Override
Expand Down