Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 <string.h>
#include <stdio.h>
#include <stdint.h>
#include <openssl/evp.h>
#include <openssl/sha.h>

// 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 main(int argc, char *argv[])
{
  printf("Usage: %s SHA-512|SHA-256 password salt iterations outputlengthinbytes\nSHA-512 is recommended (for password hashing, outputlengthinbytes <= 64)\nSHA-256 is not as good, (for password hashing, outputlengthinbytes <= 32)\n",argv[0]);
  if(argc != 6){printf("Incorrect number of parameters.\n");return 2;};
  char pass[strlen(argv[2])];
  unsigned char salt[strlen(argv[3])];
  int32_t iterations;
  uint32_t outputbytes;

  strcpy(pass,argv[2]);
  strcpy(salt,argv[3]);
  iterations = atoi(argv[4]);
  outputbytes = atoi(argv[5]);
  printf("\n%s\n",argv[1]);

  char HexResult[2*outputbytes-1];

//    printf("Computing PBKDF2(HMAC-SHA512, '%s', '%s', %d, %d) ...\n", pass, salt, iterations, outputbytes);
  memset(HexResult, 0, outputbytes);
  if (strcmp(argv[1],"SHA-512")==0)
    {
    PBKDF2_HMAC_SHA_512(pass, salt, iterations, outputbytes, HexResult);
    }
  else if (strcmp(argv[1],"SHA-256")==0)
    {
    PBKDF2_HMAC_SHA_256(pass, salt, iterations, outputbytes, HexResult);
    }
  else
    {
    printf("Invalid value for hash - choose one of:\nSHA-512\nSHA-256\n");
    return 1;
    }

  printf("Result: in Base16 (hex)\n%s\n", HexResult);
  return 0;
}
share|improve this question
add comment

2 Answers

up vote 7 down vote accepted

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. ;)

share|improve this answer
1  
Hah! I'd forgotten to convert over to puts() or fputs(). Credit the good stuff to the original author (except the CLI arguments). Thank you for the good comments! –  Anti-weakpasswords Feb 22 at 4:49
add comment
  1. A buffer overflow:

    char HexResult[2*outputbytes-1];
    

    Why the -1? There are 2 * outputbytes hex digits. So you need at least 2*outputbytes chars. Since you want a null terminated string (the rest of your code assumes it is), you even need an extra character for the \0 terminator. So this should be char HexResult[2 * outputbytes + 1];

  2. A partial initialization:

    memset(HexResult, 0, outputbytes);
    

    You're filling the first half of HexResult with \0, but you're missing the rest. This can't be right.

  3. A useless mask:

    255 & digest[i]
    

    What's the point of the masking? If a byte of the output were larger than that, that'd mean that you're running on an unusual architecture with large chars and OpenSSL has a bug.

    You could use uint8_t if you want to be explicit about using 8 bit bytes. Many crypto libraries use these typedefs.

share|improve this answer
    
Thank you, CodesInChaos, for two critical points and for the performance improvement. As it happens, I'd fixed the first two and not noticed the third in my quest to get argument parsing and x64 compilation working. –  Anti-weakpasswords Mar 1 at 1:04
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.