/* 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 ***}

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.}

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.

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:

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 ofwriting 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.

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:

@CombusterThat 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.

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.

_________________"Certainly avoid yourself. He is a newbie and might not realize it. You'll hate his code deeply a few years down the road." - Sortie[ My OS ] [ VDisk/SFS ]

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.

Who is online

Users browsing this forum: Bing [Bot], Google [Bot] and 12 guests

You cannot post new topics in this forumYou cannot reply to topics in this forumYou cannot edit your posts in this forumYou cannot delete your posts in this forumYou cannot post attachments in this forum