Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/*: fixed-size buffers (that might overflow) in 6c, 6a, 8g, 8c, 8a, 5g, 5c, 5a #393

Closed
gopherbot opened this issue Dec 8, 2009 · 17 comments

Comments

@gopherbot
Copy link
Contributor

by x41@freeshell.org:

Exploiting #221 #392 in a different way:
Fresh install of:

Last version of Golang
cb140bac9ab0 release.2009-11-12/release

Tested on Ubuntu 9.04 and Ubuntu 9.10
XXXX@XXXX:~/go_src/src$ uname -a
Linux XXXX 2.6.28-11-generic #42-Ubuntu SMP Fri Apr 17 01:57:59 UTC 2009
i686 GNU/Linux

Attached Valgrind report and GDB report.

Binary: 8c

go_src/src/lib9/main.c

Breakpoint 1, main (argc=0x2, argv=0xbfdd72d4) at
/home/dsdsds/go_src/src/lib9/main.c:35
35      p9main(argc, argv);
gdb> list
30  extern void p9main(int, char**);
31  
32  int
33  main(int argc, char **argv)
34  {
35      p9main(argc, argv);
#######################################
              HERE
36      exits("main");
37      return 99;
              HERE
#######################################
38  }

Run it
r `python -c 'print "\x41"*399 + "\x42"'`
Just.. 399 + 1

Attachments:

  1. debuginfo2.tar.gz (2240 bytes)
@gopherbot
Copy link
Contributor Author

Comment 1 by x41@freeshell.org:

/usr/include/bits/string3.h:106: warning: 
call to __builtin___strcpy_chk will always overflow destination buffer
And this i intentionally not allowed with -D_FORTIFY_SOURCE=2, which doesn't allow
crossing field boundaries for str*/stp* etc. functions (and still allows that
for mem* etc.).
If we use -00 the problem is resolved, but if we really need to use -02 or -03
we have to edit Make.conf and modify like this:
 
CFLAGS=-ggdb -I$(GOROOT)/include -O2 -fno-inline -D_FORTIFY_SOURCE=1
The difference between -D_FORTIFY_SOURCE=1 and -D_FORTIFY_SOURCE=2
is e.g. for
struct S { struct T { char buf[5]; int x; } t; char buf[20]; } var;
With -D_FORTIFY_SOURCE=1,
strcpy (&var.t.buf[1], "abcdefg");
is not considered an overflow (object is whole VAR), while
with -D_FORTIFY_SOURCE=2
strcpy (&var.t.buf[1], "abcdefg");
will be considered a buffer overflow.
==================================================
 NOTE: In Ubuntu 8.10 and later versions, -D_FORTIFY_SOURCE=2
 is set by default, and is activated when -O is set to 2 or higher.
 This enables additional compile-time and run-time checks for several
 libc functions.  To disable, specify either -U_FORTIFY_SOURCE or
 -D_FORTIFY_SOURCE=0.
==================================================
It thus OK to keep the bug as RESOLVED

@rsc
Copy link
Contributor

rsc commented Dec 10, 2009

Comment 2:

Fixed by 185828b79e7b

Owner changed to r...@golang.org.

Status changed to Fixed.

@rsc
Copy link
Contributor

rsc commented Dec 10, 2009

Comment 3:

Actually perhaps this is a different bug, but I cannot reproduce this.
I tried
r `python -c 'print "\x41"*100000 + "\x42"'`
which, if there were a buffer overflow, seems like it would cause
a crash.  No crash.  What am I doing wrong?

Status changed to WaitingForReply.

@rsc
Copy link
Contributor

rsc commented Dec 10, 2009

Comment 4:

Issue #392 has been merged into this issue.

@gopherbot
Copy link
Contributor Author

Comment 5 by x41@freeshell.org:

Did you use Ubuntu for testing it?
Like I wrote:
 NOTE: In Ubuntu 8.10 and later versions, -D_FORTIFY_SOURCE=2
 is set by default, and is activated when -O is set to 2 or higher.
 This enables additional compile-time and run-time checks for several
 libc functions.  To disable, specify either -U_FORTIFY_SOURCE or
 -D_FORTIFY_SOURCE=0.

@rsc
Copy link
Contributor

rsc commented Dec 16, 2009

Comment 6:

Note to anyone interested in helping:
There are a ton of these fixed-size buffers in 8c, 8g, etc.
We've gotten rid of a few of the most obvious ones,
but some still remain.  If someone wants to correct these,
feel free, but *do so with the minimum number of changes*.
It won't help to get a code review that rewrites entire
sections of code gratuitously.  Specific guidelines follow.
For most cases, it should suffice to use strecpy or utfecpy.
For things that accumulate large strings, use smprint and free.
For sequences of strcat, use fmtstrinit, fmtprint, fmtstrflush, and free.
Grep for these names to find examples of their usage in the code.
See http://golang.org/doc/contribute.html for instructions
on how to set up Mercurial for the code review process.

Status changed to HelpWanted.

@rsc
Copy link
Contributor

rsc commented Dec 16, 2009

Comment 7:

Issue #409 has been merged into this issue.

@gopherbot
Copy link
Contributor Author

Comment 8 by isaacsonmara:

RSC, I will try to do my best to fix this issue!

@rsc
Copy link
Contributor

rsc commented Sep 11, 2010

Comment 9:

we cleaned 6g

@gopherbot
Copy link
Contributor Author

Comment 10 by jeff.allen:

I will work on 8*, starting with 8a (yes, it's the easiest, but I'm a wimp, sue me).

@rsc
Copy link
Contributor

rsc commented Oct 6, 2011

Comment 11:

Labels changed: added priority-low.

@rsc
Copy link
Contributor

rsc commented Dec 9, 2011

Comment 12:

Labels changed: added priority-someday.

@remyoudompheng
Copy link
Contributor

Comment 14:

A few AddressSanitizer errors:
pkg/runtime
=================================================================
==11765== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60040000c214 at pc
0x402549 bp 0x7fff602fe950 sp 0x7fff602fe948
READ of size 1 at 0x60040000c214 thread T0
    #0 0x402548 (/opt/remy/go/pkg/tool/linux_amd64/6a+0x402548)
    #1 0x402ddf (/opt/remy/go/pkg/tool/linux_amd64/6a+0x402ddf)
    #2 0x4053bd (/opt/remy/go/pkg/tool/linux_amd64/6a+0x4053bd)
    #3 0x41cd88 (/opt/remy/go/pkg/tool/linux_amd64/6a+0x41cd88)
    #4 0x7fc044ac4a14 (/usr/lib/libc-2.17.so+0x21a14)
    #5 0x401748 (/opt/remy/go/pkg/tool/linux_amd64/6a+0x401748)
0x60040000c214 is located 0 bytes to the right of 4-byte region
[0x60040000c210,0x60040000c214)
allocated by thread T0 here:
    #0 0x7fc0451628fa (/opt/gccgo/lib64/libasan.so.0.0.0+0x158fa)
    #1 0x401980 (/opt/remy/go/pkg/tool/linux_amd64/6a+0x401980)
Shadow bytes around the buggy address:
  0x0c00ffff97f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c00ffff9800: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c00ffff9810: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c00ffff9820: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c00ffff9830: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 03 fa
=>0x0c00ffff9840: fa fa[04]fa fa fa 04 fa fa fa 04 fa fa fa 04 fa
  0x0c00ffff9850: fa fa 04 fa fa fa 04 fa fa fa 03 fa fa fa 03 fa
  0x0c00ffff9860: fa fa 04 fa fa fa 04 fa fa fa 04 fa fa fa 04 fa
  0x0c00ffff9870: fa fa 04 fa fa fa 04 fa fa fa 04 fa fa fa 05 fa
  0x0c00ffff9880: fa fa 03 fa fa fa 03 fa fa fa 04 fa fa fa 04 fa
  0x0c00ffff9890: fa fa 04 fa fa fa 03 fa fa fa 04 fa fa fa 06 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==11765== ABORTING
go tool dist: FAILED: /opt/remy/go/pkg/tool/linux_amd64/6a -I $WORK -I
/opt/remy/go/pkg/linux_amd64 -D GOOS_linux -D GOARCH_amd64 -o $WORK/asm_amd64.o
/opt/remy/go/src/pkg/runtime/asm_amd64.s
% addr2line -e /opt/remy/go/pkg/tool/linux_amd64/6a 0x402549
/opt/remy/go/src/cmd/6a/../cc/lexbody:273
=================================================================
==6226== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fda2ea28803 at pc
0x436a16 bp 0x7fffbe256590 sp 0x7fffbe256588
READ of size 1 at 0x7fda2ea28803 thread T0
    #0 0x436a15 (/opt/remy/go/pkg/tool/linux_amd64/6l+0x436a15)
    #1 0x44ce46 (/opt/remy/go/pkg/tool/linux_amd64/6l+0x44ce46)
    #2 0x434893 (/opt/remy/go/pkg/tool/linux_amd64/6l+0x434893)
    #3 0x434dc6 (/opt/remy/go/pkg/tool/linux_amd64/6l+0x434dc6)
    #4 0x438b61 (/opt/remy/go/pkg/tool/linux_amd64/6l+0x438b61)
    #5 0x44c34e (/opt/remy/go/pkg/tool/linux_amd64/6l+0x44c34e)
    #6 0x46c178 (/opt/remy/go/pkg/tool/linux_amd64/6l+0x46c178)
    #7 0x7fda2ee6aa14 (/usr/lib/libc-2.17.so+0x21a14)
    #8 0x401ba8 (/opt/remy/go/pkg/tool/linux_amd64/6l+0x401ba8)
0x7fda2ea28803 is located 3 bytes to the right of 10485760-byte region
[0x7fda2e028800,0x7fda2ea28800)
allocated by thread T0 here:
    #0 0x7fda2f5088fa (/opt/gccgo/lib64/libasan.so.0.0.0+0x158fa)
    #1 0x436329 (/opt/remy/go/pkg/tool/linux_amd64/6l+0x436329)
Shadow bytes around the buggy address:
  0x0ffbc5d3d0b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffbc5d3d0c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffbc5d3d0d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffbc5d3d0e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffbc5d3d0f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ffbc5d3d100:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ffbc5d3d110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ffbc5d3d120: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ffbc5d3d130: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ffbc5d3d140: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ffbc5d3d150: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==6226== ABORTING
FAIL    compress/zlib [build failed]
% addr2line -e /opt/remy/go/pkg/tool/linux_amd64/6l 436a16
/opt/remy/go/src/cmd/6l/../ld/lib.c:865

@dvyukov
Copy link
Member

dvyukov commented Mar 24, 2013

Comment 15:

Regarding ASan see:
https://groups.google.com/forum/?fromgroups=#!topic/golang-dev/RPvmDFLb6mE
I suspect this is the following "benign" memcmp overflow:
READ of size 5 at 0x60200000c214 thread T0
    #0 0x41719c in __interceptor_memcmp _asan_rtl_
    #1 0x42cb78 in lookup
src/cmd/6a/../cc/lexbody:266
    #2 0x429312 in slookup
src/cmd/6a/../cc/lexbody:220
    #3 0x428928 in p9main
src/cmd/6a/lex.c:71
    #4 0x446615 in main
src/lib9/main.c:55
0x60200000c214 is located 0 bytes to the right of 4-byte region
[0x60200000c210,0x60200000c214)
allocated by thread T0 here:
    #0 0x418378 in __interceptor_malloc _asan_rtl_
    #1 0x42cc29 in alloc
src/cmd/6a/../cc/lexbody:97
    #2 0x429312 in slookup
src/cmd/6a/../cc/lexbody:220
    #3 0x428928 in p9main
src/cmd/6a/lex.c:71
    #4 0x446615 in main
src/lib9/main.c:55

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 16:

Labels changed: added repo-main.

@rsc
Copy link
Contributor

rsc commented Mar 3, 2014

Comment 17:

Adding Release=None to all Priority=Someday bugs.

Labels changed: added release-none.

@rsc
Copy link
Contributor

rsc commented Jun 8, 2015

These are now written in Go, not C. A buffer overflow will be handled gracefully (as an index bounds check failure) and is no longer cause for concern. And most of the fixed-size buffers are gone.

@rsc rsc closed this as completed Jun 8, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
@rsc rsc removed their assignment Jun 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants