3
\$\begingroup\$

This is a Java program that generates a random keyfile (for OTP, for example).

package kfgen;

import java.io.BufferedOutputStream;
import java.io.File;
import java.io.FileOutputStream;
import java.security.SecureRandom;
import java.util.Random;
import java.util.Scanner;

public class Kfgen {

    // At least it's not a magic number.
    protected static final int MEGABYTE = 1048576;

    public static void main(String[] args) {

    System.out.println("Name?\n>");

    // choice: Scanner to get filename.
    // bos: output stream to write the keyfile
    try (final Scanner choice = new Scanner(System.in);
        final BufferedOutputStream bos = new BufferedOutputStream(
            new FileOutputStream(
                new File(choice.next()))
        )) {

        // Slow prng, but hey, this is cryptography, so ... IANAC
        final Random sr = SecureRandom.getInstanceStrong();

        System.out.println("Enter size in GB:\n>");

        // MultiplyExact to throw ugly exception if too large.
        // Dunno if this ever happens.
        int bytesize = Math.multiplyExact(choice.nextInt(), 1024);

        // Tell how many bytes.
        System.out.println(bytesize * MEGABYTE + " bytes!");

        // Write all that data in 1 MB chunks
        // (otherwise byte[] can't hold that much).
        // Given that I'm pseudo-buffering at 1 MB, should I use
        // unbuffered fileout?
        for (int i = 0; i < bytesize; i++) {
        final byte[] rand = new byte[MEGABYTE];
        sr.nextBytes(rand);
        bos.write(rand);
        }
    } catch (Exception ex) {
        throw new Error(ex); // I don't want to deal with it.
    }

    }

}

Please evaluate this for security, coding practice, and efficiency.

\$\endgroup\$
3
\$\begingroup\$

Your code is pretty good :-)

However, there a few things that could be improved.

// At least it's not a magic number.
protected static final int MEGABYTE = 1048576;

is still pretty magic, 1024 * 1024 would seem more readable. It should not be protected without reason. For constants, use private if unsure, public if you need access from other classes. protected should be used if it should be visible to subclasses and there is a specific reason not to use public.

} catch (Exception ex) {
    throw new Error(ex); // I don't want to deal with it.
}

Don't throw Errors. Use some kind of RuntimeException. If you are in your main method and you do not care, just rethrow it.

It is generally advisable to separate concerns. In this case you read some user input and use this to write (crypto-usable) random data to a file.

Applying some object orientation would go a long way to separate these two unrelated tasks.

Please consider the following code:

package kfgen;

import java.io.BufferedOutputStream;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.util.Scanner;

public class Kfgen {
    private static final int MEGABYTE = 1024 * 1024;
    private static final long GIGABYTE = MEGABYTE * 1024;

    private final SecureRandom prng;

    public Kfgen(SecureRandom prng) {
        this.prng = prng;
    }

    public void generate(File keyFile, int gigabytes) throws IOException {
        // MultiplyExact to throw ugly exception if too large.
        int megabytes = Math.multiplyExact(gigabytes, 1024);

        try (OutputStream out = new BufferedOutputStream(new FileOutputStream(
                keyFile))) {
            final byte[] buffer = new byte[MEGABYTE];
            for (int i = 0; i < megabytes; i++) {
                prng.nextBytes(buffer);
                out.write(buffer);
            }
        }
    }

    public static void main(String[] args) throws NoSuchAlgorithmException,
            IOException {
        try (Scanner scanner = new Scanner(System.in)) {
            System.out.println("Name?\n>");
            String filename = scanner.next();

            System.out.println("Enter size in GB:\n>");
            int gigabytes = scanner.nextInt();

            // Tell how many bytes.
            System.out.println(gigabytes * GIGABYTE + " bytes!");

            Kfgen kfgen = new Kfgen(SecureRandom.getInstanceStrong());
            kfgen.generate(new File(filename), gigabytes);
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

Security and correctness

The result of getInstanceStrong may block waiting for entropy (it's equivalent to /dev/random, not /dev/urandom). Reading a gigabyte will presumably use up the available entropy, so this will be a problem in practice — it will take many hours to accumulate that much randomness. Do you really need true randomness? Can you use a non-blocking pseudo-random number generator instead?

Beware of reinventing the wheel. This program is equivalent to a one-line shellscript:

head -c 1G /dev/random >keyfile

UI

Scanner is a special-purpose tool that won't read all filenames correctly. For example, if you enter one time pad, it parses that as three tokens, and writes to the file one. Read a line instead.

What happens if the filename is not valid? Perhaps it should complain and prompt again?

A command-line UI would be more flexible than prompting; it particular it's easier to call from scripts: generate-key --size 1G keyfile?

1 GB is a very large quantum. Do you expect users to only need very large keys? It would be more flexible to take a size in bytes or kilobytes.

Avoid exclamation points in messages to users, or they'll think something is seriously wrong. 12345 bytes, not 12345 bytes!

Style

The code than writes random files should be separated from the UI. How about a writeRandomFile(path, size) method?

bytesize is misnamed: it's actually a size in megabytes.

rand can be much smaller (perhaps 1 KB?). It can be reused rather than allocating a new one:

final byte[] rand = new byte[1024];
for (int i = 0; i < kilobytes; i++) {
    sr.nextBytes(rand);
    bos.write(rand);
}

A BufferedOutputStream is unnecessary if you only write 1MB chunks. This program does buffering by hand, so there's nothing for the BufferedOutputStream to do.

\$\endgroup\$
  • \$\begingroup\$ It is not a keyfile per se (it's OTP, so byte-to-byte match with plaintext). Also, the RNG isn't slow at all. 10 gigabytes take a couple minutes. \$\endgroup\$ – Simon Kuang May 20 '14 at 23:08
  • \$\begingroup\$ It may work on your machine (perhaps it has a hardware RNG, or it's falling back to a PRNG), but it's not portable. \$\endgroup\$ – Anonymous May 20 '14 at 23:35
1
\$\begingroup\$

You forgot to close your BufferedOutputStream and best practice for that is within a finally clause

try
{
}
catch (Exception e)
{
}
finally
{
    bos.close();
}

final byte[] rand = new byte[MEGABYTE];

no real reason to define the byte[] as final since your only reinitializing every time

\$\endgroup\$
  • \$\begingroup\$ The BufferedOutputStream is in a try-with-resources block, which is even better practice than explicitly closing inside finally. \$\endgroup\$ – 200_success May 20 '14 at 19:53
  • \$\begingroup\$ my mistake I missed that. \$\endgroup\$ – ndrone May 20 '14 at 20:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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