From 660230c015022cf5ca258579b7e13d1b25571889 Mon Sep 17 00:00:00 2001 From: "Jon A. Cruz" Date: Thu, 12 Mar 2015 18:29:37 -0700 Subject: [PATCH] Fixed problems in URI parsing including memory corruption. Fixed issues in CAParseURI that included * Use of uint32_t instead of size_t. - Passing by address resulted in corruption on 64-bit builds and all others where the datatypes are different. This includes various embedded devices that have a size_t smaller than uint32_t. * Miscalculation of offset would freeze parsing once a few path components or parameters were processed. - While walking parsed results list the buffer-check addition would fail and all subsequent path components or parameters (depending on each independently) would repeat the contents of a prior item. Thus the end of path components and/or parameter parts of the list would be filled with repeats of a single value instead of the correct latter values. * Replaced duplicated code blocks with a single helper function. * Added unit tests for issue. However the test cases won't be runnable until after the build machine issue with connectivity/test is addressed. Change-Id: Ife3f327ae3bbda518d663bb0d6c5b4c0d8f98e9a Signed-off-by: Jon A. Cruz Reviewed-on: https://gerrit.iotivity.org/gerrit/470 Tested-by: jenkins-iotivity Reviewed-by: jihwan seo Reviewed-by: Joseph Morrow Reviewed-by: Erich Keane --- resource/csdk/connectivity/src/caprotocolmessage.c | 84 ++++----- .../connectivity/test/caprotocolmessagetest.cpp | 187 +++++++++++++++++++++ 2 files changed, 229 insertions(+), 42 deletions(-) create mode 100644 resource/csdk/connectivity/test/caprotocolmessagetest.cpp diff --git a/resource/csdk/connectivity/src/caprotocolmessage.c b/resource/csdk/connectivity/src/caprotocolmessage.c index 07ef6fd..fbbe5d8 100644 --- a/resource/csdk/connectivity/src/caprotocolmessage.c +++ b/resource/csdk/connectivity/src/caprotocolmessage.c @@ -35,6 +35,19 @@ static const char COAP_HEADER[] = "coap://[::]/"; static uint32_t SEED = 0; +/** + * Helper that uses libcoap to parse either the path or the parameters of a URI + * and populate the supplied options list. + * + * @param str the input partial URI string (either path or query). + * @param length the length of the supplied partial URI. + * @param target the part of the URI to parse (either COAP_OPTION_URI_PATH + * or COAP_OPTION_URI_QUERY). + * @param optlist options information. + */ +static void CAParseUriPartial(const unsigned char *str, size_t length, + int target, coap_list_t **optlist); + void CAGetRequestInfoFromPdu(const coap_pdu_t *pdu, CARequestInfo_t *outReqInfo, char *outUri, uint32_t buflen) { @@ -220,9 +233,9 @@ void CAParseURI(const char *uriInfo, coap_list_t **optlist) coap_uri_t uri; coap_split_uri((unsigned char *) uriInfo, strlen(uriInfo), &uri); - unsigned char portbuf[2]; if (uri.port != COAP_DEFAULT_PORT) { + unsigned char portbuf[2]; coap_insert( optlist, CACreateNewOptionNode(COAP_OPTION_URI_PORT, @@ -230,68 +243,55 @@ void CAParseURI(const char *uriInfo, coap_list_t **optlist) CAOrderOpts); } - unsigned char uriBuffer[CA_BUFSIZE] = - { 0 }; - unsigned char *pBuf = uriBuffer; - uint32_t buflen; - int32_t res; - if (uri.path.length) - { - buflen = CA_BUFSIZE; - res = coap_split_path(uri.path.s, uri.path.length, pBuf, &buflen); + CAParseUriPartial(uri.path.s, uri.path.length, COAP_OPTION_URI_PATH, optlist); - if (res > 0) - { - uint32_t prevIdx = 0; - while (res--) - { - coap_insert( - optlist, - CACreateNewOptionNode(COAP_OPTION_URI_PATH, COAP_OPT_LENGTH(pBuf), - COAP_OPT_VALUE(pBuf)), - CAOrderOpts); + CAParseUriPartial(uri.query.s, uri.query.length, COAP_OPTION_URI_QUERY, optlist); - uint32_t optSize = COAP_OPT_SIZE(pBuf); - uint32_t nextIdx = prevIdx + optSize; - if (nextIdx < buflen) - { - pBuf += optSize; - prevIdx += nextIdx; - } - } - } - } + OIC_LOG(DEBUG, TAG, "CAParseURI OUT"); +} - if (uri.query.length) +void CAParseUriPartial(const unsigned char *str, size_t length, + int target, coap_list_t **optlist) +{ + if ((target != COAP_OPTION_URI_PATH) && (target != COAP_OPTION_URI_QUERY)) { - buflen = CA_BUFSIZE; - pBuf = uriBuffer; - res = coap_split_query(uri.query.s, uri.query.length, pBuf, &buflen); + // should never occur. Log just in case. + OIC_LOG(DEBUG, TAG, "Unexpected URI component."); + } + else if (str && length) + { + unsigned char uriBuffer[CA_BUFSIZE] = { 0 }; + unsigned char *pBuf = uriBuffer; + size_t buflen = sizeof(uriBuffer); + int res = (target == COAP_OPTION_URI_PATH) ? + coap_split_path(str, length, pBuf, &buflen) : + coap_split_query(str, length, pBuf, &buflen); if (res > 0) { - uint32_t prevIdx = 0; + size_t prevIdx = 0; while (res--) { coap_insert( optlist, - CACreateNewOptionNode(COAP_OPTION_URI_QUERY, COAP_OPT_LENGTH(pBuf), + CACreateNewOptionNode(target, COAP_OPT_LENGTH(pBuf), COAP_OPT_VALUE(pBuf)), CAOrderOpts); - uint32_t optSize = COAP_OPT_SIZE(pBuf); - uint32_t nextIdx = prevIdx + optSize; - if (nextIdx < buflen) + size_t optSize = COAP_OPT_SIZE(pBuf); + if ((prevIdx + optSize) < buflen) { pBuf += optSize; - prevIdx += nextIdx; + prevIdx += optSize; } } } + else + { + OIC_LOG_V(DEBUG, TAG, "Problem parsing URI : %d for %d", res, target); + } } - - OIC_LOG(DEBUG, TAG, "CAParseURI OUT"); } void CAParseHeadOption(const uint32_t code, const CAInfo_t info, coap_list_t **optlist) diff --git a/resource/csdk/connectivity/test/caprotocolmessagetest.cpp b/resource/csdk/connectivity/test/caprotocolmessagetest.cpp new file mode 100644 index 0000000..12d6701 --- /dev/null +++ b/resource/csdk/connectivity/test/caprotocolmessagetest.cpp @@ -0,0 +1,187 @@ +//****************************************************************** +// +// Copyright 2015 Samsung Electronics All Rights Reserved. +// +//-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +//-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= + +#include + +#include "gtest/gtest.h" + +#include "caprotocolmessage.h" + +namespace { + +static const char COAP_HEADER[] = "coap://[::]/"; + +class CoAPOptionCase +{ +public: + unsigned short key; + unsigned int length; + std::string dataStr; // data could be binary... for testing we'll use str +}; + + + +/** + * Helper to validate the state of CoAP URI parsing. + * + * @param cases array of expected parse results. + * @param numCases number of expected parse results. + * @param optlist parsed option list to verify. + */ +void verifyParsedOptions(CoAPOptionCase const *cases, + size_t numCases, + coap_list_t *optlist) +{ + size_t index = 0; + for (coap_list_t *opt = optlist; opt; opt = opt->next) + { + coap_option *option = (coap_option *) opt->data; + EXPECT_TRUE(option != NULL); + EXPECT_LT(index, numCases); + if (option && (index < numCases)) + { + unsigned short key = COAP_OPTION_KEY(*option); + unsigned int length = COAP_OPTION_LENGTH(*option); + std::string dataStr((const char*)COAP_OPTION_DATA(*option), length); + // First validate the test case: + EXPECT_EQ(cases[index].length, cases[index].dataStr.length()); + + // Ensure data matches expected parsing + EXPECT_EQ(cases[index].key, key); + EXPECT_EQ(cases[index].length, length); + EXPECT_EQ(cases[index].dataStr, dataStr); + } + + index++; + } + // Ensure we saw the proper number of parts: + EXPECT_EQ(numCases, index); +} + +} // namespace + +TEST(CAProtocolMessage, CAParseURIBase) +{ + char sampleURI[] = "coap://[::]/oc/core?rt=core.sensor&if=core.mi.ll"; + + CoAPOptionCase cases[] = { + {COAP_OPTION_URI_PATH, 2, "oc"}, + {COAP_OPTION_URI_PATH, 4, "core"}, + {COAP_OPTION_URI_QUERY, 14, "rt=core.sensor"}, + {COAP_OPTION_URI_QUERY, 13, "if=core.mi.ll"}, + }; + size_t numCases = sizeof(cases) / sizeof(cases[0]); + + + coap_list_t *optlist = NULL; + CAParseURI(sampleURI, &optlist); + + + verifyParsedOptions(cases, numCases, optlist); +} + +// Try for multiple URI path components that still total less than 128 +TEST(CAProtocolMessage, CAParseURIManyPath) +{ + char sampleURI[] = "coap://[::]" + "/medium/a/b/c/d/e/f/g/h/i/j/" + "?rt=core.sensor&if=core.mi.ll"; + + CoAPOptionCase cases[] = { + {COAP_OPTION_URI_PATH, 6, "medium"}, + {COAP_OPTION_URI_PATH, 1, "a"}, + {COAP_OPTION_URI_PATH, 1, "b"}, + {COAP_OPTION_URI_PATH, 1, "c"}, + {COAP_OPTION_URI_PATH, 1, "d"}, + {COAP_OPTION_URI_PATH, 1, "e"}, + {COAP_OPTION_URI_PATH, 1, "f"}, + {COAP_OPTION_URI_PATH, 1, "g"}, + {COAP_OPTION_URI_PATH, 1, "h"}, + {COAP_OPTION_URI_PATH, 1, "i"}, + {COAP_OPTION_URI_PATH, 1, "j"}, + {COAP_OPTION_URI_QUERY, 14, "rt=core.sensor"}, + {COAP_OPTION_URI_QUERY, 13, "if=core.mi.ll"}, + }; + size_t numCases = sizeof(cases) / sizeof(cases[0]); + + + coap_list_t *optlist = NULL; + CAParseURI(sampleURI, &optlist); + + + verifyParsedOptions(cases, numCases, optlist); +} + +// Try for multiple URI parameters that still total less than 128 +TEST(CAProtocolMessage, CAParseURIManyParams) +{ + char sampleURI[] = "coap://[::]/oc/core/" + "?rt=core.sensor&a=0&b=1&c=2&d=3&e=4&f=5&g=6&h=7&i=8&j=9"; + + CoAPOptionCase cases[] = { + {COAP_OPTION_URI_PATH, 2, "oc"}, + {COAP_OPTION_URI_PATH, 4, "core"}, + {COAP_OPTION_URI_QUERY, 14, "rt=core.sensor"}, + {COAP_OPTION_URI_QUERY, 3, "a=0"}, + {COAP_OPTION_URI_QUERY, 3, "b=1"}, + {COAP_OPTION_URI_QUERY, 3, "c=2"}, + {COAP_OPTION_URI_QUERY, 3, "d=3"}, + {COAP_OPTION_URI_QUERY, 3, "e=4"}, + {COAP_OPTION_URI_QUERY, 3, "f=5"}, + {COAP_OPTION_URI_QUERY, 3, "g=6"}, + {COAP_OPTION_URI_QUERY, 3, "h=7"}, + {COAP_OPTION_URI_QUERY, 3, "i=8"}, + {COAP_OPTION_URI_QUERY, 3, "j=9"}, + }; + size_t numCases = sizeof(cases) / sizeof(cases[0]); + + + coap_list_t *optlist = NULL; + CAParseURI(sampleURI, &optlist); + + + verifyParsedOptions(cases, numCases, optlist); +} + +// Test that an initial long path component won't hide latter ones. +TEST(CAProtocolMessage, CAParseURILongPath) +{ + char sampleURI[] = "coap://[::]/oc" + "123456789012345678901234567890123456789012345678901234567890" + "12345678901234567890123456789012345678901234567890" + "/core?rt=core.sensor&if=core.mi.ll"; + + CoAPOptionCase cases[] = { + {COAP_OPTION_URI_PATH, 112, "oc" + "123456789012345678901234567890123456789012345678901234567890" + "12345678901234567890123456789012345678901234567890"}, + {COAP_OPTION_URI_PATH, 4, "core"}, + {COAP_OPTION_URI_QUERY, 14, "rt=core.sensor"}, + {COAP_OPTION_URI_QUERY, 13, "if=core.mi.ll"}, + }; + size_t numCases = sizeof(cases) / sizeof(cases[0]); + + + coap_list_t *optlist = NULL; + CAParseURI(sampleURI, &optlist); + + + verifyParsedOptions(cases, numCases, optlist); +} -- 2.7.4