Resolved security issue-IOT1003 & IOT1013

https://jira.iotivity.org/browse/IOT-1013
[PM C]OCProvisionPairwiseDevices returns OC_STACK_OK instead of
OC_STACK_INVALID_PARAM for provisioning same device

https://jira.iotivity.org/browse/IOT-1003
[PM C] APIs returns OC_STACK_INVALID_PARAM instead of
 OC_STACK_INVALID_CALLBACK while CB = NULL

-Patch 1: Resolved security Issue on 1.1-RC1
-Patch 2: Apply MR.Cho's review comment

Change-Id: I1a2d0265e756da41cffbcc60042e4f35de2dbcf5
Signed-off-by: js126.lee <js126.lee@samsung.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/6219
Reviewed-by: Kyungsun Cho <goodsun.cho@samsung.com>
Reviewed-by: Yonggoo Kang <ygace.kang@samsung.com>
Reviewed-by: Chul Lee <chuls.lee@samsung.com>
Tested-by: jenkins-iotivity <jenkins-iotivity@opendaylight.org>
Reviewed-by: Randeep Singh <randeep.s@samsung.com>

diff --git a/resource/csdk/security/provisioning/src/ocprovisioningmanager.c b/resource/csdk/security/provisioning/src/ocprovisioningmanager.c
index 7ac40d9..d50fc36 100644 (file)
--- a/resource/csdk/security/provisioning/src/ocprovisioningmanager.c
+++ b/resource/csdk/security/provisioning/src/ocprovisioningmanager.c
@@ -114,7 +114,7 @@ OCStackResult OCSetOwnerTransferCallbackData(OicSecOxm_t oxm, OTMCallbackData_t*
 {
     if(NULL == callbackData)
     {
-        return OC_STACK_INVALID_PARAM;
+        return OC_STACK_INVALID_CALLBACK ;
     }
 
     return OTMSetOwnershipTransferCallbackData(oxm, callbackData);
@@ -128,7 +128,11 @@ OCStackResult OCDoOwnershipTransfer(void* ctx,
     {
         return OC_STACK_INVALID_PARAM;
     }
-
+    if (!resultCallback)
+    {
+        OIC_LOG(INFO, TAG, "OCDoOwnershipTransfer : NULL Callback");
+        return OC_STACK_INVALID_CALLBACK;
+    }
     return OTMDoOwnershipTransfer(ctx, targetDevices, resultCallback);
 }
 
@@ -215,11 +219,21 @@ OCStackResult OCUnlinkDevices(void* ctx,
     OCUuidList_t* idList = NULL;
     size_t numOfDev = 0;
 
-    if (!pTargetDev1 || !pTargetDev2 || !resultCallback)
+    if (!pTargetDev1 || !pTargetDev2 || !pTargetDev1->doxm || !pTargetDev2->doxm)
     {
         OIC_LOG(ERROR, TAG, "OCUnlinkDevices : NULL parameters");
         return OC_STACK_INVALID_PARAM;
     }
+    if (!resultCallback)
+    {
+        OIC_LOG(INFO, TAG, "OCUnlinkDevices : NULL Callback");
+        return OC_STACK_INVALID_CALLBACK;
+    }
+    if (0 == memcmp(&pTargetDev1->doxm->deviceID, &pTargetDev2->doxm->deviceID, sizeof(OicUuid_t)))
+    {
+        OIC_LOG(INFO, TAG, "OCUnlinkDevices : Same device ID");
+        return OC_STACK_INVALID_PARAM;
+    }
 
     // Get linked devices with the first device.
     OCStackResult res = PDMGetLinkedDevices(&(pTargetDev1->doxm->deviceID), &idList, &numOfDev);
@@ -277,11 +291,16 @@ OCStackResult OCRemoveDevice(void* ctx, unsigned short waitTimeForOwnedDeviceDis
 {
     OIC_LOG(INFO, TAG, "IN OCRemoveDevice");
     OCStackResult res = OC_STACK_ERROR;
-    if (!pTargetDev || !resultCallback || 0 == waitTimeForOwnedDeviceDiscovery)
+    if (!pTargetDev || 0 == waitTimeForOwnedDeviceDiscovery)
     {
         OIC_LOG(INFO, TAG, "OCRemoveDevice : Invalied parameters");
         return OC_STACK_INVALID_PARAM;
     }
+    if (!resultCallback)
+    {
+        OIC_LOG(INFO, TAG, "OCRemoveDevice : NULL Callback");
+        return OC_STACK_INVALID_CALLBACK;
+    }
 
     // Send DELETE requests to linked devices
     OCStackResult resReq = OC_STACK_ERROR; // Check that we have to wait callback or not.
@@ -551,16 +570,26 @@ OCStackResult OCProvisionPairwiseDevices(void* ctx, OicSecCredType_t type, size_
                                          OCProvisionResultCB resultCallback)
 {
 
-    if (!pDev1 || !pDev2 || !resultCallback)
+    if (!pDev1 || !pDev2 || !pDev1->doxm || !pDev2->doxm)
     {
         OIC_LOG(ERROR, TAG, "OCProvisionPairwiseDevices : Invalid parameters");
         return OC_STACK_INVALID_PARAM;
     }
+    if (!resultCallback)
+    {
+        OIC_LOG(INFO, TAG, "OCProvisionPairwiseDevices : NULL Callback");
+        return OC_STACK_INVALID_CALLBACK;
+    }
     if (!(keySize == OWNER_PSK_LENGTH_128 || keySize == OWNER_PSK_LENGTH_256))
     {
         OIC_LOG(INFO, TAG, "OCProvisionPairwiseDevices : Invalid key size");
         return OC_STACK_INVALID_PARAM;
     }
+    if (0 == memcmp(&pDev1->doxm->deviceID, &pDev2->doxm->deviceID, sizeof(OicUuid_t)))
+    {
+        OIC_LOG(INFO, TAG, "OCProvisionPairwiseDevices : Same device ID");
+        return OC_STACK_INVALID_PARAM;
+    }
 
     OIC_LOG(DEBUG, TAG, "Checking link in DB");
     bool linkExists = true;

diff --git a/resource/csdk/security/provisioning/src/secureresourceprovider.c b/resource/csdk/security/provisioning/src/secureresourceprovider.c
index ede10b2..cea7b43 100644 (file)
--- a/resource/csdk/security/provisioning/src/secureresourceprovider.c
+++ b/resource/csdk/security/provisioning/src/secureresourceprovider.c
@@ -629,12 +629,21 @@ OCStackResult SRPProvisionCredentials(void *ctx, OicSecCredType_t type, size_t k
                                       const OCProvisionDev_t *pDev2,
                                       OCProvisionResultCB resultCallback)
 {
-    VERIFY_NON_NULL(TAG, pDev1, ERROR,  OC_STACK_INVALID_PARAM);
-    if (SYMMETRIC_PAIR_WISE_KEY == type)
+    if (!pDev1 || !pDev2 || !pDev1->doxm || !pDev2->doxm)
     {
-        VERIFY_NON_NULL(TAG, pDev2, ERROR,  OC_STACK_INVALID_PARAM);
+        OIC_LOG(INFO, TAG, "SRPUnlinkDevices : NULL parameters");
+        return OC_STACK_INVALID_PARAM;
+    }
+    if (!resultCallback)
+    {
+        OIC_LOG(INFO, TAG, "SRPUnlinkDevices : NULL Callback");
+        return OC_STACK_INVALID_CALLBACK;
+    }
+    if (0 == memcmp(&pDev1->doxm->deviceID, &pDev2->doxm->deviceID, sizeof(OicUuid_t)))
+    {
+        OIC_LOG(INFO, TAG, "SRPUnlinkDevices : Same device ID");
+        return OC_STACK_INVALID_PARAM;
     }
-    VERIFY_NON_NULL(TAG, resultCallback, ERROR,  OC_STACK_INVALID_CALLBACK);
 
     if (SYMMETRIC_PAIR_WISE_KEY == type &&
        !(OWNER_PSK_LENGTH_128 == keySize || OWNER_PSK_LENGTH_256 == keySize))
@@ -1298,11 +1307,22 @@ OCStackResult SRPUnlinkDevices(void* ctx,
 {
     OIC_LOG(INFO, TAG, "IN SRPUnlinkDevices");
 
-    if (!pTargetDev1 || !pTargetDev2 || !resultCallback)
+    if (!pTargetDev1 || !pTargetDev2 || !pTargetDev1->doxm || !pTargetDev2->doxm)
     {
         OIC_LOG(INFO, TAG, "SRPUnlinkDevices : NULL parameters");
         return OC_STACK_INVALID_PARAM;
     }
+    if (!resultCallback)
+    {
+        OIC_LOG(INFO, TAG, "SRPUnlinkDevices : NULL Callback");
+        return OC_STACK_INVALID_CALLBACK;
+    }
+    if (0 == memcmp(&pTargetDev1->doxm->deviceID, &pTargetDev2->doxm->deviceID, sizeof(OicUuid_t)))
+    {
+        OIC_LOG(INFO, TAG, "SRPUnlinkDevices : Same device ID");
+        return OC_STACK_INVALID_PARAM;
+    }
+
     OIC_LOG(INFO, TAG, "Unlinking following devices: ");
     PMPrintOCProvisionDev(pTargetDev1);
     PMPrintOCProvisionDev(pTargetDev2);
@@ -1560,11 +1580,16 @@ OCStackResult SRPRemoveDevice(void* ctx, unsigned short waitTimeForOwnedDeviceDi
 {
     OIC_LOG(INFO, TAG, "IN SRPRemoveDevice");
 
-    if (!pTargetDev || !resultCallback || 0 == waitTimeForOwnedDeviceDiscovery)
+    if (!pTargetDev  || 0 == waitTimeForOwnedDeviceDiscovery)
     {
         OIC_LOG(INFO, TAG, "SRPRemoveDevice : NULL parameters");
         return OC_STACK_INVALID_PARAM;
     }
+    if (!resultCallback)
+    {
+        OIC_LOG(INFO, TAG, "SRPRemoveDevice : NULL Callback");
+        return OC_STACK_INVALID_CALLBACK;
+    }
 
     // Declare variables in here to handle error cases with goto statement.
     OCProvisionDev_t* pOwnedDevList = NULL;

diff --git a/resource/csdk/security/provisioning/unittest/ocprovisioningmanager.cpp b/resource/csdk/security/provisioning/unittest/ocprovisioningmanager.cpp
index b421a64..b10129a 100644 (file)
--- a/resource/csdk/security/provisioning/unittest/ocprovisioningmanager.cpp
+++ b/resource/csdk/security/provisioning/unittest/ocprovisioningmanager.cpp
@@ -20,6 +20,41 @@
 #include "gtest/gtest.h"
 #include "ocprovisioningmanager.h"
 
+static OicSecAcl_t acl1;
+static OicSecAcl_t acl2;
+static OCProvisionDev_t pDev1;
+static OCProvisionDev_t pDev2;
+static OicSecCredType_t credType = SYMMETRIC_PAIR_WISE_KEY;
+static OicSecOxm_t oicSecDoxmJustWorks = OIC_JUST_WORKS;
+static OicSecOxm_t oicSecDoxmRandomPin = OIC_RANDOM_DEVICE_PIN;
+static OicSecDoxm_t defaultDoxm1 =
+{
+    NULL,                   /* OicUrn_t *oxmType */
+    0,                      /* size_t oxmTypeLen */
+    &oicSecDoxmJustWorks,  /* uint16_t *oxm */
+    1,                      /* size_t oxmLen */
+    OIC_JUST_WORKS,         /* uint16_t oxmSel */
+    SYMMETRIC_PAIR_WISE_KEY,/* OicSecCredType_t sct */
+    false,                  /* bool owned */
+    {{0}},            /* OicUuid_t deviceID */
+    false,                  /* bool dpc */
+    {{0}},            /* OicUuid_t owner */
+};
+
+static OicSecDoxm_t defaultDoxm2 =
+{
+    NULL,                   /* OicUrn_t *oxmType */
+    0,                      /* size_t oxmTypeLen */
+    &oicSecDoxmRandomPin,  /* uint16_t *oxm */
+    1,                      /* size_t oxmLen */
+    OIC_RANDOM_DEVICE_PIN,         /* uint16_t oxmSel */
+    SYMMETRIC_PAIR_WISE_KEY,/* OicSecCredType_t sct */
+    false,                  /* bool owned */
+    {{0}},            /* OicUuid_t deviceID */
+    false,                  /* bool dpc */
+    {{0}},            /* OicUuid_t owner */
+};
+
 static void provisioningCB (void* UNUSED1, int UNUSED2, OCProvisionResult_t *UNUSED3, bool UNUSED4)
 {
     //dummy callback
@@ -29,25 +64,70 @@ static void provisioningCB (void* UNUSED1, int UNUSED2, OCProvisionResult_t *UNU
     (void) UNUSED4;
 }
 
+TEST(OCProvisionPairwiseDevicesTest, NullDevice1)
+{
+    pDev1.doxm = &defaultDoxm1;
+    uint8_t deviceId1[] = {0x64, 0x65, 0x76, 0x69, 0x63, 0x65, 0x49, 0x64};
+    memcpy(pDev1.doxm->deviceID.id, deviceId1, sizeof(deviceId1));
+
+    pDev2.doxm = &defaultDoxm2;
+    uint8_t deviceId2[] = {0x64, 0x65, 0x76, 0x69, 0x63, 0x65, 0x49, 0x63};
+    memcpy(pDev2.doxm->deviceID.id, deviceId2, sizeof(deviceId2));
+
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCProvisionPairwiseDevices(NULL, credType,
+                                                              OWNER_PSK_LENGTH_128, NULL, &acl1,
+                                                              &pDev2, &acl2, &provisioningCB));
+}
+
+TEST(OCProvisionPairwiseDevicesTest, NullDevice2)
+{
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCProvisionPairwiseDevices(NULL, credType,
+                                                              OWNER_PSK_LENGTH_128, &pDev1, &acl1,
+                                                              NULL, &acl2, &provisioningCB));
+}
+
+TEST(OCProvisionPairwiseDevicesTest, SamelDeviceId)
+{
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCProvisionPairwiseDevices(NULL, credType,
+                                                              OWNER_PSK_LENGTH_128, &pDev1, &acl1,
+                                                              &pDev1, &acl2, &provisioningCB));
+}
+
+TEST(OCProvisionPairwiseDevicesTest, NullCallback)
+{
+    EXPECT_EQ(OC_STACK_INVALID_CALLBACK, OCProvisionPairwiseDevices(NULL, credType,
+                                                              OWNER_PSK_LENGTH_128, &pDev1, &acl1,
+                                                              &pDev2, &acl2, NULL));
+}
+
+TEST(OCProvisionPairwiseDevicesTest, InvalidKeySize)
+{
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCProvisionPairwiseDevices(NULL, credType,
+                                                              0, &pDev1, &acl1,
+                                                              &pDev2, &acl2 ,&provisioningCB));
+}
+
 TEST(OCUnlinkDevicesTest, NullDevice1)
 {
-    OCProvisionDev_t dev2;
-    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCUnlinkDevices(NULL, NULL, &dev2, provisioningCB));
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCUnlinkDevices(NULL, NULL, &pDev2, provisioningCB));
 }
 
 TEST(OCUnlinkDevicesTest, NullDevice2)
 {
-    OCProvisionDev_t dev1;
-    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCUnlinkDevices(NULL, &dev1, NULL, provisioningCB));
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCUnlinkDevices(NULL, &pDev1, NULL, provisioningCB));
 }
 
 TEST(OCUnlinkDevicesTest, NullCallback)
 {
-    OCProvisionDev_t dev1;
-    OCProvisionDev_t dev2;
-    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCUnlinkDevices(NULL, &dev1, &dev2, NULL));
+    EXPECT_EQ(OC_STACK_INVALID_CALLBACK, OCUnlinkDevices(NULL, &pDev1, &pDev2, NULL));
+}
+
+TEST(OCUnlinkDevicesTest, SamelDeviceId)
+{
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCUnlinkDevices(NULL,&pDev1, &pDev1, &provisioningCB));
 }
 
+
 TEST(OCRemoveDeviceTest, NullTargetDevice)
 {
     unsigned short waitTime = 10 ;
@@ -57,15 +137,13 @@ TEST(OCRemoveDeviceTest, NullTargetDevice)
 TEST(OCRemoveDeviceTest, NullResultCallback)
 {
     unsigned short waitTime = 10;
-    OCProvisionDev_t dev1;
-    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCRemoveDevice(NULL, waitTime, &dev1, NULL));
+    EXPECT_EQ(OC_STACK_INVALID_CALLBACK, OCRemoveDevice(NULL, waitTime, &pDev1, NULL));
 }
 
 TEST(OCRemoveDeviceTest, ZeroWaitTime)
 {
     unsigned short waitTime = 0;
-    OCProvisionDev_t dev1;
-    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCRemoveDevice(NULL, waitTime, &dev1, provisioningCB));
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, OCRemoveDevice(NULL, waitTime, &pDev1, provisioningCB));
 }
 
 TEST(OCGetDevInfoFromNetworkTest, NullUnOwnedDeviceInfo)

diff --git a/resource/csdk/security/provisioning/unittest/secureresourceprovider.cpp b/resource/csdk/security/provisioning/unittest/secureresourceprovider.cpp
index 25c0acc..b8e3875 100644 (file)
--- a/resource/csdk/security/provisioning/unittest/secureresourceprovider.cpp
+++ b/resource/csdk/security/provisioning/unittest/secureresourceprovider.cpp
@@ -26,6 +26,35 @@ static OCProvisionDev_t pDev2;
 static OicSecCredType_t credType = SYMMETRIC_PAIR_WISE_KEY;
 static OCProvisionDev_t selectedDeviceInfo;
 static OicSecPconf_t pconf;
+static OicSecOxm_t oicSecDoxmJustWorks = OIC_JUST_WORKS;
+static OicSecOxm_t oicSecDoxmRandomPin = OIC_RANDOM_DEVICE_PIN;
+static OicSecDoxm_t defaultDoxm1 =
+{
+    NULL,                   /* OicUrn_t *oxmType */
+    0,                      /* size_t oxmTypeLen */
+    &oicSecDoxmJustWorks,  /* uint16_t *oxm */
+    1,                      /* size_t oxmLen */
+    OIC_JUST_WORKS,         /* uint16_t oxmSel */
+    SYMMETRIC_PAIR_WISE_KEY,/* OicSecCredType_t sct */
+    false,                  /* bool owned */
+    {{0}},            /* OicUuid_t deviceID */
+    false,                  /* bool dpc */
+    {{0}},            /* OicUuid_t owner */
+};
+
+static OicSecDoxm_t defaultDoxm2 =
+{
+    NULL,                   /* OicUrn_t *oxmType */
+    0,                      /* size_t oxmTypeLen */
+    &oicSecDoxmRandomPin,  /* uint16_t *oxm */
+    1,                      /* size_t oxmLen */
+    OIC_RANDOM_DEVICE_PIN,         /* uint16_t oxmSel */
+    SYMMETRIC_PAIR_WISE_KEY,/* OicSecCredType_t sct */
+    false,                  /* bool owned */
+    {{0}},            /* OicUuid_t deviceID */
+    false,                  /* bool dpc */
+    {{0}},            /* OicUuid_t owner */
+};
 
 static void provisioningCB (void* UNUSED1, int UNUSED2, OCProvisionResult_t *UNUSED3, bool UNUSED4)
 {
@@ -38,6 +67,14 @@ static void provisioningCB (void* UNUSED1, int UNUSED2, OCProvisionResult_t *UNU
 
 TEST(SRPProvisionACLTest, NullDeviceInfo)
 {
+    pDev1.doxm = &defaultDoxm1;
+    uint8_t deviceId1[] = {0x64, 0x65, 0x76, 0x69, 0x63, 0x65, 0x49, 0x64};
+    memcpy(pDev1.doxm->deviceID.id, deviceId1, sizeof(deviceId1));
+
+    pDev2.doxm = &defaultDoxm2;
+    uint8_t deviceId2[] = {0x64, 0x65, 0x76, 0x69, 0x63, 0x65, 0x49, 0x63};
+    memcpy(pDev2.doxm->deviceID.id, deviceId2, sizeof(deviceId2));
+
     EXPECT_EQ(OC_STACK_INVALID_PARAM, SRPProvisionACL(NULL, NULL, &acl, &provisioningCB));
 }
 
@@ -58,6 +95,13 @@ TEST(SRPProvisionCredentialsTest, NullDevice1)
                                                               &pDev2, &provisioningCB));
 }
 
+TEST(SRPProvisionCredentialsTest, SamelDeviceId)
+{
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, SRPProvisionCredentials(NULL, credType,
+                                                              OWNER_PSK_LENGTH_128, &pDev1,
+                                                              &pDev1, &provisioningCB));
+}
+
 TEST(SRPProvisionCredentialsTest, NullCallback)
 {
     EXPECT_EQ(OC_STACK_INVALID_CALLBACK, SRPProvisionCredentials(NULL, credType,
@@ -74,21 +118,22 @@ TEST(SRPProvisionCredentialsTest, InvalidKeySize)
 
 TEST(SRPUnlinkDevicesTest, NullDevice1)
 {
-    OCProvisionDev_t dev2;
-    EXPECT_EQ(OC_STACK_INVALID_PARAM, SRPUnlinkDevices(NULL, NULL, &dev2, provisioningCB));
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, SRPUnlinkDevices(NULL, NULL, &pDev2, provisioningCB));
 }
 
 TEST(SRPUnlinkDevicesTest, NullDevice2)
 {
-    OCProvisionDev_t dev1;
-    EXPECT_EQ(OC_STACK_INVALID_PARAM, SRPUnlinkDevices(NULL, &dev1, NULL, provisioningCB));
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, SRPUnlinkDevices(NULL, &pDev1, NULL, provisioningCB));
+}
+
+TEST(SRPUnlinkDevicesTest, SamelDeviceId)
+{
+    EXPECT_EQ(OC_STACK_INVALID_PARAM, SRPUnlinkDevices(NULL, &pDev1, &pDev1, provisioningCB));
 }
 
 TEST(SRPUnlinkDevicesTest, NullCallback)
 {
-    OCProvisionDev_t dev1;
-    OCProvisionDev_t dev2;
-    EXPECT_EQ(OC_STACK_INVALID_PARAM, SRPUnlinkDevices(NULL, &dev1, &dev2, NULL));
+    EXPECT_EQ(OC_STACK_INVALID_CALLBACK, SRPUnlinkDevices(NULL, &pDev1, &pDev2, NULL));
 }
 
 TEST(SRPRemoveDeviceTest, NullTargetDevice)
@@ -101,7 +146,7 @@ TEST(SRPRemoveDeviceTest, NullResultCallback)
 {
     unsigned short waitTime = 10;
     OCProvisionDev_t dev1;
-    EXPECT_EQ(OC_STACK_INVALID_PARAM, SRPRemoveDevice(NULL, waitTime, &dev1, NULL));
+    EXPECT_EQ(OC_STACK_INVALID_CALLBACK, SRPRemoveDevice(NULL, waitTime, &dev1, NULL));
 }
 
 TEST(SRPRemoveDeviceTest, ZeroWaitTime)