Hi Lennart, Thanks for your comment. I have just sent out version #2 for review. Please have a look and give your comment. Thanks.
Regards, Vu. >-----Original Message----- >From: Lennart Lund [mailto:[email protected]] >Sent: Monday, May 09, 2016 8:13 PM >To: Vu Minh Nguyen; [email protected] >Cc: [email protected]; Beatriz Brandao; Lennart Lund >Subject: RE: [PATCH 1 of 1] log: verify logBufSize to avoid node malfunctioned >[#1789] > >Hi Vu, > >This fix has a problem. The log buffer consists of two variables, a pointer to a >buffer (*log_buf) that shall contain the data to be inserted at the @Cb or @Ci >token and a size (logBufSize). The check is correct if the @Cb token is used >since the AIS says that only \0 terminated strings can be used with this token >but if the @Ci token is used there is no such restriction (as far as I know). The >@Ci token gives a hexadecimal output of the content in log_buf and can be >used with application and system streams. Since there is no good (fast) way in >the agent to find information about what token that is used in the format >string (must be fetched from the log stream object) it is not recommended to >check here. >The only thing that can be verified is that logBufSize is not greater than >SA_LOG_MAX_RECORD_SIZE. On the server side some error handling could be >added if the @Cb token is used and there is no \0 termination within >logBufSize. Also if a \0 is found and @Cb token is used then the actual string >length could be used instead of given logBufSize. > >Thanks >Lennart > >> -----Original Message----- >> From: Vu Minh Nguyen [mailto:[email protected]] >> Sent: den 4 maj 2016 09:23 >> To: [email protected]; Lennart Lund >> Cc: [email protected] >> Subject: [PATCH 1 of 1] log: verify logBufSize to avoid node malfunctioned >> [#1789] >> >> osaf/libs/agents/saf/lga/lga_api.c | 18 +++++++++++++++++ >> tests/logsv/tet_saLogStreamOpen_2.c | 4 +++ >> tests/logsv/tet_saLogWriteLogAsync.c | 38 >> ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 60 insertions(+), 0 deletions(-) >> >> >> When accidentally passing an invalid value of logBufSize to >> saLogWriteLogAsync() >> such as a very large number which is caused by not using strlen() on logBuf, >> it will cause a lot of troubles. >> >> Add code to verify if logBufSize is calculated based on logBuf or not >> and also prevent too big data sent to server side. >> >> diff --git a/osaf/libs/agents/saf/lga/lga_api.c >> b/osaf/libs/agents/saf/lga/lga_api.c >> --- a/osaf/libs/agents/saf/lga/lga_api.c >> +++ b/osaf/libs/agents/saf/lga/lga_api.c >> @@ -996,6 +996,24 @@ static SaAisErrorT handle_log_record(con >> ais_rc = >> SA_AIS_ERR_INVALID_PARAM; >> goto done; >> } >> + >> + if (logRecord->logBuffer->logBuf != NULL) { >> + SaSizeT size = logRecord- >> >logBuffer->logBufSize; >> + bool sizeOver = size > strlen((char >> *)logRecord->logBuffer->logBuf) + 1; >> + /* Prevent log client accidently >> assign too big number to logBufSize */ >> + if (sizeOver == true) { >> + TRACE("logBufSize >> > strlen(logBuf) + 1"); >> + ais_rc = >> SA_AIS_ERR_INVALID_PARAM; >> + goto done; >> + } >> + >> + /* Prevent sending too big data to >> server side */ >> + if (size > >> SA_LOG_MAX_RECORD_SIZE) { >> + TRACE("logBuf data >> is too big (max: %d)", SA_LOG_MAX_RECORD_SIZE); >> + ais_rc = >> SA_AIS_ERR_INVALID_PARAM; >> + goto done; >> + } >> + } >> } >> >> /* Set timeStamp data if not provided by application user */ >> diff --git a/tests/logsv/tet_saLogStreamOpen_2.c >> b/tests/logsv/tet_saLogStreamOpen_2.c >> --- a/tests/logsv/tet_saLogStreamOpen_2.c >> +++ b/tests/logsv/tet_saLogStreamOpen_2.c >> @@ -724,6 +724,8 @@ extern void saLogWriteLogAsync_14(void); >> extern void saLogWriteLogAsync_15(void); >> extern void saLogWriteLogAsync_16(void); >> extern void saLogWriteLogAsync_17(void); >> +extern void saLogWriteLogAsync_18(void); >> +extern void saLogWriteLogAsync_19(void); >> extern void saLogWriteLogCallbackT_01(void); >> extern void saLogWriteLogCallbackT_02(void); >> extern void saLogWriteLogCallbackT_03(void); >> @@ -774,6 +776,8 @@ extern void saLogStreamClose_01(void); >> test_case_add(2, saLogWriteLogAsync_15, "saLogWriteAsyncLog() NTF >> notificationObject length == 256"); >> test_case_add(2, saLogWriteLogAsync_16, "saLogWriteAsyncLog() NTF >> notifyingObject length == 256"); >> test_case_add(2, saLogWriteLogAsync_17, "saLogWriteLogAsync() Generic >> logSvcUsrName length == 256"); >> + test_case_add(2, saLogWriteLogAsync_18, "saLogWriteLogAsync() >> logBufSize > strlen(logBuf) + 1"); >> + test_case_add(2, saLogWriteLogAsync_19, "saLogWriteLogAsync() >> logBufSize > SA_LOG_MAX_RECORD_SIZE"); >> test_case_add(2, saLogWriteLogCallbackT_01, "saLogWriteLogCallbackT() >> SA_DISPATCH_ONE"); >> test_case_add(2, saLogWriteLogCallbackT_02, "saLogWriteLogCallbackT() >> SA_DISPATCH_ALL"); >> test_case_add(2, saLogFilterSetCallbackT_01, "saLogFilterSetCallbackT >> OK"); >> diff --git a/tests/logsv/tet_saLogWriteLogAsync.c >> b/tests/logsv/tet_saLogWriteLogAsync.c >> --- a/tests/logsv/tet_saLogWriteLogAsync.c >> +++ b/tests/logsv/tet_saLogWriteLogAsync.c >> @@ -514,3 +514,41 @@ void saLogWriteLogAsync_17(void) >> test_validate(rc1, >> SA_AIS_ERR_INVALID_PARAM); >> } >> } >> + >> +/** >> + * saLogWriteAsyncLog() - logBufSize > strlen(logBuf) + 1 >> + */ >> +void saLogWriteLogAsync_18(void) >> +{ >> + SaInvocationT invocation = 0; >> + >> + strcpy((char*)genLogRecord.logBuffer->logBuf, >> __FUNCTION__); >> + genLogRecord.logBuffer->logBufSize = strlen(__FUNCTION__) >> + 2; >> + safassert(saLogInitialize(&logHandle, &logCallbacks, >> &logVersion), SA_AIS_OK); >> + safassert(saLogStreamOpen_2(logHandle, >> &systemStreamName, NULL, 0, >> + >> SA_TIME_ONE_SECOND, &logStreamHandle), SA_AIS_OK); >> + rc = saLogWriteLogAsync(logStreamHandle, invocation, 0, >> &genLogRecord); >> + safassert(saLogFinalize(logHandle), SA_AIS_OK); >> + test_validate(rc, SA_AIS_ERR_INVALID_PARAM); >> +} >> + >> +/** >> + * saLogWriteAsyncLog() - big logBufSize > SA_LOG_MAX_RECORD_SIZE >> + */ >> +void saLogWriteLogAsync_19(void) >> +{ >> + SaInvocationT invocation = 0; >> + char logBuf[SA_LOG_MAX_RECORD_SIZE + 10]; >> + >> + memset(logBuf, 'A', sizeof(logBuf)); >> + logBuf[sizeof(logBuf) - 1] = '\0'; >> + >> + genLogRecord.logBuffer->logBuf = (SaUint8T *)&logBuf; >> + genLogRecord.logBuffer->logBufSize = >> SA_LOG_MAX_RECORD_SIZE + 10; >> + safassert(saLogInitialize(&logHandle, &logCallbacks, >> &logVersion), SA_AIS_OK); >> + safassert(saLogStreamOpen_2(logHandle, >> &systemStreamName, NULL, 0, >> + >> SA_TIME_ONE_SECOND, &logStreamHandle), SA_AIS_OK); >> + rc = saLogWriteLogAsync(logStreamHandle, invocation, 0, >> &genLogRecord); >> + safassert(saLogFinalize(logHandle), SA_AIS_OK); >> + test_validate(rc, SA_AIS_ERR_INVALID_PARAM); >> +} ------------------------------------------------------------------------------ Mobile security can be enabling, not merely restricting. Employees who bring their own devices (BYOD) to work are irked by the imposition of MDM restrictions. Mobile Device Manager Plus allows you to control only the apps on BYO-devices by containerizing them, leaving personal data untouched! https://ad.doubleclick.net/ddm/clk/304595813;131938128;j _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
