Crash on start of samterm with hardening

[leahneukirchen]

I built sam/samterm with stack protector, PIE and fortify_source, and it segfaults at the Hbindname message now:

0x00005555555c1631 in inmesg (type=<optimized out>, count=<optimized out>)
    at mesg.c:127
127         text[i]->tag = m;
(gdb) bt
#0  0x00005555555c1631 in inmesg (type=<optimized out>, count=<optimized out>)
    at mesg.c:127
#1  0x00005555555c0c27 in rcv () at mesg.c:74
#2  0x00005555555b12ea in main (argc=<optimized out>, argv=<optimized out>)
    at main.c:83

(gdb) p i
$3 = 0
(gdb) p text[i]
$4 = (Text *) 0x5628dbd8
(gdb) p *text[i]
Cannot access memory at address 0x5628dbd8

It seems PIE is at fault here, which shuffles the addresses in the new samterm process(?)... is this code really passing pointers to global variables via a pipe? In a non-PIE build, text[i] is cmd, but not sure where that is defined really.

[leahneukirchen]

Ok, it's not the same address that is sent, I misread that.

[leahneukirchen]

More debugging:

(gdb) p &cmd
$4 = (Text *) 0x5555562ac9b8
(gdb) p text[0]
$2 = (Text *) 0x562ac9b8

So something truncates this pointer.

[leahneukirchen]

outvlong generates a buf with "\270\311_VUU\000",
but invlong indata then is "\000\000\270\311_V" (\000...).

For some reason USE64BITS isn't passed to sam, so sizeof ulong == 4 for sam on 64-bit!

[leahneukirchen]

I think all files that load u.h should load config.h before?

[leahneukirchen]

After fixing the sizeof, it crashes dereferencing nw->user1 which is 0...

Since user1 is only set in flnew, it should be set after... but it isn't?

In flnew, l is correct:

(gdb) p *l
$11 = {bg = 16777215, f = {bg = 0, font = 0x0, b = 0x0, r = {min = {x = 0,
y = 0}, max = {x = 0, y = 0}}, entire = {min = {x = 0, y = 0}, max = {
x = 0, y = 0}}, box = 0x0, p0 = 0, p1 = 0, left = 0, nbox = 0,
nalloc = 0, maxtab = 0, fheight = 0, nchars = 0, nlines = 0, maxlines = 0,
lastlinefull = 0, modified = 0}, origin = 0, p0 = 0, p1 = 0, click = 0,
textfn = 0x555555557c60 , user0 = 1,
user1 = 0x55555576f6f8 , entire = {min = {x = 0, y = 0}, max = {x = 0,
y = 0}}, scroll = {min = {x = 0, y = 0}, max = {x = 0, y = 0}},
visible = None}

After, cmd.l[0] is off?

$12 = {bg = 16777215, f = {bg = 0, font = 0x0, b = 0x0, r = {min = {x = 0,
y = 0}, max = {x = 0, y = 0}}, entire = {min = {x = 0, y = 0}, max = {
x = 0, y = 0}}, box = 0x0, p0 = 0, p1 = 0, left = 0, nbox = 0,
nalloc = 0, maxtab = 0, fheight = 0, nchars = 0, nlines = 0, maxlines = 0,
lastlinefull = 0, modified = 0}, origin = 0, p0 = 0, p1 = 0,
click = 93824992246880, textfn = 0x1, user0 = 1433859832, user1 = 0x0,
entire = {min = {x = 0, y = 0}, max = {x = 0, y = 0}}, scroll = {min = {
x = 0, y = 0}, max = {x = 0, y = 0}}, visible = None}

... gah this is because the two C files don't use the same sizes.

[leahneukirchen]

A clean rebuild where I force ulong to uint64_t seems to work. The setting from config.h is currently broken.

[deadpixi]

Hey @chneukirchen, thank you for debugging this.

What compiler flags are you using? Something like this?

-D_FORTIFY_SOURCE=1 -fPIE -fstack-protector -O

?

[leahneukirchen]

make CC="clang -std=c99 -D_XOPEN_SOURCE=500 -fstack-protector-strong -O1 -D_FORTIFY_SOURCE=2 -fpie -fsanitize=undefined -g" USE64BITS=1 LDFLAGS="-pie"

Without -pie, no true 64-bit addresses seem to be used, so it looked like it worked.

[leahneukirchen]

This is on Linux btw.

[deadpixi]

Hi @chneukirchen

Please try putting a

#include "../config.h"

in include/u.h

That should fix it, I think?

[leahneukirchen]

Yes, that fixes it. I was not sure a mere "../" works, but it looks like it.

BTW, some warnings came up which you really should investigate:

clipline.c:94:5: warning: absolute value function 'abs' given an argument of
      type 'long' but has parameter of type 'int' which may cause truncation of
      value [-Wabsolute-value]
menuhit.c:144:11: warning: using the result of an assignment as a condition
      without parentheses [-Wparentheses]
            item = menu->item? menu->item[nitem] : (*menu->gen)(nitem);
rsam.c:89:13: warning: expression result unused [-Wunused-value]
    while ((nfd, &rfds, NULL, NULL, NULL) >= 0){
menu.c:290:11: warning: '&&' within '||' [-Wlogical-op-parentheses]
        if(!lock && !t->lock || n==Search || n==Look)

[deadpixi]

Hey @chneukirchen

sam compiles without warnings now, and I've believe it also compiles more-or-less correctly with the hardening flags above.

Also, I'm working on replacing the complex buffer management code with a much simpler mechanism that is also agnostic about USE64BITS. When that's ready to be put into master, I'm going to modify all of the outT* functions to always treat "l" as 64-bit, "s" as 16-bit, and so on. With that change, there won't be any further need of the USE64BITS setting.

Anyway, I'm going to close this bug now, since I think it's fixed. If not, please reopen it. Thanks!