Thanks for raise the issue along with a fix! I have understood the fix to the buffer overrun problem in lcptools/lock.c. I still need some time to understand the other two issues & fixes. After that, I will check in your patch.
Thanks
Jimmy
charles.fis...@gdc4s.com wrote on 2012-12-12:
> All,
>
>
>
> While doing a routine update of a security review of the tboot code, I
found a
> couple of minor problems two potential (but very unlikely) buffer
overrun
> problems, and one minor memory leak although the program is going to
> terminate almost immediately, so the memory comes back anyway.
>
>
>
> At any rate, here is a patch to correct the problem.
>
>
>
> Signed Off by: Charles Fisher charles.fis...@gdc4s.com
>
>
>
> diff -up tboot-1.7.2/lcptools/crtpconf.c.orig
tboot-1.7.2/lcptools/crtpconf.c
>
> --- tboot-1.7.2/lcptools/crtpconf.c.orig 2012-12-11 13:16:12.239464000
-0700
>
> +++ tboot-1.7.2/lcptools/crtpconf.c 2012-12-11 16:00:23.097982000 -0700
>
> @@ -109,14 +109,12 @@ main(int argc, char *argv[])
>
> uint16_t i = 0;
>
> uint32_t index[MAX_INDEX] = {0};
>
> uint32_t idx_num = 0;
> - unsigned char *pcr_num[MAX_INDEX] = {NULL};
>
> FILE *p_file = NULL;
>
> unsigned char* srtm_data = NULL;
>
> uint32_t data_len = 0;
>
> TPM_LOCALITY_SELECTION local_sel;
>
> lcp_result_t ret_value = LCP_E_COMD_INTERNAL_ERR;
> - uint32_t temp = 0;
>
> /*
> * No parameter input will print out the help message.
> @@ -151,28 +149,13 @@ main(int argc, char *argv[])
>
> ret_value = LCP_E_INVALID_PARAMETER;
>
> goto _error_end;
> }
> -
>
> - for (i = 0; i < MAX_INDEX; i++) {
>
> - pcr_num[i] = (unsigned char *)malloc(10);
>
> - if ( pcr_num[i] == NULL ) {
>
> - ret_value = LCP_E_OUTOFMEMORY;
>
> - goto _error_end;
>
> - }
>
> - }
>
> - if ( str_split((char *)pcr_val, (char **)&pcr_num, &idx_num) < 0 ) {
>
> - ret_value = LCP_E_INVALID_PARAMETER;
>
> - goto _error_end;
>
> - }
>
> + idx_num = MAX_INDEX;
>
> + str_split((char *)pcr_val, index, &idx_num);
>
> for ( i = 0; i < idx_num; i++ ) {
> - if ( strtonum((char *)pcr_num[i], &temp) < 0 ) {
>
> - ret_value = LCP_E_INVALID_PARAMETER;
>
> - goto _error_end;
>
> - }
>
> - if ( temp > 23 ) {
>
> + if ( index[i] > 23 ) {
>
> ret_value = LCP_E_INVALID_PARAMETER;
>
> goto _error_end;
> - }
>
> - index[i] = temp;
>
> + }
>
> }
>
> local_sel = (TPM_LOCALITY_SELECTION)locality;
> @@ -200,8 +183,7 @@ main(int argc, char *argv[])
>
> fclose(p_file);
> } else
> print_hexmsg("the PConf data is:\n", data_len, srtm_data);
> - if(srtm_data)
>
> - free(srtm_data);
>
> + free(srtm_data);
>
> } else
> goto _error_end;
> @@ -210,10 +192,10 @@ _error_end:
>
> /*
> * Error when execute.
>
> */
> - for (i = 0; i < MAX_INDEX; i++)
>
> - free(pcr_num[i]);
>
> - free(srtm_data);
>
> + if (srtm_data)
>
> + free(srtm_data);
>
> log_error("\nCommand CrtPConf failed:\n");
>
> print_error(ret_value);
>
> return ret_value;
> -}
>
> + }
>
> +
>
> diff -up tboot-1.7.2/lcptools/lcputils.c.orig
tboot-1.7.2/lcptools/lcputils.c
>
> --- tboot-1.7.2/lcptools/lcputils.c.orig 2012-12-11 13:16:30.352217000
-0700
>
> +++ tboot-1.7.2/lcptools/lcputils.c 2012-12-11 15:44:03.076312000 -0700
>
> @@ -217,42 +217,22 @@ print_hexmsg(const char *header_msg, int
>
> }
>
> /* split the input string in the format: num1,num2,...,numN
> - * into the array = {num1, num2, ... , numN}
>
> + * into the numeric array = {num1, num2, ... , numN}
>
> */
>
> -int
>
> -str_split(const char *str_in, char **str_out, unsigned int *number)
>
> +void
>
> +str_split(char *str_in, uint32_t ints[], unsigned int *nr_ints)
>
> {
>
> - char * temp;
>
> - int num = 0;
>
> - const char *sep = ",";
>
> - size_t str_length = 0;
>
> - char *string = (char *)malloc(strlen(str_in) + 1);
>
> -
>
> - if ( string == NULL )
>
> - return -1;
>
> - if ( str_in == NULL || str_out == NULL || number == NULL ) {
>
> - free(string);
>
> - return -1;
>
> - }
>
> - strcpy(string, str_in);
>
> - temp =strtok(string, sep);
>
> - if ( temp != NULL && str_out[num] )
>
> - strcpy(str_out[num], temp);//strtok(string, sep));
>
> - while (str_out[num] != NULL) {
>
> - str_length += strlen(str_out[num]);
>
> - num++;
>
> - temp = strtok(NULL, sep);
>
> - if ( temp != NULL )
>
> - strcpy(str_out[num], temp);
>
> - else
>
> - str_out[num] = NULL;
>
> + unsigned int nr = 0;
>
> +
>
> + while ( true ) {
>
> + char *str = strsep(&str_in, ",");
>
> + if ( str == NULL || nr == *nr_ints )
>
> + break;
>
> + ints[nr++] = strtoul(str, NULL, 0);
>
> }
> - free(string);
>
> - *number = num;
>
> - str_length += num - 1;
>
> - if ( str_length != strlen(str_in) )
>
> - return -1;
>
> - return 0;
>
> + if ( nr == *nr_ints )
>
> + log_error("Error: too many items in list\n");
>
> + *nr_ints = nr;
>
> }
>
> uint16_t
> diff -up tboot-1.7.2/lcptools/lcputils.h.orig
tboot-1.7.2/lcptools/lcputils.h
>
> --- tboot-1.7.2/lcptools/lcputils.h.orig 2012-12-11 15:20:08.106747000
-0700
>
> +++ tboot-1.7.2/lcptools/lcputils.h 2012-12-11 15:42:34.009610000 -0700
>
> @@ -134,6 +134,6 @@ calc_sizeofselect(uint32_t num_indices,
>
> void print_locality(unsigned char loc);
>
> void print_permissions(UINT32 perms, const char *prefix);
>
> -int str_split(const char *str_in, char **str_out, unsigned int *number);
>
> +void str_split(char *str_in, uint32_t ints[], unsigned int *number);
>
> #endif
> diff -up tboot-1.7.2/lcptools/lock.c.orig tboot-1.7.2/lcptools/lock.c
>
> --- tboot-1.7.2/lcptools/lock.c.orig 2012-12-11 14:57:02.784235000
> -0700
>
> +++ tboot-1.7.2/lcptools/lock.c 2012-12-11 15:15:43.532763000 -0700
>
> @@ -91,7 +91,8 @@ parse_cmdline(int argc, const char * arg
>
> int
>
> main (int argc, char *argv[])
>
> {
>
> - char confirm_lock[1024] = {0};
>
> + char confirm_lock[4] = {0};
>
> + char c;
>
> in_nv_definespace_t in_defspace;
>
> lcp_result_t ret_value = LCP_E_COMD_INTERNAL_ERR;
> @@ -119,12 +120,12 @@ main (int argc, char *argv[])
>
> */
> do {
> log_info("Really want to lock TPM NV? (Y/N) ");
> - dummy = scanf("%s", confirm_lock);
>
> + dummy = scanf("%3s", confirm_lock);
>
> if ( dummy <= 0 )
> return LCP_E_COMD_INTERNAL_ERR;
> - } while (strcmp(confirm_lock, "N") && strcmp(confirm_lock, "n")
&&
>
> - strcmp(confirm_lock, "Y") && strcmp(confirm_lock, "y"));
>
> - if ( !strcmp(confirm_lock, "N") || !strcmp(confirm_lock, "n") ) {
>
> + c = confirm_lock[0] | ' ';
>
> + } while ( (c != 'n') && (c != 'y') );
>
> + if ( c == 'n') {
>
> ret_value = LCP_SUCCESS;
>
> return ret_value;
> }
>
smime.p7s
Description: S/MIME cryptographic signature
------------------------------------------------------------------------------ LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d
_______________________________________________ tboot-devel mailing list tboot-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tboot-devel
