Vigenere cipher C program

Question

I implemented a Vigenere cipher that preserves case and can also decrypt if you pass the -d argument. Usage is ./vigenere <plaintext|ciphertext> key [-d] and it works fine but I think there are probably some things I could do better. So, just looking for critique.

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

#define ENCRYPT 1
#define DECRYPT -1

int mod(int x, int n)
{
    return (x % n + n) % n;
}

char *vigenere(char *input, char *keystream, int mode)
{
    if (strlen(keystream) != strlen(input))
        return NULL;
    
    int output_len = strlen(input), offset = 'a', wasupper = 0; 
    char *output = malloc(output_len);
    
    for (int i = 0; i < output_len; ++i)
    {
        if (isupper(input[i]))
        {
            input[i] = tolower(input[i]);
            wasupper = 1;
        }
        else
            wasupper = 0;
            
        output[i] = mod((input[i] - offset) + (keystream[i] - offset) * mode, 26) + offset;
        
        if (wasupper)
            output[i] = toupper(output[i]);
    }

    return output;
}

char *gen_keystream(char *key, int keystream_len)
{
    int key_len = strlen(key), j = 0;
    char *keystream = malloc(keystream_len);
    
    for (int i = 0; i < keystream_len; j = ++i % key_len)
        keystream[i] = key[j];
        
    return keystream;
}

int main(int argc, char **argv)
{
    if (argc < 3)
        return EXIT_FAILURE;
        
    int mode = ENCRYPT;
        
    if (argc > 3 && strcmp(argv[3], "-d") == 0)
        mode = DECRYPT;
        
    int keystream_len = strlen(argv[1]);
    char *keystream = gen_keystream(argv[2], keystream_len);
    char *result = vigenere(argv[1], keystream, mode);
    
    printf("%s\n", result);

    free(keystream);
    free(result);
    
    return EXIT_SUCCESS;
}

Answer 1

Process a stream of data instead of a single string

Your program works by passing the input as a command-line argument, but what if you want to encode or decode a whole file? What if the input is huge? I would try to modify the program so that it can run from a file, or from standard input if no file is specified, so you can do for example:

./vigenere key < plaintext.txt > ciphertext.txt

Answer 2

Bug: no null character

printf("%s\n", result); fails as result does not point to a string. That data lacks a null character and is short, by 1 of allocated memory needed.

// char *output = malloc(output_len);
char *output = malloc(output_len +1);
output[output_len] = '\0';

Similar problem with decryptor.

No protection against non-A-Z characters

I'd expect only encrypting/decrypting when the input[] is an A-Z.

I suspect code may generate output[i] == 0 for select inputs, rendering the string short.

---

Pedantic bug: Negative char

When input[i] < 0, isupper(input[i]) is UB. Better to access the string via an unsigned char * for is...().

A negative char value also messes-up mod(). When x < 0, x % n code does not perform a Euclidean mod.

Answer 3

Here are some things that may help you improve your code.

Always use braces

The code currently includes this:

if (isupper(input[i]))
{
    input[i] = tolower(input[i]);
    wasupper = 1;
}
else
    wasupper = 0;

    output[i] = mod((input[i] - offset) + (keystream[i] - offset) * mode, 26) + offset;
    
    if (wasupper)
        output[i] = toupper(output[i]);
    

The problem is that the indentation after the else suggests that all of those lines are executed only if isupper(input[i]) evaluates to false, but that's not the case. Only the wasupper = 0; line is exclusively executed on that path. So then the reader of the code has to try to figure out if it's an indentation error or a missing braces error. For that reason, especially if you're new to the language, I recommend always using {} for such conditional constructs.

Fix the bugs

A string in C must be terminated with a '\0' character to be able to use function calls such as strlen(), but there are bugs in gen_keystream and in vigenere that fail to account for the terminator. Remember that strlen() returns the length of a string excluding this terminating character.

Think carefully about mathematical operations

This function is not broken, but it's not as efficient as it could be:

int mod(int x, int n)
{
    return (x % n + n) % n;
}

We can express the same notion using simply return (x + n) % n; given the expected range of x is \$(-n,n)\$. You can also simplify a bit more than this, as I'll demonstrate later.

Use const where practical

Because vigenere should alter neither input nor keystream, both should be declared const.

Put each statement on a single line

It is detrimental to the readability of your code declare several variables on the same line or to abuse the comma operator:

int output_len = strlen(input), offset = 'a', wasupper = 0; 

Instead, separating each statement on its own line makes the code easier to read and maintain. See ES.10

Check return values for errors

Calls to malloc can fail. You must check the return values to make sure they haven't or your program may crash (or worse) when given malformed input or due to low system resources. Rigorous error handling is the difference between mostly working versus bug-free software. You should strive for the latter.

Consider whether a function is needed

There are other ways to structure the program so that neither gen_keystream nor keystream are needed. You could simply pass the key into a modified version of vigenere.

Consider using pointers instead of indexing

Using pointers effectively is an essential skill for every C programmer. Here's a rewritten version of your function using most of the simplifications and corrections mentioned above:

char *vigenere(const char *input, const char *key, int mode)
{
    int output_len = strlen(input); 
    if (output_len == 0) {
        return NULL;
    }
    // add +1 for the terminating NUL character
    char *output = malloc(output_len + 1);
    if (output == NULL) {
        return output;
    }
    const char *keyptr = key;
    output[output_len] = '\0';
    
    for (char *curr = output ; *input; ++input) {
        if (*keyptr == '\0') {
            keyptr = key;
        }
        *curr = (mode * (*keyptr - tolower(*input)) + 26) % 26 + 'a';
        if (isupper(*input)) {
            *curr = toupper(*curr);
        }
        ++curr;
        ++keyptr;
    }
    return output;
}

Note that in this code, I assume that all characters have already been checked and isalpha() is true for all input and that islower() and isalpha() are both true for every character of the key.

Answer 4

This isn't a Vignère cipher

This line:

if (strlen(keystream) != strlen(input))
    return NULL;

means that the key needs to be exactly the same length as the input. So what you really have is a one-time-pad.

A real Vignère cipher, in contrast, can encode arbitrarily large messages, by re-using the key every strlen(key) input characters. This explains how a Vignère cipher is less secure than OTP, particularly for short keys - but it's much more usable.