From f625e3cc5d3d3ef21b6145501cc4a25d428949ad Mon Sep 17 00:00:00 2001 From: Philippe Coval Date: Fri, 13 Jan 2017 15:11:44 +0900 Subject: [PATCH] security: Fix to access dereferenced object memory in OCSecure class [Joonghwan Lee] This patch fix below invalid memory access in OCSecure class - invalid DisplayNumContext object access - invalid UserConfirmNumContext object access - minor fix : prevent duplicated call of SetResult() [Philippe Coval] Ported from 1.2-rel branch to master Conflicts: resource/csdk/security/provisioning/src/ownershiptransfermanager.c Change-Id: I623c856f7086cf7fe7333e69f682070434961fbe Signed-off-by: Joonghwan Lee Reviewed-on: https://gerrit.iotivity.org/gerrit/16369 Tested-by: jenkins-iotivity Reviewed-by: Chul Lee Reviewed-by: Randeep Singh Author: Joonghwan Lee Signed-off-by: Philippe Coval Reviewed-on: https://gerrit.iotivity.org/gerrit/16949 --- resource/csdk/security/include/oxmverifycommon.h | 4 +- .../provisioning/src/ownershiptransfermanager.c | 3 ++ resource/csdk/security/src/oxmverifycommon.c | 10 +++- .../provisioning/src/OCProvisioningManager.cpp | 62 ++++++++++++++++------ 4 files changed, 59 insertions(+), 20 deletions(-) diff --git a/resource/csdk/security/include/oxmverifycommon.h b/resource/csdk/security/include/oxmverifycommon.h index a90eaef..bc1b2df 100644 --- a/resource/csdk/security/include/oxmverifycommon.h +++ b/resource/csdk/security/include/oxmverifycommon.h @@ -80,7 +80,7 @@ void SetDisplayNumCB(void * ptr, DisplayNumCallback displayNumCB); /* * Unset Callback for displaying verification PIN */ -void UnsetDisplayNumCB(); +void* UnsetDisplayNumCB(); /* * Set Callback for getting user confirmation @@ -90,7 +90,7 @@ void SetUserConfirmCB(void * ptr, UserConfirmCallback userConfirmCB); /* * Unset Callback for getting user confirmation */ -void UnsetUserConfirmCB(); +void* UnsetUserConfirmCB(); /* * Set verification method option. diff --git a/resource/csdk/security/provisioning/src/ownershiptransfermanager.c b/resource/csdk/security/provisioning/src/ownershiptransfermanager.c index 3b841ec..6d73f8f 100644 --- a/resource/csdk/security/provisioning/src/ownershiptransfermanager.c +++ b/resource/csdk/security/provisioning/src/ownershiptransfermanager.c @@ -984,6 +984,9 @@ static OCStackApplicationResult OwnerUuidUpdateHandler(void *ctx, OCDoHandle UNU { OIC_LOG(WARNING, TAG, "OwnerUuidUpdateHandler : SRPResetDevice error"); } + OIC_LOG(ERROR, TAG, "OwnerUuidUpdateHandler:Failed to verify user confirm"); + SetResult(otmCtx, res); + return OC_STACK_DELETE_TRANSACTION; } } diff --git a/resource/csdk/security/src/oxmverifycommon.c b/resource/csdk/security/src/oxmverifycommon.c index 3f13ed7..169fb65 100644 --- a/resource/csdk/security/src/oxmverifycommon.c +++ b/resource/csdk/security/src/oxmverifycommon.c @@ -44,9 +44,12 @@ void SetDisplayNumCB(void * ptr, DisplayNumCallback displayNumCB) gDisplayNumContext.context = ptr; } -void UnsetDisplayNumCB() +void* UnsetDisplayNumCB() { + void *prevctx = gDisplayNumContext.context; gDisplayNumContext.callback = NULL; + gDisplayNumContext.context= NULL; + return prevctx; } void SetUserConfirmCB(void * ptr, UserConfirmCallback userConfirmCB) @@ -60,9 +63,12 @@ void SetUserConfirmCB(void * ptr, UserConfirmCallback userConfirmCB) gUserConfirmContext.context = ptr; } -void UnsetUserConfirmCB() +void* UnsetUserConfirmCB() { + void *prevctx = gUserConfirmContext.context; gUserConfirmContext.callback = NULL; + gUserConfirmContext.context = NULL; + return prevctx; } void SetVerifyOption(VerifyOptionBitmask_t verifyOption) diff --git a/resource/provisioning/src/OCProvisioningManager.cpp b/resource/provisioning/src/OCProvisioningManager.cpp index 1e08a3c..5590065 100644 --- a/resource/provisioning/src/OCProvisioningManager.cpp +++ b/resource/provisioning/src/OCProvisioningManager.cpp @@ -736,14 +736,21 @@ namespace OC OCStackResult OCSecure::displayNumCallbackWrapper(void* ctx, uint8_t verifNum[MUTUAL_VERIF_NUM_LEN]) { - OCStackResult result; + uint8_t *number = NULL; DisplayNumContext* context = static_cast(ctx); - uint8_t *number = new uint8_t[MUTUAL_VERIF_NUM_LEN]; - memcpy(number, verifNum, MUTUAL_VERIF_NUM_LEN); - result = context->callback(number); - delete context; - return result; + if (!context) + { + oclog() << "Invalid context"; + return OC_STACK_INVALID_PARAM; + } + + if (NULL != verifNum) { + number = new uint8_t[MUTUAL_VERIF_NUM_LEN]; + memcpy(number, verifNum, MUTUAL_VERIF_NUM_LEN); + } + + return context->callback(number); } OCStackResult OCSecure::registerDisplayNumCallback(DisplayNumCB displayNumCB) @@ -754,9 +761,14 @@ namespace OC return OC_STACK_INVALID_CALLBACK; } - OCStackResult result; - auto cLock = OCPlatform_impl::Instance().csdkLock().lock(); + OCStackResult result = OCSecure::deregisterDisplayNumCallback(); + if (OC_STACK_OK != result) + { + oclog() << "Failed to de-register callback for display."< lock(*cLock); - UnsetDisplayNumCB(); + DisplayNumContext* context = static_cast(UnsetDisplayNumCB()); + if (context) + { + oclog() << "Delete registered display num context"<(ctx); - result = context->callback(); - delete context; - return result; + if (!context) + { + oclog() << "Invalid context"; + return OC_STACK_INVALID_PARAM; + } + + return context->callback(); } OCStackResult OCSecure::registerUserConfirmCallback(UserConfirmNumCB userConfirmCB) @@ -809,7 +828,13 @@ namespace OC return OC_STACK_INVALID_CALLBACK; } - OCStackResult result; + OCStackResult result = OCSecure::deregisterUserConfirmCallback(); + if (OC_STACK_OK != result) + { + oclog() << "Failed to de-register callback for comfirm."< lock(*cLock); - UnsetUserConfirmCB(); + UserConfirmNumContext* context = static_cast(UnsetUserConfirmCB()); + if (context) + { + oclog() << "Delete registered user confirm context"<