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

[Theodore A. Roth]

Hi Seth,

On Wed, 30 Oct 2002, Seth LaForge wrote:

:) > 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 agree with you completely.

Of course, suid is _strongly_ discouraged and should not even be
documented. ppdev is the correct way to do it, but some people still use
root or suid.

:)
:) 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.
:)

I think it drops suid after opening the parallel port. I'll have to check
that again and make sure it does it for all cases.

:) > 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.

I figured I'd be proven wrong on that. ;-)

:)
:) 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 don't think you need to use more than MI_LINEBUF_SIZE chars in your
buffer since that is all fgets will read when reading a line from the
srec.

I still think a bounds check and throw is useful so you might know if
you come across a bogus srec file.

Thanks for the input this.

I'll make up a new patch and let you have a look before I commit it.

Ted Roth