OpenSSL PBKDF2-HMAC-SHAx proof of concept

I have regrettably been away from C programming for a very long time, so I'd like to get a quick code review of a first proof of concept, before I get too far into adding capabilities, using the getopt library for arguments, alternate output and input character sets, a PolarSSL implementation, and so on and so forth.

Target platforms: gcc 4.5+ for MinGW for Windows, gcc 4.7+ for Linux.

My ranking of concerns:

• Security holes

• Incorrect use of OpenSSL

• Memory leaks

• Performance

• Suboptimal or incorrect compilation commands

• Inefficient code

• deviations from ANSI C/portability

• Unclean code

• Suggestions for improvement

• other

As a purely secondary interest, if anyone has advice on the getopt library, C code for Base32/Base64/etc output formats, or PolarSSL use, I'd appreciate it.

Linux compilation:

gcc -O3 -lssl pbkdf2.c -o pbkdf2

Windows compilation:

gcc -O3 -I OpenSSLLibraryPath\include -L OpenSSLBinariesPath -leay32 pbkdf2.c -o pbkdf2

The code:

```
#include
#include
#include
#include
#include

// Originally from http://stackoverflow.com/questions/10067729/fast-sha-2-authentication-with-apache-is-it-even-possible

void PBKDF2_HMAC_SHA_256(const char pass, const unsigned char salt, int32_t iterations, uint32_t outputbytes, char* HexResult)
{
unsigned int i;
unsigned char digest[outputbytes];
PKCS5_PBKDF2_HMAC(pass, strlen(pass), salt, strlen(salt), iterations, EVP_sha256(), outputbytes, digest);
for (i = 0; i < sizeof(digest); i++)
sprintf(HexResult + (i * 2), "%02x", 255 & digest[i]);
}

void PBKDF2_HMAC_SHA_512(const char pass, const unsigned char salt, int32_t iterations, uint32_t outputbytes, char* HexResult)
{
unsigned int i;
unsigned char digest[outputbytes];
PKCS5_PBKDF2_HMAC(pass, strlen(pass), salt, strlen(salt), iterations, EVP_sha512(), outputbytes, digest);
for (i = 0; i < sizeof(digest); i++)
sprintf(HexResult + (i * 2), "%02x", 255 & digest[i]);
}

int

Things you did well on:

-
Looks like you weren't too rusty based on this code. Looks very nice organizationally.

-
You return different values for different error conditions.

Things you could improve on:

Standards

-
Initialize i in your for loops.(C99)

for (unsigned int i = 0; i < sizeof(digest); i++)

Syntax

-
Don't over-compact your code.

if(argc != 6){printf("Incorrect number of parameters.\n");return 2;};

Let it breathe a bit.

if(argc != 6)
{
    printf("Incorrect number of parameters.\n");
    return 2;
}

-
Use puts() instead of printf() when you aren't formatting a string.

puts("Incorrect number of parameters.");

-
You don't follow proper C naming conventions.

char HexResult[2*outputbytes-1];

Either use camelCase, or snake_case with variable names.

-
Your indentation is a bit odd.

if (strcmp(argv[1],"SHA-512")==0)
  {
  PBKDF2_HMAC_SHA_512(pass, salt, iterations, outputbytes, HexResult);
  }

But that could be a copy-paste issue.

Commenting:

-
The only comments I see are a link to a Stack Overflow post, and a commented-out bit of old code. Use more comments to explain what your code does, and how it accomplishes that/why it works.

Look at it as if a new developer was going to look at your code. You should try to explain it so that he could understand it's basic function and how it works right away. You might thank yourself when you return back to this project in the future, forgetting completely how it works. ;)

StackExchange Code Review Q#42492, answer score: 8