How to prevent password masking from bufferoverflow

Question

#include <conio.h>
#include <windows.h>
#include <stdio.h>

int main ()
{
char input[255];
int i = 0;
for(;;i++) /* Infinite loop, exited when RETURN is pressed */
{
    char temp;
    temp = getch (); /* Get the current character of the password */
    if (GetAsyncKeyState (VK_RETURN)) /* If the user has pressed return */
    {
        input[i]='\0';
        break;
    }
    input[i] = temp;
    printf("*"); /* Print a star */
}
//printf("%s",input);
if(strcmp(input,"kamal")==0)
{
                     printf("ACCEPTED");
                     }
                     else
                     printf("not");
_getch();
return EXIT_SUCCESS; /* Program was executed successfully */
}

This is my Code. How can i prevent Buffer overflow , if i input password more , then my program crashes. is there anyway i can overcome the problem of this ?

*"if i input password more , then my program crashes"* This is not English... Could you try to clarify the reason of the crashes, as it is quite unclear? Otherwise it would hard to help you any further — Veger, Feb 25 '13 at 12:46
if i more character than 255 it crashes , and it makes my program vulnerable. — Kamal Kafkaesque, Feb 25 '13 at 13:01

Rajesh · Accepted Answer · 2013-02-25T13:12:14.140

Local variable char input[255] stored in the stack. There is no boundary check for array in C. Problem is that when we add more than 255 characters the value of the other variable stored in the stack may get changed. This may cause crash.

one solution is you read the characters and assign only to input array if range (i) less than 255.

#include <conio.h>
#include <windows.h>
#include <stdio.h>

int main ()
{
    char input[255];
    int i = 0;
    int flag = 0;
   for(;;i++) /* Infinite loop, exited when RETURN is pressed */
   {
       char temp;
       temp = getch (); /* Get the current character of the password */
       if (GetAsyncKeyState (VK_RETURN)) /* If the user has pressed return */
       {
           input[i]='\0';
           break;
       }
       if ( i< 255)
       {
            input[i] = temp;
       }
       else     
       {
           flag = 1;
       }

       printf("*"); /* Print a star */
 }
//printf("%s",input);
if(strcmp(input,"kamal")==0 && flag == 0)
{
      printf("ACCEPTED");
 }
 else
       printf("not");
 getch();
 return EXIT_SUCCESS; /* Program was executed successfully */
}

Another solution is to dynamically allocate (realloc()) size of the input array.

there is one problem that when i press the backspace key it treats it as key , it doesn't delete the last caharacter — Kamal Kafkaesque, Feb 26 '13 at 12:00
@KamalKafkaesque when you press backspace whether the character is deleted from console? or any junk character ? — Rajesh, Feb 27 '13 at 04:59
nothing is deleted ! it is taking it as keystroke and storing it in the array — Kamal Kafkaesque, Feb 27 '13 at 12:28
compare the char with '\b' or ascii 8. if so delete last char from array (decrement i by one). — Rajesh, Feb 27 '13 at 13:02

score 1 · Answer 2 · answered Feb 25 '13 at 12:46

1

Always check the bounds. Always check the value of i against the length of the buffer.

answered Feb 25 '13 at 12:46

Mike Sherrill 'Cat Recall'

91,602
17
122
185

can u be more specific please. – Kamal Kafkaesque Feb 25 '13 at 13:02

autistic · Answer 3 · 2013-02-25T13:56:49.400

1

In some cases, it's acceptable to expand the buffer. This is rarely ideal, since unbounded expansion can cause other problems.

In other cases, it's acceptable to truncate input. This might be an option here, but it's not ideal either.

In this case, as you're comparing against a string that doesn't change, you could skip the "store the input in an array" stage and compare the input as it's recieved, byte by byte, to the password. Such code ought to look something like this, though beware as this is untested:

char password[] = "kamal";
size_t position = 0;
char c = getch();
while (password[position] != '\0' || strchr("\r\n", (unsigned char) c) == NULL) {
    if (c != password[position++] || position == sizeof password) {
        // Password mismatch. Discard the rest of the password, then tell the user...
        while (strchr("\r\n", (unsigned char) c) == NULL) {
            c = getch();
        }
        position = 0;
        puts("Invalid password. Please retry.");
    }
    c = getch();
}

... If there is no buffer to overflow, then what are you worried about?

edited Feb 25 '13 at 13:56

answered Feb 25 '13 at 13:16

autistic

1
3
35
80

i am little noob in c so can you please give me the full code of this because i am unable to understand where to put this code. – Kamal Kafkaesque Feb 25 '13 at 13:20
If you need to ask me for the full solution, then the method you are using to learn is obviously not working for you. Have you considered a book? – autistic Feb 25 '13 at 13:24
This site has a list of them: http://www.iso-9899.info/wiki/Main_Page ... and this is also pretty nice: http://c-faq.com/ – autistic Feb 25 '13 at 13:32
this code is giving an error ! error an 2nd last line saying syntax error on getch(); – Kamal Kafkaesque Feb 27 '13 at 12:30
@KamalKafkaesque Does your code give the same error? If so, how do you fix it? – autistic Feb 27 '13 at 15:01
If not, please tell me the error. I hope it isn't on my part, as I have read many books ;) – autistic Feb 27 '13 at 15:13

Paul · Answer 4 · 2013-02-25T16:27:02.710

Try wrapping something like:

if(i<255) {
    ...
}

...around your character collection process.

-- EDIT --

#include <conio.h>
#include <windows.h>
#include <stdio.h>

int main ()
{
char input[255];
int i = 0;
for(;;i++) /* Infinite loop, exited when RETURN is pressed */
{
    if(i < 255) 
    {
        char temp;
        temp = getch (); /* Get the current character of the password */
        if (GetAsyncKeyState (VK_RETURN)) /* If the user has pressed return */
        {
            input[i]='\0';
            break;
        }
        input[i] = temp;
        printf("*"); /* Print a star */
    }
}
//printf("%s",input);
if(strcmp(input,"kamal")==0)
{
                     printf("ACCEPTED");
                     }
                     else
                     printf("not");
_getch();
return EXIT_SUCCESS; /* Program was executed successfully */
}

Did you test that code? What happens if you try to enter more than 255 chars? zzzap. Undefined behaviour. — autistic, Feb 27 '13 at 15:05
I was simply using what was already there as an example of wrapping the central character collection section in an `if` block; so, no - I didn't test the code. I knew that the code wouldn't work as it won't ever be able to exit. — Paul, Feb 27 '13 at 19:36

score 0 · Answer 5 · answered Feb 25 '13 at 13:08

0

As you indicate, you program crashes if the user provides more than 255 characters.

So, you should check the amount of characters that are already provided, if it reaches the maximum (of your buffer), you should stop grabbing more keys (as the password is already wrong, it does not really matter...):

#define BUFFER_MAX 255

// +1 so we can always add the 0-terminator
char input[BUFFER_MAX + 1];
do {
    char temp;
    temp = getch (); /* Get the current character of the password */
    // Check if char does fit in the buffer
    if(i < BUFFER_MAX) {
        // add to buffer
        input[i] = temp;
        i++;
    }
    printf("*"); /* Print a star */
    // Check if the user pressed return
} while(GetAsyncKeyState (VK_RETURN) == false);
input[i]='\0';

Note: I also rearranged your loop, as it is 'ugly' (bad practice and unnecessary in this case) to use infinite loops and breaking from them...

answered Feb 25 '13 at 13:08

Veger

37,240
11
105
116

Hey Veger , Can u provide me the final total code , it would be highly appreciable. – Kamal Kafkaesque Feb 25 '13 at 13:15
Well... just put it in your main function and keep the password check part! (Oh... and place the macro (`#define`) outside the main function...) – Veger Feb 25 '13 at 13:23
compiler is having an error at the line while(GetAsyncKeyState (VK_RETURN) == false); saying false is undefined , define it first – Kamal Kafkaesque Feb 27 '13 at 12:31
`false` is defined as `0`. So either define it (or find a definition) or just replace it with `0`. – Veger Feb 27 '13 at 12:46

How to prevent password masking from bufferoverflow

5 Answers5