Address review comment.

project-chip · Apr 10, 2023 · 12b2df3 · 12b2df3
1 parent 1cd4c69
commit 12b2df3
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 27 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRCSRInfo.h b/src/darwin/Framework/CHIP/MTRCSRInfo.h
@@ -35,10 +35,7 @@ API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4))
 @property (nonatomic, copy, readonly) MTRCSRDERBytes csr;
 
 /**
- * The nonce associated with this CSR.  Depending on where the
- * MTROperationalCSRInfo comes from, this could be the nonce from the CSRRequest
- * command that led to this CSR being created, or the nonce from the
- * csrElementsTLV.
+ * The nonce associated with this CSR.
  */
 @property (nonatomic, copy, readonly) NSData * csrNonce;
 
@@ -58,9 +55,8 @@ API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4))
 @property (nonatomic, copy, readonly) NSData * attestationSignature;
 
 /**
- * Initialize an MTROperationalCSRInfo by providing all the fields.  It's the
- * caller's responsibility to ensure that the provided csr matches
- * csrElementsTLV.
+ * Initialize an MTROperationalCSRInfo by providing all the fields.  This will
+ * ensure that csr and csrNonce match the data in csrElementsTLV.
  */
 - (instancetype)initWithCSR:(MTRCSRDERBytes)csr
                    csrNonce:(NSData *)csrNonce
@@ -70,7 +66,9 @@ API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4))
 /**
  * Initialize an MTROperationalCSRInfo by providing the csrNonce (for example,
  * the nonce the client initially supplied), and the csrElementsTLV and
- * attestationSignature that the server returned.
+ * attestationSignature that the server returned.  This will ensure that
+ * csrNonce matches the data in csrElementsTLV, and extract the csr from
+ * csrElementsTLV.
  */
 - (instancetype)initWithCSRNonce:(NSData *)csrNonce
                   csrElementsTLV:(MTRTLVBytes)csrElementsTLV

diff --git a/src/darwin/Framework/CHIP/MTRCSRInfo.mm b/src/darwin/Framework/CHIP/MTRCSRInfo.mm
@@ -47,10 +47,10 @@ + (void)initialize
     MTRFrameworkInit();
 }
 
-- (instancetype)initWithCSR:(MTRCSRDERBytes)csr
-                   csrNonce:(NSData *)csrNonce
-             csrElementsTLV:(MTRTLVBytes)csrElementsTLV
-       attestationSignature:(NSData *)attestationSignature;
+- (instancetype)_initWithValidatedCSR:(MTRCSRDERBytes)csr
+                             csrNonce:(NSData *)csrNonce
+                       csrElementsTLV:(MTRTLVBytes)csrElementsTLV
+                 attestationSignature:(NSData *)attestationSignature;
 {
     if (self = [super init]) {
         _csr = csr;
@@ -61,29 +61,57 @@ - (instancetype)initWithCSR:(MTRCSRDERBytes)csr
     return self;
 }
 
+- (instancetype)initWithCSR:(MTRCSRDERBytes)csr
+                   csrNonce:(NSData *)csrNonce
+             csrElementsTLV:(MTRTLVBytes)csrElementsTLV
+       attestationSignature:(NSData *)attestationSignature;
+{
+    chip::ByteSpan extractedCSR, extractedNonce;
+    VerifyOrReturnValue(ExtractCSRAndNonce(csrElementsTLV, extractedCSR, extractedNonce) == CHIP_NO_ERROR, nil);
+
+    if (!extractedCSR.data_equal(AsByteSpan(csr))) {
+        MTR_LOG_ERROR("Provided CSR does not match provided csrElementsTLV");
+        return nil;
+    }
+
+    if (!extractedNonce.data_equal(AsByteSpan(csrNonce))) {
+        MTR_LOG_ERROR("Provided CSR nonce does not match provided csrElementsTLV");
+        return nil;
+    }
+
+    return [self _initWithValidatedCSR:csr
+                              csrNonce:csrNonce
+                        csrElementsTLV:csrElementsTLV
+                  attestationSignature:attestationSignature];
+}
+
 - (instancetype)initWithCSRNonce:(NSData *)csrNonce
                   csrElementsTLV:(MTRTLVBytes)csrElementsTLV
             attestationSignature:(NSData *)attestationSignature
 {
-    chip::ByteSpan csr;
-    {
-        // We don't care about the nonce.
-        chip::ByteSpan ignoredNonce;
-        VerifyOrReturnValue(ExtractCSRAndNonce(csrElementsTLV, csr, ignoredNonce) == CHIP_NO_ERROR, nil);
+    chip::ByteSpan csr, extractedNonce;
+    VerifyOrReturnValue(ExtractCSRAndNonce(csrElementsTLV, csr, extractedNonce) == CHIP_NO_ERROR, nil);
+
+    if (!extractedNonce.data_equal(AsByteSpan(csrNonce))) {
+        MTR_LOG_ERROR("Provided CSR nonce does not match provided csrElementsTLV");
+        return nil;
     }
 
-    return [self initWithCSR:AsData(csr) csrNonce:csrNonce csrElementsTLV:csrElementsTLV attestationSignature:attestationSignature];
+    return [self _initWithValidatedCSR:AsData(csr)
+                              csrNonce:csrNonce
+                        csrElementsTLV:csrElementsTLV
+                  attestationSignature:attestationSignature];
 }
 
 - (instancetype)initWithCSRElementsTLV:(MTRTLVBytes)csrElementsTLV attestationSignature:(NSData *)attestationSignature
 {
     chip::ByteSpan csr, csrNonce;
     VerifyOrReturnValue(ExtractCSRAndNonce(csrElementsTLV, csr, csrNonce) == CHIP_NO_ERROR, nil);
 
-    return [self initWithCSR:AsData(csr)
-                    csrNonce:AsData(csrNonce)
-              csrElementsTLV:csrElementsTLV
-        attestationSignature:attestationSignature];
+    return [self _initWithValidatedCSR:AsData(csr)
+                              csrNonce:AsData(csrNonce)
+                        csrElementsTLV:csrElementsTLV
+                  attestationSignature:attestationSignature];
 }
 
 - (instancetype)initWithCSRResponseParams:(MTROperationalCredentialsClusterCSRResponseParams *)responseParams

diff --git a/src/darwin/Framework/CHIP/MTROperationalCertificateIssuer.h b/src/darwin/Framework/CHIP/MTROperationalCertificateIssuer.h
@@ -75,11 +75,9 @@ API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4))
  * This will be called on the dispatch queue passed as
  * operationalCertificateIssuerQueue in the MTRDeviceControllerFactoryParams.
  *
- * The csrNonce in the provided MTROperationalCSRInfo will be the nonce _we_
- * provided when sending the CSRRequest commmand.  If device attestation
- * succeeded, this will match the nonce returned in the CSRResponse command.
- * The actual nonce returned in CSRResponse can be determined by initializing a
- * new MTROperationalCSRInfo with csrInfo.csrElementsTLV.
+ * The csrNonce in the provided MTROperationalCSRInfo will be the nonce that was
+ * sent in the CSRRequest command, which will be guaranteed, at this point, to
+ * match the nonce in the CSRResponse command.
  */
 - (void)issueOperationalCertificateForRequest:(MTROperationalCSRInfo *)csrInfo
                               attestationInfo:(MTRDeviceAttestationInfo *)attestationInfo