Generic remote attestation implementation by Akretsch · Pull Request #148 · siemens/cmp-ra-component

Akretsch · 2025-10-30T09:37:45Z

Description

Implement remote attestation message flow over CMP as far as specified and stable in

sonarqubecloud · 2025-11-06T13:47:55Z

Quality Gate passed

Issues
19 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.4% Coverage on New Code
2.1% Duplication on New Code

See analysis details on SonarQube Cloud

ralienpp · 2026-01-22T15:03:33Z

+        EvidenceBundle evidenceBundle = EvidenceBundle.getInstance(evidenceItav.getParsedValue());
+        EvidenceStatement[] evidences = evidenceBundle.getEvidences();
+        AttestationResult[] attestationResults = new AttestationResult[evidences.length];


Would it be feasible for an attacker to craft a message that makes it look like evidences.length is a very large number?

Yes, but ASN.1 is unlimited too.

TODO consider writing some tests to check if BouncyCastle itself will take care of such malicious payloads.

I think the worst thing that could happen here is a OutOfMemoryError exception, right?
If so, at which level would it be caught and handled?

ralienpp · 2026-01-22T15:45:40Z

+            NonceRequest aktRequest = nonceRequests[i];
+            NonceResponseRet ret = verifyAdapter.generateNonce(
+                    msg.getHeader().getTransactionID().getOctets(),
+                    ifNotNull(aktRequest.getLen(), ASN1Integer::getValue),


Would it make sense to set an upper bound to the size of the nonce we generate?

ASN1 Integers are essentially unbounded, so a specially crafted request could potentially exhaust our memory.

If the nonce must be PQ-safe it could become bigger :-)

@ralienpp is right, an attacker could easily provoke an out-of-memory condition and thus do a DoS attack.
Clearly the size of the nonces should be limited to a sensible value.

ralienpp · 2026-01-27T09:30:53Z

Additional recommended reading material: RFC 9334, esp. section 5.2 background check model.

sonarqubecloud · 2026-01-28T07:47:18Z

Quality Gate passed

Issues
28 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.4% Coverage on New Code
2.1% Duplication on New Code

See analysis details on SonarQube Cloud

DDvO

Nice first draft.

At appropriate time, some updates and improvements will be due.

DDvO · 2026-03-23T18:25:44Z

+                NonceRequest nonceRequest = new NonceRequest(
+                        attestationContext.getNonceRequestLen(),
+                        attestationContext.getNonceRequestType(),
+                        attestationContext.getNonceRequestHint(),
+                        attestationContext.getNonceRequestVendorextension());
+                NonceRequestValue nonceRequestValue = new NonceRequestValue(new NonceRequest[] {nonceRequest});


So here you generate a request for a single nonce.
Please add at least a TODO that this should be generalized to request for multiple nonces.

Needs change of
ClientAttestationContext ClientContext:getAttestationContext()
replace with
Collection <ClientAttestationContext> ClientContext:getAttestationContext()

DDvO · 2026-03-23T18:28:32Z

+                final InfoTypeAndValue[] itav = content.toInfoTypeAndValueArray();
+                if (itav != null) {
+                    for (final InfoTypeAndValue aktitav : itav) {
+                        if (AttestationObjectIdentifiers.id_it_NonceResponse.equals(aktitav.getInfoType())) {


Otherwise should throw an error

Its about robustness -> Ignore, what you are unable to handle.

DDvO · 2026-03-23T18:29:47Z

+                final GenRepContent content = (GenRepContent) ratNonceResponseBody.getContent();
+                final InfoTypeAndValue[] itav = content.toInfoTypeAndValueArray();
+                if (itav != null) {
+                    for (final InfoTypeAndValue aktitav : itav) {


Should throw an error if not getting exactly one ITAV

DDvO · 2026-03-23T18:30:58Z

+                                return null;
+                            }
+                            final NonceResponseValue ratNonce = NonceResponseValue.getInstance(infoValue);
+                            NonceResponse[] nonceResponses = ratNonce.getNonceResponse();


Should check that the number of nonces obtains equals the number requested, which so far is always 1.

Robustness.

DDvO · 2026-03-23T18:38:27Z

+        EvidenceBundle evidenceBundle = EvidenceBundle.getInstance(evidenceItav.getParsedValue());
+        EvidenceStatement[] evidences = evidenceBundle.getEvidences();
+        AttestationResult[] attestationResults = new AttestationResult[evidences.length];


I think the worst thing that could happen here is a OutOfMemoryError exception, right?
If so, at which level would it be caught and handled?

DDvO · 2026-03-23T18:56:12Z

+     * @param evidence evidence provided by EE as DER encoded {@link EvidenceStatement}
+     * @return verification result as DER  encoded {@link AttestationResult}
+     */
+    byte[] processRatVerification(byte[] transactionId, byte[] evidence);


I think the verifier should also get the certs from the evicence/attestation bundle.

DDvO · 2026-03-23T19:00:26Z

+ * AttestationResultBundle ::= SEQUENCE {
+ * results SEQUENCE SIZE (1..MAX) OF AttestationResult,
+ * certs SEQUENCE SIZE (1..MAX) OF CertificateChoices OPTIONAL,
+ * -- CertificateChoices MUST only contain certificate or other,
+ * -- see Section 10.2.2 of [RFC5652]


For some reason, this structure has disappeared in https://datatracker.ietf.org/doc/html/draft-ietf-lamps-csr-attestation-22

DDvO · 2026-03-23T19:04:10Z

+                            }
+                            final NonceResponseValue ratNonce = NonceResponseValue.getInstance(infoValue);
+                            NonceResponse[] nonceResponses = ratNonce.getNonceResponse();
+                            EvidenceStatement[] evidenceStatements = new EvidenceStatement[nonceResponses.length];


Does the number of evidence statements always follow the number of nonce responses?
Please add at least a respective TODO to generalize this.

DDvO · 2026-03-23T19:06:42Z

+            return null;
+        }
+        EvidenceBundle evidenceBundle = EvidenceBundle.getInstance(evidenceItav.getParsedValue());
+        EvidenceStatement[] evidences = evidenceBundle.getEvidences();


Should the number of evidence/attestation statements follow the number of nonce requests/responses?
Please add at least a respective TODO to possibly check for this.

DDvO · 2026-03-23T19:09:12Z

+            NonceRequest aktRequest = nonceRequests[i];
+            NonceResponseRet ret = verifyAdapter.generateNonce(
+                    msg.getHeader().getTransactionID().getOctets(),
+                    ifNotNull(aktRequest.getLen(), ASN1Integer::getValue),


@ralienpp is right, an attacker could easily provoke an out-of-memory condition and thus do a DoS attack.
Clearly the size of the nonces should be limited to a sensible value.

DDvO · 2026-03-24T06:55:51Z

+    PKIMessage buildFurtherRequest(
+            final PKIMessage formerResponse,
+            final PKIBody requestBody,
+            final boolean withImplicitConfirm,
+            final int pvno)


Maybe it is simpler to keep at least part of these argument in some state fields and take them from there?
I suppose one could even avoid distinguishing between methods building initial vs. further requests.

Holding (transaction) states global and non final makes it hard to implement concurrency and slows down the VM. So I would keep everything local and final as much as possible.

ZMaradics · 2026-04-13T13:23:04Z

+     * Siemens proprietary extension to carry additional data
+     * @return additional data or <code>null</code>
+     */
+    default byte[] getNonceRequestVendorextension() {


Consider getNonceRequestVendorExtension (camelCase) instead of getNonceRequestVendorextension.

ZMaradics · 2026-04-13T14:37:03Z

@@ -409,6 +418,51 @@ private boolean grantsImplicitConfirm(final PKIMessage msg) {
    public EnrollmentResult invokeEnrollment() {


This method is around 200 lines of code with a high complexity.
I recommend refactoring it into smaller methods to improve readability and simplify unit testing. Helps future maintenance.

ZMaradics · 2026-04-13T14:39:58Z

+         * Siemens proprietary extension to carry additional data
+         * @return additional data or <code>null</code>
+         */
+        default byte[] getVendorextension() {


Recommended to rename getVendorextension to getVendorExtension

ZMaradics · 2026-04-13T14:40:45Z

+            BigInteger len,
+            String type,
+            String hint,
+            byte[] vendorextension,


Rename to vendorExtension

Akretsch force-pushed the feat_generic_RAT_impl branch 2 times, most recently from da71f21 to 86b8402 Compare October 30, 2025 10:22

Akretsch changed the title ~~Generic remote attestation imlementation~~ Generic remote attestation implementation Oct 30, 2025

Akretsch force-pushed the feat_generic_RAT_impl branch 3 times, most recently from 0814d99 to 0e2e343 Compare October 30, 2025 16:12

Akretsch requested review from DDvO, ZMaradics and ralienpp October 30, 2025 16:32

Akretsch force-pushed the feat_generic_RAT_impl branch 2 times, most recently from bbab075 to 812c16d Compare November 4, 2025 11:27

ralienpp reviewed Jan 22, 2026

View reviewed changes

Akretsch added 3 commits January 28, 2026 08:21

implement generic RAT based on OCTET String

c6c8cc2

improve test coverage

f48d400

add vendor extension to nonce request and response

c0f7848

Akretsch force-pushed the feat_generic_RAT_impl branch 2 times, most recently from ce8b445 to 2cef504 Compare January 28, 2026 07:27

fix typos

2cef504

DDvO requested changes Mar 23, 2026

View reviewed changes

DDvO reviewed Mar 24, 2026

View reviewed changes

ZMaradics reviewed Apr 13, 2026

View reviewed changes

		@@ -409,6 +418,51 @@ private boolean grantsImplicitConfirm(final PKIMessage msg) {
		public EnrollmentResult invokeEnrollment() {

Uh oh!

Conversation

Akretsch commented Oct 30, 2025

Description

Uh oh!

sonarqubecloud Bot commented Nov 6, 2025

Quality Gate passed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ralienpp commented Jan 27, 2026

Uh oh!

sonarqubecloud Bot commented Jan 28, 2026

Quality Gate passed

Uh oh!

DDvO left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants