Em 11-04-2014 06:45, Fuchs, Andreas escreveu: > Disclaimer: > I could not complie-test or runtime-test these patches right now. This is a > pure code-only review of the patches. > > inside ima_get_entries_by_pcr() name is unused... ?!?!? maybe the fread() > should be replaced with an fseek(fp, len, SEEK_CUR) in this case. However the > length check would make sense non-the-less I guess. > Same for digest. I'd also externalize the 20 for DIGEST_SIZE similar to > FILENAME_MAXSIZE Yes, name is not used in both functions. Go figure...
To be honest, I'm avoiding to make changes here as much as possible, since I'm still investigating if this code should be part of TrouSerS. It was created for use of IMA, but they don't use it that anymore, according to one IMA developer I talked to. So, the idea was just to fix the issue as quickly as possible. > > inside ima_get_entry() the same is true. > > Also, since I don't know, how name is used, I am not sure, if it needs to be > '\0'-terminated. If so, then the check would need to be (len > MAX_LEN - 1). > > Sorry, I cannot say anything about this patch, since I don't understand the > functions. From my perspective, it doesn't expect \0. But since name is not used anyway, doesn't matter. > > Am Mittwoch, den 09.04.2014, 15:41 -0300 schrieb [email protected]: >> From: Richard Maciel <[email protected]> >> >> Since the size of the name could be read from a file, but the buffer >> to contain it was fixed size, a check was needed to ensure that >> the fread doesn't overrun the buffer. >> >> Signed-off-by: Richard Maciel <[email protected]> >> --- >> src/tcs/tcs_evlog_imaem.c | 22 +++++++++++++++++----- >> 1 file changed, 17 insertions(+), 5 deletions(-) >> >> diff --git a/src/tcs/tcs_evlog_imaem.c b/src/tcs/tcs_evlog_imaem.c >> index 1771dbc..d905381 100644 >> --- a/src/tcs/tcs_evlog_imaem.c >> +++ b/src/tcs/tcs_evlog_imaem.c >> @@ -50,6 +50,8 @@ >> >> #ifdef EVLOG_SOURCE_IMA >> >> +#define EVLOG_FILENAME_MAXSIZE 255 >> + >> struct ext_log_source ima_source = { >> ima_open, >> ima_get_entries_by_pcr, >> @@ -84,7 +86,7 @@ ima_get_entries_by_pcr(FILE *handle, UINT32 pcr_index, >> UINT32 first, >> TSS_RESULT result = TCSERR(TSS_E_INTERNAL_ERROR); >> FILE *fp = (FILE *) handle; >> uint len; >> - char name[255]; >> + char name[EVLOG_FILENAME_MAXSIZE]; >> >> if (!fp) { >> LogError("File handle is NULL!\n"); >> @@ -132,8 +134,12 @@ ima_get_entries_by_pcr(FILE *handle, UINT32 pcr_index, >> UINT32 first, >> result = TCSERR(TSS_E_INTERNAL_ERROR); >> goto free_list; >> } >> - >> - memset(name, 0, sizeof name); >> + if (len > EVLOG_FILENAME_MAXSIZE) { >> + LogError("Event log file name too big! Max size is %d", >> EVLOG_FILENAME_MAXSIZE); >> + result = TCSERR(TSS_E_INTERNAL_ERROR); >> + goto free_list; >> + } >> + memset(name, 0, EVLOG_FILENAME_MAXSIZE); >> if (fread(name, 1, len, fp) != len) { >> LogError("Failed to read event log file"); >> result = TCSERR(TSS_E_INTERNAL_ERROR); >> @@ -229,7 +235,7 @@ ima_get_entry(FILE *handle, UINT32 pcr_index, UINT32 >> *num, TSS_PCR_EVENT **ppEve >> TSS_RESULT result = TCSERR(TSS_E_INTERNAL_ERROR); >> TSS_PCR_EVENT *event = NULL; >> FILE *fp = (FILE *) handle; >> - char name[255]; >> + char name[EVLOG_FILENAME_MAXSIZE]; >> >> rewind(fp); >> while (fread(page, 24, 1, fp)) { >> @@ -269,7 +275,13 @@ ima_get_entry(FILE *handle, UINT32 pcr_index, UINT32 >> *num, TSS_PCR_EVENT **ppEve >> result = >> TCSERR(TSS_E_INTERNAL_ERROR); >> goto done; >> } >> - memset(name, 0, sizeof name); >> + if (len > EVLOG_FILENAME_MAXSIZE) { >> + free(event); >> + LogError("Event log file name >> too big! Max size is %d", EVLOG_FILENAME_MAXSIZE); >> + result = >> TCSERR(TSS_E_INTERNAL_ERROR); >> + goto done; >> + } >> + memset(name, 0, EVLOG_FILENAME_MAXSIZE); >> if (fread(name, 1, len, fp) != len) { >> free(event); >> LogError("Failed to read event >> log file"); ------------------------------------------------------------------------------ Put Bad Developers to Shame Dominate Development with Jenkins Continuous Integration Continuously Automate Build, Test & Deployment Start a new project now. Try Jenkins in the cloud. http://p.sf.net/sfu/13600_Cloudbees _______________________________________________ TrouSerS-tech mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/trousers-tech
