OSDev.org

The Place to Start for Operating System Developers
It is currently Tue Sep 26, 2017 2:07 pm

All times are UTC - 6 hours




Post new topic Reply to topic  [ 13 posts ] 
Author Message
 Post subject: GCC stack protector - false positive?
PostPosted: Fri Oct 03, 2014 12:45 pm 
Offline
Member
Member

Joined: Sun Sep 06, 2009 3:54 am
Posts: 169
Location: Brighton, United Kingdom
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.


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Fri Oct 03, 2014 2:36 pm 
Offline
Member
Member
User avatar

Joined: Mon Mar 05, 2012 11:23 am
Posts: 549
Location: Germany
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

_________________
-maxdev
Ghost OS / https://github.com/maxdev1/ghost


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Fri Oct 03, 2014 3:07 pm 
Offline
Member
Member

Joined: Tue Aug 21, 2007 1:41 am
Posts: 206
Location: Germany
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.


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Sat Oct 04, 2014 8:24 am 
Offline
Member
Member

Joined: Sun Sep 06, 2009 3:54 am
Posts: 169
Location: Brighton, United Kingdom
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.


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Sat Oct 04, 2014 8:53 am 
Offline
Member
Member
User avatar

Joined: Wed Oct 18, 2006 3:45 am
Posts: 9228
Location: An der schönen blauen Donau, watching muppets
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

_________________
"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 ]


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Sat Oct 04, 2014 9:44 am 
Offline
Member
Member

Joined: Sun Sep 06, 2009 3:54 am
Posts: 169
Location: Brighton, United Kingdom
@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.


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Sat Oct 04, 2014 1:05 pm 
Offline
Member
Member
User avatar

Joined: Mon Mar 05, 2012 11:23 am
Posts: 549
Location: Germany
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.

_________________
-maxdev
Ghost OS / https://github.com/maxdev1/ghost


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Sun Oct 05, 2014 3:01 am 
Offline
Member
Member

Joined: Tue Aug 21, 2007 1:41 am
Posts: 206
Location: Germany
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.


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Sun Oct 05, 2014 3:27 am 
Offline
Member
Member
User avatar

Joined: Wed Oct 18, 2006 3:45 am
Posts: 9228
Location: An der schönen blauen Donau, watching muppets
Yup, that was certainly an error. I went there and replaced the default canary to something appropriate, and added some lessons learned ;)

_________________
"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 ]


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Sun Oct 05, 2014 4:59 am 
Offline
Member
Member
User avatar

Joined: Mon Mar 05, 2012 11:23 am
Posts: 549
Location: Germany
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.
Oh yeah youre right, I must have been a little drunk. :mrgreen:

_________________
-maxdev
Ghost OS / https://github.com/maxdev1/ghost


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Sun Oct 05, 2014 9:02 am 
Offline
Member
Member

Joined: Sun Sep 06, 2009 3:54 am
Posts: 169
Location: Brighton, United Kingdom
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.


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Sun Oct 05, 2014 10:24 am 
Offline
Member
Member
User avatar

Joined: Wed Oct 18, 2006 3:45 am
Posts: 9228
Location: An der schönen blauen Donau, watching muppets
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 ]


Top
 Profile  
 
 Post subject: Re: GCC stack protector - false positive?
PostPosted: Sun Oct 05, 2014 11:04 am 
Offline
Member
Member

Joined: Tue Aug 21, 2007 1:41 am
Posts: 206
Location: Germany
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 :)
}


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 13 posts ] 

All times are UTC - 6 hours


Who is online

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


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

Search for:
Jump to:  
Powered by phpBB © 2000, 2002, 2005, 2007 phpBB Group