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.fisher@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;

         }