1

The following program:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
 int check_authentication(char *password) 
{
 int auth_flag = 0;
 char password_buffer[16];
 strcpy(password_buffer, password);
 if(strcmp(password_buffer, "unipi") == 0)
    auth_flag = 1;
 if(strcmp(password_buffer, "SSL") == 0)
    auth_flag = 1;
 return auth_flag;
}

int main(int argc, char *argv[]) {
 if(argc < 2) {
    printf("Usage: %s <password>\n", argv[0]);
    exit(0);
 }
 if(check_authentication(argv[1])!=0)
 {
    printf("\n-=-=-=-=-=-=-=-=-=-=-=-=-=-\n");
    printf(" Access Granted.\n");
    printf("-=-=-=-=-=-=-=-=-=-=-=-=-=-\n");
 }
 else
 {
     printf("\nAccess Denied.\n");
 }
}

At line strcpy(password_buffer, password);it has a buffer overflow vulnerability. If we want to make this program secure without taking out the strcpy , how is it possible?

8
  • 2
    The best way is to replace strcpy with a safe function. What are you looking to accomplish by keeping strcpy? Commented Feb 22, 2016 at 6:50
  • "If we want to make this program secure" Secure against what? Commented Feb 22, 2016 at 14:01
  • 3
    char password_buffer[strlen(password)+1]. Welcome to stack overflow. Commented Feb 22, 2016 at 17:45
  • 1
    Why are you copying password at all? Commented Feb 22, 2016 at 18:33
  • @Micheal This is out of the scope of this question. I just wanted to try to make it secure without removing the strcpy Commented Feb 23, 2016 at 10:40

3 Answers 3

2

Easy. You change the verification that the user input is as expected else where. With strlen so that you can check that the string is shorter than 16 bytes.

if(strlen(argv[1]) > 15)
{
    printf("too long password\n");
    exit(0);
}
Sign up to request clarification or add additional context in comments.

7 Comments

This check should probably be done inside the check_authentication() function.
No you should have it in the main function before the call so to not waste resources.
Waste what resources?
Definitely do it inside the check_authentication() function because A) that's the function which controls the size of the password buffer and B) then you don't have to [forget to] put this check everywhere check_authentication() is called.
Saving the resources to build a stack frame is not a concern for this example. It's of more value to teach where the proper place for parameter validation is than to teach about worrying about the resources used to build a stack frame (which is negligible in nearly all cases other than maybe for the tightest of loops). Except for unbounded recursion, the creation of a stack frame is probably one of the lowest priority things to be concerned with in programming.
|
1

I have found this on Wiki:

To prevent the buffer overflow from happening in this example, the call to strcpy could be replaced with strncpy, which takes the maximum capacity of A as an additional parameter and ensures that no more than this amount of data is written to A:

strncpy(password_buffer, password, sizeof(A));

Note that the above code is not free from problems either; while a buffer overrun has been prevented this time, the strncpy library function does not null-terminate the destination buffer if the source string's length is greater than or equal to the size of the buffer (the third argument passed to the function), therefore A is, in this case, not null-terminated and cannot be treated as a valid C-style string.

Comments

1

There's a few ways to deal with this.


First, and perhaps most important, is to avoid fixed memory allocations especially when dealing with input. In your example above instead of copying to a fixed size buffer you can make the buffer the size you need.

char password_copy[strlen(password) + 1];
strcpy(password_copy, password);

But that's not always possible. Maybe you've got already allocated memory. Maybe you do want to truncate the input (though 16 is far too small for a password).


One is to not use the C string handling functions at all, they're riddled with flaws and security holes. Instead use a library like Gnome Lib which features the G_String type. G_Strings track their length and allocated size. This takes a little more memory, but it's faster. Finding the length of the string, something that happens a lot, doesn't require iterating through every byte of the string.

G_Strings have their own set of string handling functions which are much handier than the C ones. It also can grow strings as necessary or allocate new strings for you.

/* Allocate memory for the copy and copy the password */
G_String *password_copy = g_string_new(password);

For compatibility with regular string functions password_copy->str returns a normal char *.

This is IMO the best way. You no longer have to remember to check string lengths and allocated sizes and worry about null bytes everywhere you use strings. You will forget. Let the computer do that.


If you must use C standard functions, don't use strncpy because it fails to guarantee the truncated string will be null terminated. Instead use strlcpy. It's like strncpy but it guarantees the copied string will be null terminated. strlcpy is a BSD extension, so it's not guaranteed to be portable. glibc refuses to implement it.

For maximum portability, efficiency, and safety use strlcpy and provide a fallback using memmove and #ifndef strlcpy so it will only be used if strlcpy is not already available.

#include <string.h>
#include <stdio.h>

#ifndef strlcpy
size_t strlcpy(char * restrict dst, const char * restrict src, size_t dst_size) {
    /* size is the allocated size. len leaves space for the null byte */
    size_t dst_len = dst_size - 1;
    size_t src_len = strlen(src);

    /* Use the smaller of the two string lengths to avoid buffer overflow */
    size_t move_len = src_len > dst_len ? dst_len : src_len;

    /* Copy the string, truncate if necessary. It will work
     * even if src and dst overlap. */
    memmove(dst, src, move_len);

    /* Guarantee there's a null byte */
    dst[move_len] = '\0';

    /* strlcpy returns the size of the string it tried to make.
     * This is used to detect truncation. */
    return src_len;
}
#endif

int main()
{
    char dst[10];
    char *src = "12345678901234567890";

    printf("%zu\n", strlcpy(dst, src, 10));
    printf("src: %s, dst: %s\n", src, dst);

    return 0;
}

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.