OSDev.org https://forum.osdev.org/ |
|
GCC stack protector - false positive? https://forum.osdev.org/viewtopic.php?f=1&t=28571 |
Page 1 of 1 |
Author: | Synon [ Fri Oct 03, 2014 12:45 pm ] |
Post subject: | GCC stack protector - false positive? |
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: 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: 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. |
Author: | max [ Fri Oct 03, 2014 2:36 pm ] |
Post subject: | Re: GCC stack protector - false positive? |
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... Synon wrote: 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: uint32_t value = 0xDEADBEEF;
uint8_t* p = (uint8_t*) &value; p[2] = 0xED; p[3] = 0xFE; // value is now 0xFEEDBEEF |
Author: | cyr1x [ Fri Oct 03, 2014 3:07 pm ] |
Post subject: | Re: GCC stack protector - false positive? |
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. |
Author: | Synon [ Sat Oct 04, 2014 8:24 am ] |
Post subject: | Re: GCC stack protector - false positive? |
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: src/stack_guard.o: CFLAGS := $(filter-out -fstack-protector-all,$(CFLAGS)) -fno-stack-protector and the false positive persists. I'm confused. |
Author: | Combuster [ Sat Oct 04, 2014 8:53 am ] |
Post subject: | Re: GCC stack protector - false positive? |
Be careful when you use your tools. changing the canary knee-deep in the stack is deadly: Code: __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: SECTION .data
__stack_chk_guard DQ 0x6A1D00CD51AC00CD |
Author: | Synon [ Sat Oct 04, 2014 9:44 am ] |
Post subject: | Re: GCC stack protector - false positive? |
@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. |
Author: | max [ Sat Oct 04, 2014 1:05 pm ] |
Post subject: | Re: GCC stack protector - false positive? |
cyr1x wrote: [...] 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.cyr1x wrote: 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.
|
Author: | cyr1x [ Sun Oct 05, 2014 3:01 am ] |
Post subject: | Re: GCC stack protector - false positive? |
max wrote: The line "*p = 0x00000aff" is not a bug, ... How does 0xAFF fit into one byte? Code: 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. |
Author: | Combuster [ Sun Oct 05, 2014 3:27 am ] |
Post subject: | Re: GCC stack protector - false positive? |
Yup, that was certainly an error. I went there and replaced the default canary to something appropriate, and added some lessons learned |
Author: | max [ Sun Oct 05, 2014 4:59 am ] |
Post subject: | Re: GCC stack protector - false positive? |
cyr1x wrote: max wrote: The line "*p = 0x00000aff" is not a bug, ... How does 0xAFF fit into one byte? Code: 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. |
Author: | Synon [ Sun Oct 05, 2014 9:02 am ] |
Post subject: | Re: GCC stack protector - false positive? |
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: 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. |
Author: | Combuster [ Sun Oct 05, 2014 10:24 am ] |
Post subject: | Re: GCC stack protector - false positive? |
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. |
Author: | cyr1x [ Sun Oct 05, 2014 11:04 am ] |
Post subject: | Re: GCC stack protector - false positive? |
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: uintptr_t __stack_chk_guard;
void setup() { __stack_chk_guard = 0xCD000AFFUL; // no more pointer magic here :) } |
Page 1 of 1 | All times are UTC - 6 hours |
Powered by phpBB © 2000, 2002, 2005, 2007 phpBB Group http://www.phpbb.com/ |