GCC stack protector - false positive?

[Synon]

I'm using gcc's stack protector but it's telling me there's a stack corruption too early on in the kernel for it to be real.

Note: I think this is a mistake in the article.

The article's code:

Code: Select all

void * __stack_chk_guard = NULL;
 
void __stack_chk_guard_setup()
{
    unsigned char * p;
    p = (unsigned char *)&__stack_chk_guard; // *** Notice that this takes the address of __stack_chk_guard  ***
 
    /* If you have the ability to generate random numbers in your kernel then use them,
       otherwise for 32-bit code: */
    *p =  0x00000aff; // *** p is &__stack_chk_guard so *p writes to __stack_chk_guard rather than *__stack_chk_guard ***
}
 
void __attribute__((noreturn)) __stack_chk_fail()
{ 
    /* put your panic function or similar in here */
    unsigned char * vid = (unsigned char *)0xB8000;
    vid[1] = 7;
    for(;;)
    vid[0]++;
}

When I removed the address-of operator from (my version of) the above code, it no longer generated a kernel panic.

Here is the code that I'm using:

Code: Select all

void* __stack_chk_guard = NULL;
 
void __stack_chk_guard_setup(void)
{
    (*(uint32_t*)__stack_chk_guard) = 0x00000AFF; // Notice that this sets the value of __stack_chk_guard.
    // Also notice that it's uint32_t, not unsigned char (as in the article) which causes overflow.
}
 
void __noreturn __stack_chk_fail()
{ 
    panic("stack smashing attempt detected.\n");
}

I can't correct the article myself because I'm not in the appropriate group (or something), but someone should. The address-of operator is a subtle mistake but the fact that it tries to set an unsigned char (8 bits) to a 32-bit value is glaring. Unfortunately I only noticed the obvious one until now.

[max]

The code in the article is correct. There must be something wrong in your system.

In fact its correct to write the value to the symbol "__stack_chk_guard" itself. This is done by referencing it and then writing the value there.

What you are doing is: You cast the "void*" to a "uint32_t*", then you dereference it and write the value there; thus you're writing the value to the address 0, not to the address of "__stack_chk_guard".

The purpose of this is that when a vulnerable function is called, the canary is taken from the global variable "__stack_chk_guard". GCC doesn't care for the type of this variable; it assumes that at exactly this location there is a value of the size of a pointer on your architecture, and because we are using "void*" it is correct for all architectures. Also we are only writing an "unsigned char" to it because it is initially set to 0 and like this it works even on a 8bit machine.



EDIT: by the way...

The address-of operator is a subtle mistake but the fact that it tries to set an unsigned char (8 bits) to a 32-bit value is glaring.

A void* is not necessarily 4 bytes. On a 64 bit system for example its 8 bytes. It always has the size of a pointer on your respective architecture.

There's absolutely nothing wrong with writing 1 byte (8 bits) to a variable that has a size of 4 bytes (32 bits). For example, if you work on an x86 (uses little endian), only the lowest-order byte of the value is overwritten. A little example:

Code: Select all

uint32_t value = 0xDEADBEEF;
uint8_t* p = (uint8_t*) &value;
p[2] = 0xED;
p[3] = 0xFE;
// value is now 0xFEEDBEEF

[cyr1x]

Did you disable the stack protector for this particular translation unit?
If not you may run into trouble as the function epilogue code will check against the old value.

EDIT:
After revisiting the wiki article and its history I found some changes where made which are pretty unfortunate.
The time I created this article, I actually used the array access method to initialize the stack guard to get around the issue of
writing an int to an unsigned char (which in its new version IS! a bug). According to the C standard you MUST use a pointer to char, every other type is undefined behaviour if you access memory of a different type.
Second the sizeof-operator which I used back then should allow for portability between 32bit and 64bit which was also removed, this
actually isn't that bad as you should use a random number anyway, but if not I would keep it.

[Synon]

I've created a(n extremely) stripped-down version of my kernel to demonstrate the error more clearly. Here is a tarball containing the source and two pre-made ISOs that demonstrate the difference where address_operator.iso (compiled with the address-of operator) shows a red screen, indicating that a (false) stack smashing attempt was detected, and no_address_operator.iso (compiled without) shows a blue one, indicating no such detection. If you have grub-mkrescue and i686-none-elf-gcc you can compile and execute the code for yourself (if you have a different i686 cross-compiler, change the FORMAT variable in the Makefile accordingly).

I don't see what could be causing the error -- it's literally 100 LOC and it all looks right to me. There are no compiler warnings except for the multiboot2 header checksum in start.asm, which overflows - but that's how you're supposed to compute the checksum according to the spec; if I change it, GRUB won't load the kernel.

In this code, there is a special Makefile rule that disables the stack protector for stack_guard.c:

Code: Select all

src/stack_guard.o: CFLAGS := $(filter-out -fstack-protector-all,$(CFLAGS)) -fno-stack-protector

and the false positive persists.

I'm confused.

[Combuster]

Be careful when you use your tools. changing the canary knee-deep in the stack is deadly:

Code: Select all

__stack_chk_guard = 0;

void boot (...)
{
     //prologue
     stack_canary = __stack_chk_guard;

     //__stack_chk_guard_setup();
     __stack_chk_guard = 0xaff;  
  
     // epilogue
     if (stack_canary != __stack_chk_guard)
     {
         // always executed
         panic();
     }
}

I cross-referenced with my own stack protector and stack_chk_guard_setup is never referenced, instead the canary value is initialized at compile time:

Code: Select all

SECTION .data
__stack_chk_guard DQ 0x6A1D00CD51AC00CD

[Synon]

@Combuster
That makes sense, but I was hoping to use a random canary value when my kernel gets to the point that it can generate random numbers. With a compile-time value, I won't be able to do that.

Also, originally, __stack_chk_guard_setup was the first thing called after _start creates the stack, but it was tripping __stack_chk_fail before the console was initialised so I couldn't see the panic screen. Ofc, I could modify console_init to only initialise the console once and then have __stack_chk_fail call console_init to make sure the console is initialised, but the point is that the stack check was failing even when the only function that had run was __stack_chk_guard_setup! Then again, that was before I knew to disable the stack protector for stack_guard.c (which seems like an obvious thing to do, but I had assumed gcc would consider that __stack_chk_guard_setup would change the stack guard's value), so perhaps with the stack protector disabled for that file, and the protector setup being called before any other C code, it will work. I'll check and get back to you.

Got it! What I had to do is call __stack_chk_guard_setup before the other C code and disable the stack protector for stack_guard.c.

[max]

[...] writing an int to an unsigned char (which in its new version IS! a bug).

The line "*p = 0x00000aff" is not a bug, not even UB, because 0xAFF doesn't overflow and is implicitly converted, but it in fact is a little unfortunate.

According to the C standard you MUST use a pointer to char, every other type is undefined behaviour if you access memory of a different type.

Yes it's not defined by the C standard and therefore, because it's dependent of your architecture, but its not wrong.

[cyr1x]

The line "*p = 0x00000aff" is not a bug, ...

How does 0xAFF fit into one byte?

Code: Select all

void * test = NULL;

int main()
{
	unsigned char * p = (unsigned char*)&test;
	*p = 0x00000AFF;

	printf("%X", test);	

	return 0;
}

Trying this the compiler even yells at me with a warning. Execution even prints FF not AFF.

[Combuster]

Yup, that was certainly an error. I went there and replaced the default canary to something appropriate, and added some lessons learned

[max]

The line "*p = 0x00000aff" is not a bug, ...

How does 0xAFF fit into one byte?

Code: Select all

void * test = NULL;

int main()
{
	unsigned char * p = (unsigned char*)&test;
	*p = 0x00000AFF;

	printf("%X", test);	

	return 0;
}

Trying this the compiler even yells at me with a warning. Execution even prints FF not AFF.

Oh yeah youre right, I must have been a little drunk.

[Synon]

I think the sizeof(...) thing which was originally in the article is best. It's more portable because the only assumption it makes is sizeof(void*) >= 2.

Here's what I changed mine to:

Code: Select all

void* __stack_chk_guard;

void __stack_chk_guard_setup()
{
    unsigned char* p = (unsigned char*)&__stack_chk_guard;
    memset(p, 0, sizeof(__stack_chk_guard));
    p[sizeof(__stack_chk_guard) - 2] = 0x0A;
    p[sizeof(__stack_chk_guard) - 1] = 0xFF;
}

when I have a random number generator, I'll replace 0xFF and 0x0A with random bytes.

I've joined the wiki group so unless someone has an objection, I'll change it in the article.

[Combuster]

There's a reason I changed the canary to start with CD 00. If a string operation reaches it you'll get one garbage character followed by a null terminator, keeping the rest of the stack safe. If executed it results in an exception. And since the most typical written value is zero, having the canary start with a non-zero (or anything statistically rare number) makes it more likely to catch off-by-one writes as well.

[cyr1x]

I think using uintptr_t instead of void* is probably more elegant here, so you can assign an integer value directly and it is always valid for 32bit and 64bit.
The only drawback is that you need uintptr_t or an equivalent type defined.

Code: Select all

uintptr_t __stack_chk_guard;

void setup()
{
    __stack_chk_guard = 0xCD000AFFUL; // no more pointer magic here :)
}