[IOT-2293] [IOT-2192] reject POST to /acl2 Resource with ACL1 payload

/acl2 POST handler now calls a version-check-only optional CBORPayloadToAcl()
function and, if an acl1 payload is found, denies the POST request.

This update caught some unit test issues that were also corrected with
this patch.

Change-Id: I20d148ef037c82f5862fd9fec156bbb399ab7417
Signed-off-by: Nathan Heldt-Sheller <nathan.heldt-sheller@intel.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/20189
Tested-by: jenkins-iotivity <jenkins@iotivity.org>
Reviewed-by: Randeep Singh <randeep.s@samsung.com>
Reviewed-by: Dmitriy Zhuravlev <d.zhuravlev@samsung.com>

2 files changed, 48 insertions, 7 deletions

diff --git a/resource/csdk/security/src/aclresource.c b/resource/csdk/security/src/aclresource.c
index e5783b5e8..e8f5379bc 100644
--- a/resource/csdk/security/src/aclresource.c
+++ b/resource/csdk/security/src/aclresource.c
@@ -1335,11 +1335,19 @@ exit:
 }
 
 // This function converts CBOR format to ACL data.
+// Callers should normally invoke "CBORPayloadToAcl()" unless wishing to check
+// version of payload only.
 // Caller needs to invoke 'OICFree' on returned value when done using
 // TODO IOT-2220 this function is a prime example of why the SVR CBOR functions need
 // to be re-factored throughout.  It's even worse with the addition of /acl2.
-// note: This function is used in unit test hence not declared static,
-OicSecAcl_t* CBORPayloadToAcl(const uint8_t *cborPayload, const size_t size)
+// @param[in]  cborPayload The CBOR data to be decoded into OicSecAcl_t
+// @param[in]  size The size of the CBOR data
+// @param[out] versionCheck If included, this function will determine the version of
+//              ACL in the payload, assign to 'versionCheck', and return NULL
+//              without decoding the rest of the payload.  If NULL, this function will complete
+//              decoding as normal, and will not assign a value to 'versionCheck'.
+static OicSecAcl_t* CBORPayloadToAclVersionOpt(const uint8_t *cborPayload, const size_t size,
+    OicSecAclVersion_t *versionCheck)
 {
     if (NULL == cborPayload || 0 == size)
     {
@@ -1383,12 +1391,27 @@ OicSecAcl_t* CBORPayloadToAcl(const uint8_t *cborPayload, const size_t size)
             OIC_LOG_V(DEBUG, TAG, "%s found %s tag.", __func__, tagName);
             if (0 == strcmp(tagName, OIC_JSON_ACLIST_NAME))
             {
+                if (NULL != versionCheck)
+                {
+                    OIC_LOG_V(DEBUG, TAG, "%s Found v1 ACL; assigning 'versionCheck' and returning NULL.", __func__);
+                    *versionCheck = OIC_SEC_ACL_V1;
+                    OICFree(acl);
+                    return NULL;
+                }
                 OIC_LOG_V(DEBUG, TAG, "%s decoding v1 ACL.", __func__);
                 aclistVersion = OIC_SEC_ACL_V1;
                 aclistTagJustFound = true;
+
             }
             else if (0 == strcmp(tagName, OIC_JSON_ACLIST2_NAME))
             {
+                if (NULL != versionCheck)
+                {
+                    OIC_LOG_V(DEBUG, TAG, "%s Found v2 ACL; assigning 'versionCheck' and returning NULL.", __func__);
+                    *versionCheck = OIC_SEC_ACL_V2;
+                    OICFree(acl);
+                    return NULL;
+                }
                 OIC_LOG_V(DEBUG, TAG, "%s decoding v2 ACL.", __func__);
                 aclistVersion = OIC_SEC_ACL_V2;
                 aclistTagJustFound = true;
@@ -1992,6 +2015,14 @@ exit:
     return acl;
 }
 
+// This function converts CBOR format to ACL data.
+// Caller needs to invoke 'OICFree' on returned value when done using
+// note: This function is used in unit test hence not declared static.
+OicSecAcl_t* CBORPayloadToAcl(const uint8_t *cborPayload, const size_t size)
+{
+    return CBORPayloadToAclVersionOpt(cborPayload, size, NULL);
+}
+
 #ifdef MULTIPLE_OWNER
 bool IsValidAclAccessForSubOwner(const OicUuid_t* uuid, const uint8_t *cborPayload, const size_t size)
 {
@@ -2650,6 +2681,15 @@ static OCEntityHandlerResult HandleACLPostRequest(const OCEntityHandlerRequest *
 
     if (payload)
     {
+        // Clients should not POST v1 ACL to OCF 1.0 Server
+        OicSecAclVersion_t payloadVersionReceived = OIC_SEC_ACL_V1;
+        CBORPayloadToAclVersionOpt(payload, size, &payloadVersionReceived);
+        if (OIC_SEC_ACL_V2 != payloadVersionReceived)
+        {
+            OIC_LOG_V(WARNING, TAG, "%s /acl Resource is v2; POST of v1 ACL not acceptable.", __func__);
+            ehRet = OC_EH_NOT_ACCEPTABLE;
+            goto exit;
+        }
         OicSecAcl_t *newAcl = NULL;
         OIC_LOG(DEBUG, TAG, "ACL payload from POST request << ");
         OIC_LOG_BUFFER(DEBUG, TAG, payload, size);
diff --git a/resource/csdk/security/unittest/aclresourcetest.cpp b/resource/csdk/security/unittest/aclresourcetest.cpp
index fa946595e..b37a64202 100644
--- a/resource/csdk/security/unittest/aclresourcetest.cpp
+++ b/resource/csdk/security/unittest/aclresourcetest.cpp
@@ -111,7 +111,8 @@ static int GetNumberOfResource(const OicSecAce_t* ace)
 
 TEST(ACLResourceTest, CBORDefaultACLConversion)
 {
-    uint8_t defaultAclSub[] = { 0x2a };
+    uint8_t defaultAclSub[] = {0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31,
+        0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31};
     uint8_t defaultAclOwnrs[] = {0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32,
         0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32};
 
@@ -136,7 +137,7 @@ TEST(ACLResourceTest, CBORDefaultACLConversion)
 
     size_t defaultAclSize = 0;
     uint8_t *defaultPsStorage = NULL;
-    OCStackResult convRet = AclToCBORPayload(defaultAcl, OIC_SEC_ACL_V1, &defaultPsStorage, &defaultAclSize);
+    OCStackResult convRet = AclToCBORPayload(defaultAcl, OIC_SEC_ACL_V2, &defaultPsStorage, &defaultAclSize);
     EXPECT_EQ(OC_STACK_OK, convRet);
     ASSERT_TRUE(NULL != defaultPsStorage);
     EXPECT_NE(static_cast<size_t>(0), defaultAclSize);
@@ -451,7 +452,7 @@ TEST(ACLResourceTest, ACLDeleteWithSingleResourceTest)
     //GET CBOR POST payload
     size_t size = 0;
     uint8_t  *payload = NULL;
-    EXPECT_EQ(OC_STACK_OK, AclToCBORPayload(&acl, OIC_SEC_ACL_V1, &payload, &size));
+    EXPECT_EQ(OC_STACK_OK, AclToCBORPayload(&acl, OIC_SEC_ACL_V2, &payload, &size));
     ASSERT_TRUE(NULL != payload);
 
     // Security Payload
@@ -511,7 +512,7 @@ TEST(ACLResourceTest, ACLDeleteWithMultiResourceTest)
     //GET CBOR POST payload
     size_t size = 0;
     uint8_t *payload = NULL;
-    EXPECT_EQ(OC_STACK_OK, AclToCBORPayload(&acl, OIC_SEC_ACL_V1, &payload, &size));
+    EXPECT_EQ(OC_STACK_OK, AclToCBORPayload(&acl, OIC_SEC_ACL_V2, &payload, &size));
     ASSERT_TRUE(NULL != payload);
 
     // Security Payload
@@ -578,7 +579,7 @@ TEST(ACLResourceTest, ACLGetWithQueryTest)
     //GET CBOR POST payload
     size_t size = 0;
     uint8_t *payload = NULL;
-    EXPECT_EQ(OC_STACK_OK, AclToCBORPayload(&acl, OIC_SEC_ACL_V1, &payload, &size));
+    EXPECT_EQ(OC_STACK_OK, AclToCBORPayload(&acl, OIC_SEC_ACL_V2, &payload, &size));
     ASSERT_TRUE(NULL != payload);
 
     // Security Payload