[prev in list] [next in list] [prev in thread] [next in thread] 

List:       trousers-tech
Subject:    Re: [TrouSerS-tech] [PATCH 06/17] Added check to prevent buffer overflow in name buffer.
From:       "Fuchs, Andreas" <andreas.fuchs () sit ! fraunhofer ! de>
Date:       2014-04-11 14:07:26
Message-ID: 1397225246.15139.67.camel () pc-fuchslap2 ! sit ! fraunhofer ! de
[Download RAW message or body]

Am Freitag, den 11.04.2014, 10:24 -0300 schrieb Richard:
> 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.

Well, I'd guess that the patch is correct, but I'd not know for sure... ;-)

> > 
> > 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 rmaciel@linux.vnet.ibm.com:
> > > From: Richard Maciel <rmaciel@linux.vnet.ibm.com>
> > > 
> > > 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 <rmaciel@linux.vnet.ibm.com>
> > > ---
> > > 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
TrouSerS-tech@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/trousers-tech


[prev in list] [next in list] [prev in thread] [next in thread]