Skip to content
This repository

test failure with current master in Fedora build environment #136

Closed
tchollingsworth opened this Issue · 10 comments

3 participants

T.C. Hollingsworth Ben Noordhuis mschwendt
T.C. Hollingsworth

I'm getting the folowing failure in test-nonstrict with the latest master in the Fedora build environment:

+ ./out/Release/test-nonstrict
sizeof(http_parser) = 28
response scan 1/2        0%  2%  4%  6%  8% 10% 12% 14% 16% 18% 20% 22% 24% 26% 28% 30% 32% 34% 36% 38% 40% 42% 44% 46% 48% 50% 52% 54% 56% 58% 60% 62% 63% 65% 67% 69% 71% 73% 75% 77% 79% 81% 83% 85% 87% 89% 91% 93% 95% 97% 99%100%
response scan 2/2        0%  0%  1%  1%  1%  1%  2%  2%  2%  3%  3%  3%  4%  4%  4%  4%  5%  5%  5%  6%  6%  6%  7%  7%  7%  7%  8%  8%  8%  9%  9%  9%  9% 10% 10% 10% 11% 11% 11% 12% 12% 12% 12% 13% 13% 13% 14% 14% 14% 15% 15% 15% 15% 16% 16% 16% 17% 17% 17% 18% 18% 18% 18% 19% 19% 19% 20% 20% 20% 20% 21% 21% 21% 22% 22% 22% 23% 23% 23% 23% 24% 24% 24% 25% 25% 25% 26% 26% 26% 26% 27% 27% 27% 28% 28% 28% 28% 29% 29% 29% 30% 30% 30% 31% 31% 31% 31% 32% 32% 32% 33% 33% 33% 34% 34% 34% 34% 35% 35% 35% 36% 36% 36% 37% 37% 37% 37% 38% 38% 38% 39% 39% 39% 39% 40% 40% 40% 41% 41% 41% 42% 42% 42% 42% 43% 43% 43% 44% 44% 44% 45% 45% 45% 45% 46% 46% 46% 47% 47% 47% 47% 48% 48% 48% 49% 49% 49% 50% 50% 50% 50% 51% 51% 51% 52% 52% 52% 53% 53% 53% 53% 54% 54% 54% 55% 55% 55% 55% 56% 56% 56% 57% 57% 57% 58% 58% 58% 58% 59% 59% 59% 60% 60% 60% 61% 61% 61% 61% 62% 62% 62% 63% 63% 63% 64% 64% 64% 64% 65% 65% 65% 66% 66% 66% 66% 67% 67% 67% 68% 68% 68% 69% 69% 69% 69% 70% 70% 70% 71% 71% 71% 72% 72% 72% 72% 73% 73% 73% 74% 74% 74% 74% 75% 75% 75% 76% 76% 76% 77% 77% 77% 77% 78% 78% 78% 79% 79% 79% 80% 80% 80% 80% 81% 81% 81% 82% 82% 82% 82% 83% 83% 83% 84% 84% 84% 85% 85% 85% 85% 86% 86% 86% 87% 87% 87% 88% 88% 88% 88% 89% 89% 89% 90% 90% 90% 91% 91% 91% 91% 92% 92% 92% 93% 93% 93% 93% 94% 94% 94% 95% 95% 95% 96% 96% 96% 96% 97% 97% 97% 98% 98% 98% 99% 99% 99% 99%100%*** buffer overflow detected ***: ./out/Release/test-nonstrict terminated
======= Backtrace: =========
/lib/libc.so.6(__fortify_fail+0x45)[0x6e2925]
/lib/libc.so.6(+0x10b8fa)[0x6e08fa]
/lib/libc.so.6(+0x10ad29)[0x6dfd29]
./out/Release/test-nonstrict[0x8049bde]
./out/Release/lib.target/libhttp_parser.so.2(http_parser_execute+0x1667)[0x3cc367]
./out/Release/test-nonstrict[0x804a155]
./out/Release/test-nonstrict[0x804b454]
./out/Release/test-nonstrict[0x8049111]
/lib/libc.so.6(__libc_start_main+0xf5)[0x5ee865]
./out/Release/test-nonstrict[0x8049369]
======= Memory map: ========
00261000-0033e000 r-xp 00000000 fd:00 2113672                            /usr/lib/libstdc++.so.6.0.17
0033e000-0033f000 ---p 000dd000 fd:00 2113672                            /usr/lib/libstdc++.so.6.0.17
0033f000-00343000 r--p 000dd000 fd:00 2113672                            /usr/lib/libstdc++.so.6.0.17
00343000-00344000 rw-p 000e1000 fd:00 2113672                            /usr/lib/libstdc++.so.6.0.17
00344000-0034b000 rw-p 00000000 00:00 0 
003ca000-003ce000 r-xp 00000000 fd:00 2633579                            /builddir/build/BUILD/joyent-http-parser-245f6f0/out/Release/lib.target/libhttp_parser.so.2
003ce000-003cf000 r--p 00003000 fd:00 2633579                            /builddir/build/BUILD/joyent-http-parser-245f6f0/out/Release/lib.target/libhttp_parser.so.2
003cf000-003d0000 rw-p 00004000 fd:00 2633579                            /builddir/build/BUILD/joyent-http-parser-245f6f0/out/Release/lib.target/libhttp_parser.so.2
0041b000-00437000 r-xp 00000000 fd:00 2113200                            /usr/lib/libgcc_s-4.7.2-20121109.so.1
00437000-00438000 r--p 0001b000 fd:00 2113200                            /usr/lib/libgcc_s-4.7.2-20121109.so.1
00438000-00439000 rw-p 0001c000 fd:00 2113200                            /usr/lib/libgcc_s-4.7.2-20121109.so.1
0045b000-0045c000 r-xp 00000000 00:00 0                                  [vdso]
00595000-005d3000 r-xp 00000000 fd:00 2113323                            /usr/lib/libm-2.16.so
005d3000-005d4000 r--p 0003d000 fd:00 2113323                            /usr/lib/libm-2.16.so
005d4000-005d5000 rw-p 0003e000 fd:00 2113323                            /usr/lib/libm-2.16.so
005d5000-00784000 r-xp 00000000 fd:00 2113315                            /usr/lib/libc-2.16.so
00784000-00786000 r--p 001af000 fd:00 2113315                            /usr/lib/libc-2.16.so
00786000-00787000 rw-p 001b1000 fd:00 2113315                            /usr/lib/libc-2.16.so
00787000-0078a000 rw-p 00000000 00:00 0 
0099f000-009be000 r-xp 00000000 fd:00 2113308                            /usr/lib/ld-2.16.so
009be000-009bf000 r--p 0001e000 fd:00 2113308                            /usr/lib/ld-2.16.so
009bf000-009c0000 rw-p 0001f000 fd:00 2113308                            /usr/lib/ld-2.16.so
08048000-08065000 r-xp 00000000 fd:00 2633581                            /builddir/build/BUILD/joyent-http-parser-245f6f0/out/Release/test-nonstrict
08065000-0813b000 r--p 0001d000 fd:00 2633581                            /builddir/build/BUILD/joyent-http-parser-245f6f0/out/Release/test-nonstrict
0813b000-0813c000 rw-p 000f3000 fd:00 2633581                            /builddir/build/BUILD/joyent-http-parser-245f6f0/out/Release/test-nonstrict
0813c000-0814f000 rw-p 00000000 00:00 0 
08928000-08949000 rw-p 00000000 00:00 0                                  [heap]
f77dc000-f77df000 rw-p 00000000 00:00 0 
f77e1000-f77e4000 rw-p 00000000 00:00 0 
ffa1b000-ffa72000 rw-p 00000000 00:00 0                                  [stack]
100%
responses okay

Judging by the backtrace I'd guess this has something to do with the Fedora buildsystem adding -D_FORTIFY_SOURCE=2 to CFLAGS. I'm not sure whether this can be fixed on your end or we'll have to remove that from CFLAGS. (The full build output from the buildsystem is here if that's helpful.)

Ben Noordhuis
Collaborator

Can you post the exact steps you used to compile http-parser? Please start with a git clean -dfx.

mschwendt

Here's a detailed backtrace that shows how it overflows a buffer in header_value_cb():

#0  0x00000030bcc35935 in raise () from /lib64/libc.so.6
No symbol table info available.
#1  0x00000030bcc370e8 in abort () from /lib64/libc.so.6
No symbol table info available.
#2  0x00000030bcc74e8b in __libc_message () from /lib64/libc.so.6
No symbol table info available.
#3  0x00000030bcd094d7 in __fortify_fail () from /lib64/libc.so.6
No symbol table info available.
#4  0x00000030bcd07690 in __chk_fail () from /lib64/libc.so.6
No symbol table info available.
#5  0x00000030bcd06823 in __strncat_chk () from /lib64/libc.so.6
No symbol table info available.
#6  0x0000000000401d9d in strncat (__len=, 
    __src=, __dest=)
    at /usr/include/bits/string3.h:152
No locals.
#7  header_value_cb (p=, buf=, 
    len=)
    at ../../../../../personal/tmp/rpm/BUILD/joyent-http-parser-245f6f0/test.c:1446
        m = 0x6f29e0
#8  0x00007ffff7dfa8dc in http_parser_execute (parser=0x706010, 
    settings=settings@entry=0x6f2840, 
    data=data@entry=0x404b50 "GET / HTTP/1.1\r\nX-SSL-Bullshit:   -----BEGIN CERTIFICATE-----\r\n\tMIIFbTCCBFWgAwIBAgICH4cwDQYJKoZIhvcNAQEFBQAwcDELMAkGA1UEBhMCVUsx\r\n\tETAPBgNVBAoTCGVTY2llbmNlMRIwEAYDVQQLEwlBdXRob3JpdHkxCzAJBgNVBAMT\r\n\tAk"..., len=2044)
    at ../../../../../personal/tmp/rpm/BUILD/joyent-http-parser-245f6f0/http_parser.c:1425
        c = 
        ch = 
        unhex_val = 
        p = 0x404da5 "\r\n\tgW7cMA/s/XKgL1GEC7rQw8lIZT8RApukCGqOVHSi/F1SiFlPDxuDfmdiNzL31+sL\r\n\t0iwHDdNkGjy5pyBSB8Y79dsSJtCW/iaLB0/n8Sj7HgvvZJ7x0fr+RQjYOUUfrePP\r\n\tu2MSpFyf+9BbC/aXgaZuiCvSR+8Snv3xApQY+fULK/xY8h8Ua51iXoQ5jrgu2Sq"...
        header_field_mark = 0x0
        header_value_mark = 0x404d65 "W51rEyFYiIeZGx/BVzwXbeBoNUK41OK65sxGuflMo5gLflbwJtHBRIEKAfVVp3YR\r\n\tgW7cMA/s/XKgL1GEC7rQw8lIZT8RApukCGqOVHSi/F1SiFlPDxuDfmdiNzL31+sL\r\n\t0iwHDdNkGjy5pyBSB8Y79dsSJtCW/iaLB0/n8Sj7HgvvZJ7x0fr+RQjYOUUfrePP\r\n"...
        url_mark = 0x0
        body_mark = 
#9  0x0000000000402227 in parse (
    buf=buf@entry=0x404b50 "GET / HTTP/1.1\r\nX-SSL-Bullshit:   -----BEGIN CERTIFICATE-----\r\n\tMIIFbTCCBFWgAwIBAgICH4cwDQYJKoZIhvcNAQEFBQAwcDELMAkGA1UEBhMCVUsx\r\n\tETAPBgNVBAoTCGVTY2llbmNlMRIwEAYDVQQLEwlBdXRob3JpdHkxCzAJBgNVBAMT\r\n\tAk"..., len=)
    at ../../../../../personal/tmp/rpm/BUILD/joyent-http-parser-245f6f0/test.c:1737
No locals.
#10 0x00000000004033b2 in test_simple (
    buf=buf@entry=0x404b50 "GET / HTTP/1.1\r\nX-SSL-Bullshit:   -----BEGIN CERTIFICATE-----\r\n\tMIIFbTCCBFWgAwIBAgICH4cwDQYJKoZIhvcNAQEFBQAwcDELMAkGA1UEBhMCVUsx\r\n\tETAPBgNVBAoTCGVTY2llbmNlMRIwEAYDVQQLEwlBdXRob3JpdHkxCzAJBgNVBAMT\r\n\tAk"..., err_expected=err_expected@entry=HPE_OK)
    at ../../../../../personal/tmp/rpm/BUILD/joyent-http-parser-245f6f0/test.c:2659
        err = 
#11 0x0000000000401450 in main ()
    at ../../../../../personal/tmp/rpm/BUILD/joyent-http-parser-245f6f0/test.c:3241
        i = 20
        j = 
        k = 
        request_count = 34
        response_count = 20
        all_methods = {0x407cfb "DELETE", 0x407d02 "GET", 0x407d06 "HEAD", 
          0x407d0b "POST", 0x407d10 "PUT", 0x407d14 "OPTIONS", 
          0x407d1c "TRACE", 0x407d22 "COPY", 0x407d47 "LOCK", 
          0x407d27 "MKCOL", 0x407d2d "MOVE", 0x407d32 "PROPFIND", 
          0x407d3b "PROPPATCH", 0x407d45 "UNLOCK", 0x407d4c "REPORT", 
          0x407d53 "MKACTIVITY", 0x407d5e "CHECKOUT", 0x407d67 "MERGE", 
          0x407d6d "M-SEARCH", 0x407d76 "NOTIFY", 0x407d7f "SUBSCRIBE", 
          0x407d7d "UNSUBSCRIBE", 0x407d3f "PATCH", 0x0}
        this_method = 
        bad_methods = {0x407d89 "C******", 0x407d91 "M****", 0x0}
        dumbfuck2 = 0x404b50 "GET / HTTP/1.1\r\nX-SSL-Bullshit:   -----BEGIN CERTIFICATE-----\r\n\tMIIFbTCCBFWgAwIBAgICH4cwDQYJKoZIhvcNAQEFBQAwcDELMAkGA1UEBhMCVUsx\r\n\tETAPBgNVBAoTCGVTY2llbmNlMRIwEAYDVQQLEwlBdXRob3JpdHkxCzAJBgNVBAMT\r\n\tAk"...

Ben Noordhuis
Collaborator

@mschwendt Do you have a test case I can try?

mschwendt

The src.rpm that was built can be found here: http://koji.fedoraproject.org/koji/buildinfo?buildID=369508

It includes everything you need, but the %check section to run the test-suite is commented out in the spec file and would need to be uncommented. I simply built the package on Fedora 17 x86_64 to verify that the issue is not specific to the Fedora buildsystem.

T.C. Hollingsworth

@bnoordhuis

export 'CFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 '
gyp -f make --depth=`pwd` http_parser.gyp
make
./out/Debug/test-nonstrict

The RPM does a few more elaborate things like addiitonal arch-specific CFLAGS and it builds a shared library, but the above is sufficient to reproduce this locally.

mschwendt

A deeper albeit quick look in the debugger, in test.c strlen(dumbfuck2) is 2044, headers MAX_ELEMENT_SIZE is only 500, however, and it overflows those 500 by appending chunks of the full 2044 characters via strncat. Even if it's just test.c, this is inherently insecure coding style, if the "n" in strncat is not set appropriately to avoid a buffer overflow. It should always recalculate the remaining "n" relative to the buffer dest ptr.

Ben Noordhuis
Collaborator

Can you guys verify if 32 bits build have the same issue? I can't reproduce it on a 32 bits F18 install. Valgrind doesn't complain either.

mschwendt

Well, I can confirm that CentOS 6 i686 isn't affected (no 32-bit Fedora installation here anymore for quick access). rpmbuild excerpt:

+ cd joyent-http-parser-245f6f0
+ unset DISPLAY
+ export LD_LIBRARY_PATH=./out/Release/lib.target
+ LD_LIBRARY_PATH=./out/Release/lib.target
+ ./out/Release/test-nonstrict
sizeof(http_parser) = 28
response scan 1/2  100%
response scan 2/2  100%
responses okay
request scan 1/4  100%
request scan 2/4  100%
request scan 3/4  100%
request scan 4/4  100%
requests okay
+ ./out/Release/test-strict
sizeof(http_parser) = 28
response scan 1/2  100%
response scan 2/2  100%
responses okay
request scan 1/4  100%
request scan 2/4  100%
request scan 3/4  100%
request scan 4/4  100%
requests okay
+ exit 0

That's unusual, because that could mean you may have managed to make something arch-dependent in the C code, perhaps with unsafe casts or sizeof assumptions. FORTIFY_SOURCE isn't anything like a new feature.

Do you have any comment on what I've pointed out further above? Simply increasing MAX_ELEMENT_SIZE to something larger than strlen(dumbfuck2) makes the tests succeed on F17 x86_64 without causing a buffer overflow.

Could you think of anything to enhance the test suite in any related way?

mschwendt

Inserting some printf lines in header_value_cb(), I see that the 32-bit build also overflows the buffer, but this isn't caught by strncat protection. The diff between 64-bit and 32-bit debug output is as follows. The lines prefixed with "+" are printed only for the 32-bit build:

@@ -1,4 +1,4 @@
-sizeof(http_parser) = 32
+sizeof(http_parser) = 28
 headers = 
 len = 22
 strlen(header) = 22
@@ -16669273,3 +16669273,4698291 @@
 strlen(header) = 475
 headers = -----BEGIN CERTIFICATE-----MIIFbTCCBFWgAwIBAgICH4cwDQYJKoZIhvcNAQEFBQ
AwcDELMAkGA1UEBhMCVUsxETAPBgNVBAoTCGVTY2llbmNlMRIwEAYDVQQLEwlBdXRob3JpdHkxCzAJBg
NVBAMTAkNBMS0wKwYJKoZIhvcNAQkBFh5jYS1vcGVyYXRvckBncmlkLXN1cHBvcnQuYWMudWswHhcNMD
YwNzI3MTQxMzI4WhcNMDcwNzI3MTQxMzI4WjBbMQswCQYDVQQGEwJVSzERMA8GA1UEChMIZVNjaWVuY2
UxEzARBgNVBAsTCk1hbmNoZXN0ZXIxCzAJBgNVBAcTmrsogriqMWLAk1DMRcwFQYDVQQDEw5taWNoYWV
sIHBhcmQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANPEQBgl1IaKdSS1TbhF3hEXSl72G9J+WC/1R6
4fAcEF
 len = 64
+strlen(header) = 539
+headers = -----BEGIN CERTIFICATE-----MIIFbTCCBFWgAwIBAgICH4cwDQYJKoZIhvcNAQEFBQ
AwcDELMAkGA1UEBhMCVUsxETAPBgNVBAoTCGVTY2llbmNlMRIwEAYDVQQLEwlBdXRob3JpdHkxCzAJBg
NVBAMTAkNBMS0wKwYJKoZIhvcNAQkBFh5jYS1vcGVyYXRvckBncmlkLXN1cHBvcnQuYWMudWswHhcNMD
YwNzI3MTQxMzI4WhcNMDcwNzI3MTQxMzI4WjBbMQswCQYDVQQGEwJVSzERMA8GA1UEChMIZVNjaWVuY2
UxEzARBgNVBAsTCk1hbmNoZXN0ZXIxCzAJBgNVBAcTmrsogriqMWLAk1DMRcwFQYDVQQDEw5taWNoYWV
sIHBhcmQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANPEQBgl1IaKdSS1TbhF3hEXSl72G9J+WC/1R6
4fAcEFW51rEyFYiIeZGx/BVzwXbeBoNUK41OK65sxGuflMo5gLflbwJtHBRIEKAfVVp3YR
+len = 64
+strlen(header) = 603
+headers = -----BEGIN CERTIFICATE-----MIIFbTCCBFWgAwIBAgICH4cwDQYJKoZIhvcNAQEFBQ
AwcDELMAkGA1UEBhMCVUsxETAPBgNVBAoTCGVTY2llbmNlMRIwEAYDVQQLEwlBdXRob3JpdHkxCzAJBg
NVBAMTAkNBMS0wKwYJKoZIhvcNAQkBFh5jYS1vcGVyYXRvckBncmlkLXN1cHBvcnQuYWMudWswHhcNMD
YwNzI3MTQxMzI4WhcNMDcwNzI3MTQxMzI4WjBbMQswCQYDVQQGEwJVSzERMA8GA1UEChMIZVNjaWVuY2
UxEzARBgNVBAsTCk1hbmNoZXN0ZXIxCzAJBgNVBAcTmrsogriqMWLAk1DMRcwFQYDVQQDEw5taWNoYWV
sIHBhcmQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANPEQBgl1IaKdSS1TbhF3hEXSl72G9J+WC/1R6
4fAcEFW51rEyFYiIeZGx/BVzwXbeBoNUK41OK65sxGuflMo5gLflbwJtHBRIEKAfVVp3YRgW7cMA/s/X
KgL1GEC7rQw8lIZT8RApukCGqOVHSi/F1SiFlPDxuDfmdiNzL31+sL
+len = 64
+strlen(header) = 667
...
...
and so on for 32-bit, whereas 64-bit has crashed at the top of this diff already.
Ben Noordhuis
Collaborator

Thanks for the report. Fixed in cd01361 and safeguards put in place in 14d42be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.