[IOT-2726][IOT-2107] doxm POST handler fix 19/22619/19
authorOleksandr Dmytrenko <o.dmytrenko@samsung.com>
Wed, 27 Sep 2017 11:59:06 +0000 (14:59 +0300)
committerNathan Heldt-Sheller <nathan.heldt-sheller@intel.com>
Thu, 19 Oct 2017 22:03:27 +0000 (22:03 +0000)
Fixed doxm POST handler modifies values without request from OBT

Change-Id: I4631063002ebf830160b772f55c9a9f139b78dbc
Signed-off-by: Oleksandr Dmytrenko <o.dmytrenko@samsung.com>
Signed-off-by: Dan Mihai <Daniel.Mihai@microsoft.com>
resource/csdk/security/src/aclresource.c
resource/csdk/security/src/doxmresource.c
resource/csdk/security/src/pstatresource.c

index 2152240..046f9f3 100644 (file)
@@ -2943,29 +2943,16 @@ static OCEntityHandlerResult HandleACLPostRequest(const OCEntityHandlerRequest *
                     }
                 }
             }
-            memcpy(&(gAcl->rownerID), &(newAcl->rownerID), sizeof(OicUuid_t));
 
-            DeleteACLList(newAcl);
-
-            if(OC_EH_OK == ehRet)
+            // set acl rowner id and save
+            OCStackResult ownerRes = SetAclRownerId(&newAcl->rownerID);
+            if (OC_STACK_OK != ownerRes && OC_STACK_NO_RESOURCE != ownerRes)
             {
-                size_t cborSize = 0;
-                uint8_t *cborPayload = NULL;
-
-                if (OC_STACK_OK == AclToCBORPayload(gAcl, OIC_SEC_ACL_V2, &cborPayload, &cborSize))
-                {
-                    if (UpdateSecureResourceInPS(OIC_JSON_ACL_NAME, cborPayload, cborSize) == OC_STACK_OK)
-                    {
-                        ehRet = OC_EH_CHANGED;
-                    }
-                    OICFree(cborPayload);
-                }
-
-                if(OC_EH_CHANGED != ehRet)
-                {
-                    ehRet = OC_EH_ERROR;
-                }
+                OIC_LOG_V(ERROR, TAG, "%s: set acl RownerId", __func__);
+                ehRet = OC_EH_ERROR;
             }
+
+            DeleteACLList(newAcl);
         }
         else
         {
@@ -3076,29 +3063,16 @@ static OCEntityHandlerResult HandleACL2PostRequest(const OCEntityHandlerRequest
                     ehRet = OC_EH_ERROR;
                 }
             }
-            memcpy(&(gAcl->rownerID), &(newAcl->rownerID), sizeof(OicUuid_t));
 
-            DeleteACLList(newAcl);
-
-            if(OC_EH_OK == ehRet)
+            // set acl rowner id and save
+            OCStackResult ownerRes = SetAclRownerId(&newAcl->rownerID);
+            if (OC_STACK_OK != ownerRes && OC_STACK_NO_RESOURCE != ownerRes)
             {
-                size_t cborSize = 0;
-                uint8_t *cborPayload = NULL;
-
-                if (OC_STACK_OK == AclToCBORPayload(gAcl, OIC_SEC_ACL_V2, &cborPayload, &cborSize))
-                {
-                    if (UpdateSecureResourceInPS(OIC_JSON_ACL_NAME, cborPayload, cborSize) == OC_STACK_OK)
-                    {
-                        ehRet = OC_EH_CHANGED;
-                    }
-                    OICFree(cborPayload);
-                }
-
-                if(OC_EH_CHANGED != ehRet)
-                {
-                    ehRet = OC_EH_ERROR;
-                }
+                OIC_LOG_V(ERROR, TAG, "%s: set acl RownerId", __func__);
+                ehRet = OC_EH_ERROR;
             }
+
+            DeleteACLList(newAcl);
         }
         else
         {
index 957fd5f..8dc70ee 100644 (file)
@@ -961,82 +961,67 @@ static OCEntityHandlerResult HandleDoxmGetRequest (const OCEntityHandlerRequest
     return ehRet;
 }
 
-OCStackResult DoxmUpdateWriteableProperty(const OicSecDoxm_t* src, OicSecDoxm_t* dst)
+void printDoxm(const OicSecDoxm_t* doxm)
 {
-    OCStackResult result = OC_STACK_OK;
+    char uuidString[UUID_STRING_SIZE] = { 0 };
 
-    if(src && dst)
+    if (OCConvertUuidToString(doxm->owner.id, uuidString))
+    {
+        OIC_LOG_V(DEBUG, TAG, "%s: doxm.devowneruuid = %s", __func__, uuidString);
+    }
+    if (OCConvertUuidToString(doxm->rownerID.id, uuidString))
+    {
+        OIC_LOG_V(DEBUG, TAG, "%s: doxm.rowneruuid = %s", __func__, uuidString);
+    }
+    if (OCConvertUuidToString(doxm->deviceID.id, uuidString))
     {
-        // Update oxmsel
-        dst->oxmSel = src->oxmSel;
-        OIC_LOG_V(DEBUG, TAG, "%s: updated doxm.oxmsel = %d", __func__,
-            (int)dst->oxmSel);
+        OIC_LOG_V(DEBUG, TAG, "%s: doxm.deviceuuid = %s", __func__, uuidString);
+    }
+}
 
-        // Update devowneruuid
-        memcpy(&(dst->owner), &(src->owner), sizeof(OicUuid_t));
-#ifndef NDEBUG // if debug build, log the new uuid
-        char uuidString[UUID_STRING_SIZE] = { 0 };
-        bool convertedUUID = OCConvertUuidToString(dst->owner.id, uuidString);
-        if (convertedUUID)
-        {
-            OIC_LOG_V(DEBUG, TAG, "%s: updated doxm.devowneruuid = %s", __func__,
-                uuidString);
-        }
-#endif
+OCStackResult DoxmUpdateWriteableProperty(const OicSecDoxm_t* src, OicSecDoxm_t* dst)
+{
+    OIC_LOG_V(DEBUG, TAG, "IN: %s", __func__);
+    OCStackResult result = OC_STACK_OK;
 
-        // Update rowneruuid
-        memcpy(&(dst->rownerID), &(src->rownerID), sizeof(OicUuid_t));
-#ifndef NDEBUG // if debug build, log the new uuid
-        convertedUUID = OCConvertUuidToString(dst->rownerID.id, uuidString);
-        if (convertedUUID)
-        {
-            OIC_LOG_V(DEBUG, TAG, "%s: updated doxm.rowneruuid = %s", __func__,
-                uuidString);
-        }
-#endif
+    VERIFY_NOT_NULL_RETURN(TAG, src, ERROR, OC_STACK_INVALID_PARAM);
+    VERIFY_NOT_NULL_RETURN(TAG, dst, ERROR, OC_STACK_INVALID_PARAM);
 
-        // Update deviceuuid
-        memcpy(&(dst->deviceID), &(src->deviceID), sizeof(OicUuid_t));
-#ifndef NDEBUG // if debug build, log the new uuid
-        convertedUUID = OCConvertUuidToString(dst->deviceID.id, uuidString);
-        if (convertedUUID)
-        {
-            OIC_LOG_V(DEBUG, TAG, "%s: updated doxm.deviceuuid = %s", __func__,
-                uuidString);
-        }
-#endif
+    // Update oxmsel
+    dst->oxmSel = src->oxmSel;
+    OIC_LOG_V(DEBUG, TAG, "%s: updated doxm.oxmsel = %d", __func__, (int)dst->oxmSel);
 
-        // Update owned status
-        if(dst->owned != src->owned)
-        {
-            dst->owned = src->owned;
-            OIC_LOG_V(DEBUG, TAG, "%s: updated owned = %s", __func__,
-                dst->owned?"true":"false");
-        }
+    // Update deviceuuid
+    memcpy(&(dst->deviceID), &(src->deviceID), sizeof(OicUuid_t));
+    // Update rowneruuid
+    memcpy(&(dst->rownerID), &(src->rownerID), sizeof(OicUuid_t));
+    // Update devowneruuid
+    memcpy(&(dst->owner), &(src->owner), sizeof(OicUuid_t));
 
 #ifdef MULTIPLE_OWNER
-        if(src->mom)
+    if(src->mom)
+    {
+        OIC_LOG(DEBUG, TAG, "Detected 'mom' property");
+        if(NULL == dst->mom)
         {
-            OIC_LOG(DEBUG, TAG, "Detected 'mom' property");
-            if(NULL == dst->mom)
-            {
-                dst->mom = (OicSecMom_t*)OICCalloc(1, sizeof(OicSecMom_t));
-                if (NULL == dst->mom)
-                {
-                    result = OC_STACK_NO_MEMORY;
-                }
-            }
-
-            if (NULL != dst->mom)
-            {
-                dst->mom->mode = src->mom->mode;
-                OIC_LOG_V(DEBUG, TAG, "%s: updated mom->mode = %d", __func__,
-                    (int)dst->mom->mode);
-            }
+            result = OC_STACK_NO_MEMORY;
+            dst->mom = (OicSecMom_t*)OICCalloc(1, sizeof(OicSecMom_t));
+            VERIFY_NOT_NULL(TAG, dst->mom, ERROR);
+            result = OC_STACK_OK;
         }
-#endif //MULTIPLE_OWNER
+
+        dst->mom->mode = src->mom->mode;
+        OIC_LOG_V(DEBUG, TAG, "%s: updated mom->mode = %d", __func__, (int)dst->mom->mode);
     }
+#endif //MULTIPLE_OWNER
 
+#ifndef NDEBUG // if debug build, log the new uuid
+    printDoxm(dst);
+#endif
+#ifdef MULTIPLE_OWNER
+exit:
+#endif
+    OIC_LOG_V(DEBUG, TAG, "OUT: %s", __func__);
     return result;
 }
 
@@ -1262,27 +1247,27 @@ exit:
 #endif //MULTIPLE_OWNER
 #endif // __WITH_DTLS__ or __WITH_TLS__
 
-int HandleDoxmPostRequestSVR()
+OCEntityHandlerResult HandleDoxmPostRequestSetROwnerId()
 {
-    OCStackResult ownerRes = SetAclRownerId(&gDoxm->owner);
+    OCStackResult ownerRes = SetAclRownerId(&gDoxm->rownerID);
     if (OC_STACK_OK != ownerRes && OC_STACK_NO_RESOURCE != ownerRes)
     {
         OIC_LOG_V(ERROR, TAG, "%s: set acl RownerId", __func__);
-        return 1;
+        return OC_EH_ERROR;
     }
-    ownerRes = SetCredRownerId(&gDoxm->owner);
+    ownerRes = SetCredRownerId(&gDoxm->rownerID);
     if (OC_STACK_OK != ownerRes && OC_STACK_NO_RESOURCE != ownerRes)
     {
         OIC_LOG_V(ERROR, TAG, "%s: set cred RownerId", __func__);
-        return 1;
+        return OC_EH_ERROR;
     }
-    ownerRes = SetPstatRownerId(&gDoxm->owner);
+    ownerRes = SetPstatRownerId(&gDoxm->rownerID);
     if (OC_STACK_OK != ownerRes && OC_STACK_NO_RESOURCE != ownerRes)
     {
         OIC_LOG_V(ERROR, TAG, "%s: set pstat RownerId", __func__);
-        return 1;
+        return OC_EH_ERROR;
     }
-    return 0;
+    return OC_EH_OK;
 }
 
 OCEntityHandlerResult HandleDoxmPostRequestUpdatePS(bool fACE)
@@ -1566,6 +1551,7 @@ static OCEntityHandlerResult HandleDoxmPostRequest(OCEntityHandlerRequest *ehReq
     OCEntityHandlerResult ehRet = OC_EH_INTERNAL_SERVER_ERROR;
     static uint16_t previousMsgId = 0;
     bool isDuplicatedMsg = false;
+    bool fACE = false;
 
     /*
      * Convert CBOR Doxm data into binary. This will also validate
@@ -1610,7 +1596,7 @@ static OCEntityHandlerResult HandleDoxmPostRequest(OCEntityHandlerRequest *ehReq
     VERIFY_NOT_NULL(TAG, gDoxm, ERROR);
 
     // in owned state
-    if (true == gDoxm->owned)
+    if (true == gDoxm->owned && !UuidCmp(&gDoxm->owner, &newDoxm->owner))
     {
         if (false == ValidateOxmsel(gDoxm->oxm, gDoxm->oxmLen, &newDoxm->oxmSel))
         {
@@ -1634,79 +1620,69 @@ static OCEntityHandlerResult HandleDoxmPostRequest(OCEntityHandlerRequest *ehReq
         HandleDoxmPostRequestMom(newDoxm, ehRequest);
 #endif //MULTIPLE_OWNER
 #endif // defined(__WITH_DTLS__) || defined (__WITH_TLS__)
-
-        //Update new state in persistent storage
-        ehRet = HandleDoxmPostRequestUpdatePS(false);
-        goto exit;
     }
-
-    // in unowned state
-    // TODO [IOT-2107] this logic assumes that the only POST to /doxm in
-    // unowned state is either a) changing to owned or b) setting oxmsel and
-    // therefore (in case b) should enable the proper cipher for OTM.  But it's
-    // allowable for Client to be posting other things such as /doxm.rowneruuid
-    // when owned == false, too.  Added a workaround (see 'workaround' below)
-    // but this POST handler needs to be fixed per IOT-2107.
-    if ((false == gDoxm->owned) && (false == newDoxm->owned))
+    else // unowned: false == gDoxm->owned
     {
-        if (false == ValidateOxmsel(gDoxm->oxm, gDoxm->oxmLen, &newDoxm->oxmSel))
+        OicSecDostype_t dos;
+
+        VERIFY_SUCCESS(TAG, OC_STACK_OK == GetDos(&dos), ERROR);
+
+        if (DOS_RESET == dos.state ||
+            DOS_RFNOP == dos.state)
         {
-            OIC_LOG(ERROR, TAG, "Not acceptable request because oxmsel does not support on Server");
+            OIC_LOG_V(ERROR, TAG, "%s /doxm resource is read-only in RESET or RFNOP", __func__);
             ehRet = OC_EH_NOT_ACCEPTABLE;
             goto exit;
         }
 
-        // workaround
-        // We wouldn't be at this point in the
-        // code if the POST contained R-only Properties for the current /pstat.dos.s
-        // state, so we want to update writeable properties now that we've validated
-        // oxmsel is a valid oxm for this device.
-        res = DoxmUpdateWriteableProperty(newDoxm, gDoxm);
-        if (OC_STACK_OK != res)
-        {
-            OIC_LOG(ERROR, TAG,
-                    "gDoxm properties were not able to be updated so we cannot handle the request.");
-            ehRet = OC_EH_ERROR;
-            goto exit;
-        }
+        OIC_LOG_V(WARNING, TAG, "%s: OTM %s", __func__, newDoxm->owned ? "stop" : "start");
 
-        ehRet = HandleDoxmPostRequestSrv(newDoxm, ehRequest, isDuplicatedMsg);
-        VERIFY_SUCCESS(TAG, OC_EH_OK == ehRet, ERROR);
+        if (false == newDoxm->owned && !UuidCmp(&gDoxm->owner, &newDoxm->owner))
+        {
+            if (false == ValidateOxmsel(gDoxm->oxm, gDoxm->oxmLen, &newDoxm->oxmSel))
+            {
+                OIC_LOG(ERROR, TAG, "Not acceptable request because oxmsel does not support on Server");
+                ehRet = OC_EH_NOT_ACCEPTABLE;
+                goto exit;
+            }
 
-        // Update new state in persistent storage
-        ehRet = HandleDoxmPostRequestUpdatePS(false);
-    }
+            res = DoxmUpdateWriteableProperty(newDoxm, gDoxm);
+            if (OC_STACK_OK != res)
+            {
+                OIC_LOG(ERROR, TAG,
+                    "gDoxm properties were not able to be updated so we cannot handle the request.");
+                ehRet = OC_EH_ERROR;
+                goto exit;
+            }
 
-    /*
-     * When current state of the device is un-owned and Provisioning
-     * Tool is attempting to change the state to 'Owned' with a
-     * qualified value for the field 'Owner'
-     */
-    // TODO [IOT-2107] reconcile POST handler behavior with Specification
-    if ((false == gDoxm->owned) && (true == newDoxm->owned) &&
-            UuidCmp(&gDoxm->owner, &newDoxm->owner))
-    {
-        if (HandleDoxmPostRequestSVR())
-        {
-            ehRet = OC_EH_ERROR;
-            goto exit;
+            ehRet = HandleDoxmPostRequestSrv(newDoxm, ehRequest, isDuplicatedMsg);
+            VERIFY_SUCCESS(TAG, OC_EH_OK == ehRet, ERROR);
         }
+        else // finish OTM
+        {
+            if (!gDoxm->owned &&
+                !IsNilUuid(&gDoxm->owner) &&
+                !UuidCmp(&gDoxm->owner, &gDoxm->rownerID))
+            {
+                memcpy(&gDoxm->rownerID, &gDoxm->owner, sizeof(OicUuid_t));
 
-        gDoxm->owned = true;
-        memcpy(&gDoxm->rownerID, &gDoxm->owner, sizeof(OicUuid_t));
-
-        // Update new state in persistent storage
-        ehRet = HandleDoxmPostRequestUpdatePS(true);
+                gDoxm->owned = true;
+                fACE = true;
+            }
+        }
 
-#if defined(__WITH_DTLS__) || defined (__WITH_TLS__)
-        if (OIC_MANUFACTURER_CERTIFICATE == gDoxm->oxmSel ||
+ #if defined(__WITH_DTLS__) || defined (__WITH_TLS__)
+       if (OIC_MANUFACTURER_CERTIFICATE == gDoxm->oxmSel ||
             OIC_CON_MFG_CERT == gDoxm->oxmSel)
         {
             CAregisterPkixInfoHandler(GetPkixInfo);
             CAregisterGetCredentialTypesHandler(InitCipherSuiteList);
         }
 #endif // __WITH_DTLS__ or __WITH_TLS__
-    }
+
+        // Update new state in persistent storage
+        ehRet = HandleDoxmPostRequestUpdatePS(fACE);
+   }//true == gDoxm->owned
 exit:
     if (OC_EH_OK != ehRet)
     {
index 0ca4920..e8c9466 100644 (file)
@@ -854,19 +854,6 @@ static OCEntityHandlerResult HandlePstatPostRequest(OCEntityHandlerRequest *ehRe
                 pstat->cm &= ~UPDATE_SOFTWARE; // Unset the cm bit, per spec
             }
 
-            // update om
-            gPstat->om = pstat->om;
-
-            // update tm
-            OIC_LOG_V(INFO, TAG, "%s setting gPstat->tm = %u", __func__, pstat->tm);
-            gPstat->tm = pstat->tm;
-
-            // update rownerID
-            gPstat->rownerID = pstat->rownerID;
-
-            // update dos LAST of all Properties, as changing dos can also
-            // change other Properties and we want the dos-asserted values
-            // to "stick" rather than being over-written by prior values.
             if (pstat->dos.state != gPstat->dos.state)
             {
                 OCStackResult stateChangeResult = OC_STACK_ERROR;
@@ -897,11 +884,27 @@ static OCEntityHandlerResult HandlePstatPostRequest(OCEntityHandlerRequest *ehRe
                 }
             }
 
+            // update om
+            gPstat->om = pstat->om;
+
+            // update tm
+            OIC_LOG_V(INFO, TAG, "%s setting gPstat->tm = %u", __func__, pstat->tm);
+            gPstat->tm = pstat->tm;
+
+            // set rowner and save
+            OicUuid_t prevId = {.id={0}};
+            memcpy(&prevId, &gPstat->rownerID, sizeof(OicUuid_t));
+            memcpy(&gPstat->rownerID, &pstat->rownerID, sizeof(OicUuid_t));
+
             // Convert pstat data into CBOR for update to persistent storage
             if (UpdatePersistentStorage(gPstat))
             {
                 ehRet = OC_EH_OK;
             }
+            else
+            {
+                memcpy(&gPstat->rownerID, &prevId, sizeof(OicUuid_t));
+            }
         }
     }
 
@@ -1107,40 +1110,22 @@ OCStackResult GetPstatRownerId(OicUuid_t *rowneruuid)
 
 OCStackResult SetPstatRownerId(const OicUuid_t *rowneruuid)
 {
-    OCStackResult ret = OC_STACK_ERROR;
-    uint8_t *cborPayload = NULL;
-    size_t size = 0;
+    OCStackResult ret = OC_STACK_OK;
     OicUuid_t prevId = {.id={0}};
 
-    if(NULL == rowneruuid)
-    {
-        ret = OC_STACK_INVALID_PARAM;
-    }
-    if(NULL == gPstat)
-    {
-        ret = OC_STACK_NO_RESOURCE;
-    }
-
-    if(rowneruuid && gPstat)
-    {
-        memcpy(prevId.id, gPstat->rownerID.id, sizeof(prevId.id));
-        memcpy(gPstat->rownerID.id, rowneruuid->id, sizeof(gPstat->rownerID.id));
-
-        ret = PstatToCBORPayload(gPstat, &cborPayload, &size);
-        VERIFY_SUCCESS(TAG, OC_STACK_OK == ret, ERROR);
+    VERIFY_NOT_NULL_RETURN(TAG, rowneruuid, ERROR, OC_STACK_INVALID_PARAM);
+    VERIFY_NOT_NULL_RETURN(TAG, gPstat, ERROR, OC_STACK_NO_RESOURCE);
 
-        ret = UpdateSecureResourceInPS(OIC_JSON_PSTAT_NAME, cborPayload, size);
-        VERIFY_SUCCESS(TAG, OC_STACK_OK == ret, ERROR);
+    memcpy(&prevId, &gPstat->rownerID, sizeof(OicUuid_t));
+    memcpy(&gPstat->rownerID, rowneruuid, sizeof(OicUuid_t));
 
-        OICFree(cborPayload);
+    if (!UpdatePersistentStorage(gPstat))
+    {
+        memcpy(&gPstat->rownerID, &prevId, sizeof(OicUuid_t));
+        ret = OC_STACK_ERROR;
     }
 
     return ret;
-
-exit:
-    OICFree(cborPayload);
-    memcpy(gPstat->rownerID.id, prevId.id, sizeof(gPstat->rownerID.id));
-    return ret;
 }
 
 OCStackResult GetPstatDosS(OicSecDeviceOnboardingState_t *s)