[Uisp-dev] Re: [Bug #1551] Buffer overflow causes crash in uisp on some s19 files

[Seth LaForge]

> Date: 2002-Oct-29 21:33             By: troth
> This is particularly nasty since uisp may be run SUID root if the user wishes
>  to use direct parallel port access.

Dear God no!  The uisp source is nowhere close to secure when SUID
root.  Even with this problem fixed, you can easily read or write to
any file on the system as root.  If you really want to make it safe to
run SUID root, it needs some serious architectural changes.  The code
is littered with unsafe string operations.  For example, in the
terminal mode you just have to type more than 32 characters to do a
buffer overflow attack.

I don't think I'd ever trust a suid uisp on a secure system.  What I
would trust was a little suid program which could only be used to
twiddle bits on the parallel port, and uisp using that program for
direct parallel port access.  If you're interested in this approach, I
might be convinced to code it up.

> Could I get a file which causes this behaviour for testing?

Execute:
  echo 'int main() {}' \
        > this_file_has_a_very_long_name_and_may_confuse_uisp.c
  avr-gcc this_file_has_a_very_long_name_and_may_confuse_uisp.c \
        -Wl,--oformat,srec -o \
        this_file_has_a_very_long_name_and_may_confuse_uisp.s19

Now, the s19 file contains:
  
S02B0000746869735F66696C655F6861735F615F766572795F6C6F6E675F6E616D655F616E645F6D61795F6394
  S11300000DC028C027C026C025C024C023C022C0DC
  S10D001021C020C01FC01EC01DC087
  S113001A11241FBE20E0A89521BD20E025BF40E1A0
  S113002AF0E000E1B060206003C0C89531960D92FB
  S113003A0031B207D1F700E1B060206001C01D921F
  S10F004A0031B207E1F703C000C01895B4
  S10D00561FE3D2E0DEBFCDBFFFCFF1
  S9030000FC

You proposed a patch which included:

  --- src/MotIntl.C       13 Jun 2002 10:50:52 -0000      1.3
  +++ src/MotIntl.C       30 Oct 2002 07:19:49 -0000
  @@ -81,8 +81,9 @@
   
   void TMotIntl::UploadMotorola(){
     unsigned char srec_len, buf_len, srec_cc_sum;
  -  char seg_name[32];
  -  char* p;             /* line buffer pointer */
  +  char seg_name[80];    /* format spec says "An S-record will be less than or
  +                           equal to 78 bytes in length." */
  +  char* p;              /* line buffer pointer */
     TAddr addr;
     TAddr total_bytes_uploaded=0;
     TAddr hash_cnt=0;

and pointed at <http://www.amelek.gda.pl/avr/uisp/srecord.htm>.  That
document says that the entire line will be less than 78 bytes of
characters, which means less than 78 / 2 pairs of hex bytes.  However,
binutils clearly violates the format described in your page - it
creates lines with up to 40 data bytes, meaning 80 characters,
resulting in 90 characters with the other fields included.

Following is a patch which I've tested and believe is correct.  I got
rid of the srec_len check - if srec_len is too big, Htoi will run into
the CRLF or NUL byte at the end of the string and give up.  I made the
buffer 256 bytes long - since srec_len is a single byte, it can't
possibly overflow the buffer.

I also got rid of another buffer overflow.

diff -u -r uisp-20020626/src/AvrAtmel.C uisp-20020626.new/src/AvrAtmel.C
--- uisp-20020626/src/AvrAtmel.C        2002-06-24 06:10:43.000000000 -0700
+++ uisp-20020626.new/src/AvrAtmel.C    2002-10-30 09:58:06.000000000 -0800
@@ -560,17 +560,18 @@
   
   /* Retrieve supported codes */
   TByte sup_codes[1] = {'t'};
-  TByte sup_prg_code[32];
+  // This is not used and is a security hole:
+  // TByte sup_prg_code[32];
   Tx(sup_codes, 1);
   TByte buf_code;
   timeval timeout = {1, 0};
-  int i=0;
+  // int i=0;
   if (desired_partname==NULL){
     Info(1, "  Supported Parts:\n\tNo\tAbbreviation\tDescription\n");
   }
   do{
     Rx(&buf_code, 1, &timeout);
-    sup_prg_code[i++] = buf_code;
+    // sup_prg_code[i++] = buf_code;
     if (buf_code==0){break;}    
     if (desired_partname!=NULL){ 
       if (buf_code==desired_avrcode){got_device=true;}
diff -u -r uisp-20020626/src/MotIntl.C uisp-20020626.new/src/MotIntl.C
--- uisp-20020626/src/MotIntl.C 2002-06-13 03:50:52.000000000 -0700
+++ uisp-20020626.new/src/MotIntl.C     2002-10-30 11:17:07.000000000 -0800
@@ -43,11 +43,19 @@
 
 TByte TMotIntl::Htoi(const char* p){
   unsigned char val = 0;
-  if (*p==0){throw Error_Device("Bad file format.");}
-  if (*p>=0 && *p<='9'){val += *p-'0';}else{val += *p-'A'+10;}
+  if (*p>='0' && *p<='9')
+    val += *p-'0';
+  else if (*p>='A' && *p<='F')
+    val += *p-'A'+10;
+  else
+    throw Error_Device("Bad file format.");
   val <<= 4; p++;    
-  if (*p==0){throw Error_Device("Bad file format.");}  
-  if (*p>=0 && *p<='9'){val += *p-'0';}else{val += *p-'A'+10;}
+  if (*p>='0' && *p<='9')
+    val += *p-'0';
+  else if (*p>='A' && *p<='F')
+    val += *p-'A'+10;
+  else
+    throw Error_Device("Bad file format.");
   cc_sum += val;
   return val;
 }
@@ -81,7 +89,7 @@
 
 void TMotIntl::UploadMotorola(){
   unsigned char srec_len, buf_len, srec_cc_sum;
-  char seg_name[32];
+  char seg_name[256];   /* data field length is a byte, so this is safe */
   char* p;             /* line buffer pointer */
   TAddr addr;
   TAddr total_bytes_uploaded=0;