2
\$\begingroup\$

I am writing a program that needs to retain specific data but that should not be easily readable. I am in no way very skilled in crypto but I wrote this class that takes all of the data in and then returns the AES encrypted strings.

Is this an appropriate way to encrypt the data for storage in XML?

public class Crypto {

private String home;
private static PasswordProtection pwP;


public Crypto(String choice) throws Exception
{
    pwP = new PasswordProtection("cryptoTesting".toCharArray());
    home = System.getProperty("user.home") + "\\dataStore.keystore";
}


//Method to access the keystore, generate, and store the key.
public void storeSecretKey() throws KeyStoreException, IOException, NoSuchAlgorithmException, CertificateException 
{
    //create the actual Keystore file
    KeyStore keyStore = createKeyStore(home, "cryptoTesting");

    //Generate our secret key
    SecretKey key = KeyGenerator.getInstance("AES").generateKey();
    KeyStore.SecretKeyEntry keyStoreEntry = new KeyStore.SecretKeyEntry(key);


    //Store our secret key
    keyStore.setEntry("dataVariables", keyStoreEntry, pwP);
    keyStore.store(new FileOutputStream(home), "cryptoTesting".toCharArray());

}

//retrieve our AES key from the store
public SecretKey retrieveKey() throws Exception
{
    //Load up the KeyStore file
    KeyStore store = createKeyStore(System.getProperty(home), "cryptoTesting");

    //Retrieve the entry from the keystore
    KeyStore.Entry entry = store.getEntry("dataVariables", pwP);

    //Assign the entry as our secret key for later retrieval.
    SecretKey key = ((KeyStore.SecretKeyEntry) entry).getSecretKey();

    return key;
}

//Actually create the keystore file.
private KeyStore createKeyStore(String path, String password) throws KeyStoreException, IOException, NoSuchAlgorithmException, CertificateException
{
    File file = new File(home);

    final KeyStore keyStoreCreate = KeyStore.getInstance("JCEKS");

    if(file.exists())
    {
        //if the file exists load the keystore.
        keyStoreCreate.load(new FileInputStream(file), password.toCharArray());
    }
    else
    {
        //create keyStore.
        keyStoreCreate.load(null,null);
        keyStoreCreate.store(new FileOutputStream(home), password.toCharArray());
    }
    return keyStoreCreate;
}

//Encrypt the data passed to the method
public String encryptData(String data) throws Exception
{
    String encryptedString = "";

    //Actual encryption process
    Cipher cipher = Cipher.getInstance("AES");
    byte[] dataToEncrypt = data.getBytes();
    cipher.init(Cipher.ENCRYPT_MODE, retrieveKey());
    byte[] encryptedBytes = cipher.doFinal(dataToEncrypt);
    Base64.Encoder encoder = Base64.getEncoder();
    encryptedString = encoder.encodeToString(encryptedBytes);

    return encryptedString;
}

//Decrypt the data retrieved from the XML File
public String decryptData(String data) throws Exception 
{
    String decryptedText = "";
    Cipher cipher = Cipher.getInstance("AES");
    Base64.Decoder decoder = Base64.getDecoder();
    byte[] encryptedBytes = decoder.decode(data);
    cipher.init(Cipher.DECRYPT_MODE, retrieveKey());
    byte[] decryptedBytes = cipher.doFinal(encryptedBytes);
    decryptedText = new String(decryptedBytes);
    return decryptedText;
}
 }

Here is the XML class to create that portion:

public class DataStorage {
private String account, username, password;

public DataStorage(String account, String username, String password)
{
    this.account = account;
    this.username = username;
    this.password = password;
}

public void CreateXML() throws Exception
{
    try
    {
        DocumentBuilderFactory documentFactory = DocumentBuilderFactory.newInstance();
        DocumentBuilder builder = documentFactory.newDocumentBuilder();

        //Root element addition
        Document document = builder.newDocument();
        Element rootElement = document.createElement("AccountInformation");
        document.appendChild(rootElement);

        //Identification number
        Element userId = document.createElement("Developer");
        rootElement.appendChild(userId);

        //ID attributes
        Attr attr = document.createAttribute("id");
        attr.setValue("######-#######-#######-#######");
        userId.setAttributeNode(attr);

        //account name 
        Element accountName = document.createElement("Account");
        accountName.appendChild(document.createTextNode(account));
        userId.appendChild(accountName);

        //user id
        Element userAccount = document.createElement("Username");
        userAccount.appendChild(document.createTextNode(username));
        userId.appendChild(userId);

        //password
        Element userPassword = document.createElement("Password");
        userPassword.appendChild(document.createTextNode(password));
        userId.appendChild(userPassword);

        //write to XML
        TransformerFactory transformerFactory = TransformerFactory.newInstance();
        Transformer transformer = transformerFactory.newTransformer();
        transformer.setOutputProperty(OutputKeys.INDENT,"yes");
        transformer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", "2");
        DOMSource source = new DOMSource(document);


        //String storage path
        String path = System.getProperty("user.home") + "\\dataFile.XML";
        StreamResult result = new StreamResult(new File(path));

        transformer.transform(source, result);

    }
    catch(ParserConfigurationException | DOMException | IllegalArgumentException | TransformerException exc)
    {
        exc.printStackTrace();
    }
}
}
\$\endgroup\$
3
\$\begingroup\$

Is this an appropriate way to encrypt the data for storage in XML?

Maybe. Note that encryption always works with bytes. So it doesn't care if your file is an image, a plaintext, an XML, or even something worse.

Crypto

private String home;
private static PasswordProtection pwP;

Why is the latter static? Shouldn't both be just final?

public Crypto(String choice) throws Exception
{
  • But you have no choice. Unused constructor argument makes hardly even sense.
  • The brace belongs on the previous line.
  • Declaring throws Exception forces you to catch Exception later. This is nearly always wrong. Wouldn't GeneralSecurityException do?

    pwP = new PasswordProtection("cryptoTesting".toCharArray());

pwP is a terrible name. A password stored in the code gives you zero security against anyone in possession of the code. But there may not be a way out.

home = System.getProperty("user.home") + "\\dataStore.keystore";

Using \ as a separator is a terrible idea. For very lazy people or use-once code, / can be used as it (nearly always) works on Windows, too. The right way is to use File.separator.(*)

//Method to access the keystore, generate, and store the key.
public void storeSecretKey() throws KeyStoreException, IOException, NoSuchAlgorithmException, CertificateException 

The comment buys you nothing:

  • It's obviously a method, so never use the word "method".
  • The method is called storeSecretKey, either it's exacly what it does, or it should be renamed.
  • It actually generates and stores a key, so I'd call it generateAndStoreSecretKey.
  • Comments with // or /* ... */ are for explaining code details or hacks. For method, Javadoc should be used.

    //create the actual Keystore file KeyStore keyStore = createKeyStore(home, "cryptoTesting"); KeyStore.SecretKeyEntry keyStoreEntry = new KeyStore.SecretKeyEntry(key);

Not much information in the comment, but OK. Saying what a small code lock does makes sometimes sense. But note that writing Javadoc for non-trivial methods is way more important.

Use constants. Especially for repeated things like here.

private static final KEYSTORE_NAME = "cryptoTesting";

Twenty lines later, I see that I was wrong. It's not the KEYSTORE_NAME, but a PASSWORD. Using a properly named constant makes it clear.

 

//Store our secret key
keyStore.setEntry("dataVariables", keyStoreEntry, pwP);
keyStore.store(new FileOutputStream(home), "cryptoTesting".toCharArray());

For non-trivial file sizes you should use

new BufferedOutputStream(new FileOutputStream(home))

final KeyStore keyStoreCreate = KeyStore.getInstance("JCEKS");

Calling a variable ...Create is ugly. Variable names should be nouns, method names should be verbs. Just keyStore or better result.

The reason why crypto uses char[] instead of String is security. Strings are immutable and will hang around until they gets GC'ed, while an array can be cleared after use. But with the password being part of the program, you can safely ignore such things.


//Encrypt the data passed to the method
public String encryptData(String data) throws Exception
{

Again a negative-value comment. The method is clear without it. You could make it clearer with something like

public String encrypt(String plaintext) throws GeneralSecurityException {

 

    String encryptedString = "";

You really really don't need it here. Always minimize the scope of all variables. Actually, you don't need it at all, just

return encoder.encodeToString(encryptedBytes);

a few lines later.

Note that there's usually no need for the String result. For storing in a file, a byte[] is simpler. Actually, for everything a byte[] is better. You don't show any use of encryptData.

//Decrypt the data retrieved from the XML File
public String decryptData(String data) throws Exception 

The comment is just confusing. If you later decide to switch from XML to something better, it'll stay there and confuse the reader even more. There's no XML in this method.

DataStorage

private String account, username, password;

The variables should be final. By convention, there should be just one variable per line.

public void CreateXML() throws Exception

Method names should always start with a lowercase letter!

catch(ParserConfigurationException | DOMException | IllegalArgumentException | TransformerException exc)
{
    exc.printStackTrace();
}

Wouldn't be a throws clause better? Printing the stack trace doesn't look like it solved the problem, does it?


(*) Many thanks Sun for this ugliness. Other languages (e.g. perl) simply guarantee that / works everywhere.

\$\endgroup\$
  • \$\begingroup\$ Well, I do thank you for your input on everything. As I said this is merely a personal code project to attempt to learn things that I have not previously been exposed to. You make several extremely valid points and I will take them and go back to try and make the code better and more functional. As for the comments those were merely my personal reminders. I know the method should be self explanatory but on my personal things I like to just jot something down for personal remembrance. Thank you for you tips and I will go back and make it not ugly and moronic. \$\endgroup\$ – basic Jun 25 '15 at 20:07
  • \$\begingroup\$ @xXspynXx You're welcome. And thank you for the kind words. It wasn't "ugly and moronic", it's just that there's always a lot to say. Have fun! \$\endgroup\$ – maaartinus Jun 25 '15 at 20:15
  • \$\begingroup\$ haha quite welcome like always I came here for some advice and guidance and you followed through :) so thank you for that! \$\endgroup\$ – basic Jun 25 '15 at 20:18
0
\$\begingroup\$

In createKeyStore you don't actually use the first parameter path. Instead you default to home always.

The home field is a File path and everywhere you use it (except in one of the calls createKeyStore where it doesn't have any effect) you use it as such; so you may as well store it as a File and make it clear.

This lets you avoid the \\ vs / and use the constructor that always works:

home = new File(System.getProperty("user.home"), "dataStore.keystore");
\$\endgroup\$

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.