8
\$\begingroup\$

I am developing JSafebox, a portable safebox project written in Java. The goal is to provide a secured environment where the user can browse encrypted files without leaking data on the drive.

The project is highly focused on security and I would appreciate any feedback that would help improve the quality of the code.

The majority of sensitive operations are located in the following classes:

org/ortis/jsafebox/Safe.java

/*******************************************************************************
 * Copyright 2018 Ortis ([email protected])
 * 
 * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
 * 
 * http://www.apache.org/licenses/LICENSE-2.0
 * 
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 * License for the specific language governing permissions and limitations under the License.
 ******************************************************************************/

package org.ortis.jsafebox;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.Closeable;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.RandomAccessFile;
import java.lang.reflect.Type;
import java.nio.file.Files;
import java.security.spec.AlgorithmParameterSpec;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.TreeMap;
import java.util.concurrent.CancellationException;

import javax.crypto.Cipher;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.SecretKeySpec;

import org.ortis.jsafebox.task.TaskProbe;

import com.google.gson.Gson;
import com.google.gson.reflect.TypeToken;

/**
 * Virtual vault where files are stored
 * 
 * @author Ortis <br>
 *         2018 Apr 26 7:29:29 PM <br>
 */
public class Safe implements Closeable
{

    public final static String UTF8 = "UTF-8";
    public final static Gson GSON = new Gson();
    private final static Type MAP_STRING_STRING_TYPE = new TypeToken<Map<String, String>>()
    {
    }.getType();

    public final static Type BYTE_ARRAY_TYPE = new TypeToken<byte []>()
    {
    }.getType();

    public final static String ENCRYPTION_LABEL = "encryption";
    public final static String ENCRYPTION_IV_LABEL = "iv";
    public final static String KEY_ALGO_LABEL = "algo";
    public final static String PROTOCOL_SPEC_LABEL = "protocol description";

    public final static String PROTOCOL_SPEC = "JSafebox is using a very simple protocol so encrypted files can be easily read by another program, as long as you have the encryption password.Each datagram is preceded by its length stored as a 64 bits (8 bytes) integer (`long` in Java): length 0|datagram 0|length 1|datagram 1|length 3|...|datagram N. The first datagram `datagram 0` is the *header* and is **the only datagram not encrypted**. The *header* contains text entries specified by the user and various additional entries incuding a protocol explanation, the type of encoding and the IV of the encryption. The *header*'s data is stored in JSON format and can be seen by opening the safe file with a basic text editor. The second datagram `datagram 1` is the *properties*. It contains encrypted text entries specified by the user. The following datagrams (from 2 to N) are the encrypted files. They work by pair: `datagram i ` contains the metadata of the file as an encrypted JSON text and `datagram i+1` contains the bytes of the encrypted file.";

    private final File originalFile;

    private final Cipher cipher;
    private final SecretKeySpec keySpec;
    private final AlgorithmParameterSpec algoSpec;

    private final RandomAccessFile original;

    private final File tempFile;
    private final RandomAccessFile temp;

    private final Map<String, String> publicHeader;
    private final Map<String, String> privateProperties;
    private final Map<String, Block> roBlocks;
    private final Map<String, Block> blocks;
    private final Map<String, Block> tempBlocks;
    private final Map<String, Block> deletedBlocks;

    private final int bufferSize;

    private final Folder root;

    /**
     * Create an instance of {@link Safe}
     * 
     * @param file:
     *            the safe file
     * @param cipher:
     *            cipher to decrypt the data
     * @param keySpec:
     *            key specification
     * @param algoSpec:
     *            encryption specification
     * @param bufferSize:
     *            size of the <code>byte</code> buffer to be used in IO operation
     * @throws Exception
     */
    public Safe(final File file, final Cipher cipher, final SecretKeySpec keySpec, final AlgorithmParameterSpec algoSpec, final int bufferSize) throws Exception
    {

        this.originalFile = file.getAbsoluteFile();
        this.cipher = cipher;

        this.keySpec = keySpec;
        this.algoSpec = algoSpec;
        this.bufferSize = bufferSize;

        this.cipher.init(Cipher.DECRYPT_MODE, this.keySpec, this.algoSpec);
        this.original = new RandomAccessFile(file, "rw");
        this.tempFile = Files.createTempFile(null, null).toFile();
        this.temp = new RandomAccessFile(this.tempFile, "rw");

        final HashMap<String, String> publicProps = new LinkedHashMap<>();
        this.publicHeader = Collections.unmodifiableMap(publicProps);
        final HashMap<String, String> props = new LinkedHashMap<>();
        this.privateProperties = Collections.unmodifiableMap(props);
        this.blocks = new LinkedHashMap<>();
        this.roBlocks = Collections.unmodifiableMap(blocks);
        this.tempBlocks = new LinkedHashMap<>();
        this.deletedBlocks = new LinkedHashMap<>();
        this.root = new Folder(null, Folder.ROOT_NAME);

        final byte [] buffer = new byte[bufferSize];
        final byte [] outBuffer = new byte[bufferSize];

        long length;
        int read;

        final ByteArrayOutputStream baos = new ByteArrayOutputStream(buffer.length);

        final long headerLength = this.original.readLong();

        length = headerLength;

        while (length > 0)

        {

            if (length < buffer.length)
                read = this.original.read(buffer, 0, (int) length);
            else
                read = this.original.read(buffer);

            baos.write(buffer, 0, read);
            length -= read;

        }

        String json = new String(baos.toByteArray(), UTF8);

        publicProps.putAll(GSON.fromJson(json, MAP_STRING_STRING_TYPE));

        // read private privateProperties
        final long propLength = this.original.readLong();
        length = propLength;

        baos.reset();

        while (length > 0)
        {

            if (length < buffer.length)
                read = this.original.read(buffer, 0, (int) length);
            else
                read = this.original.read(buffer);

            final int decrypted = cipher.update(buffer, 0, read, outBuffer);

            baos.write(outBuffer, 0, decrypted);
            // baos.write(buffer, 0, read);
            length -= read;

        }

        baos.write(cipher.doFinal());

        json = new String(baos.toByteArray());

        props.putAll(GSON.fromJson(json, MAP_STRING_STRING_TYPE));

        while (this.original.getFilePointer() < this.original.length())
        {
            baos.reset();
            final long offset = this.original.getFilePointer();
            final long metaLength = this.original.readLong();
            final long metaOffset = this.original.getFilePointer();

            length = metaLength;
            while (length > 0)
            {

                if (length < buffer.length)
                    read = this.original.read(buffer, 0, (int) length);
                else
                    read = this.original.read(buffer);

                final int decrypted = cipher.update(buffer, 0, read, outBuffer);
                baos.write(outBuffer, 0, decrypted);
                length -= read;
            }
            baos.write(cipher.doFinal());
            json = new String(baos.toByteArray());

            final Map<String, String> properties = new HashMap<>(GSON.fromJson(json, MAP_STRING_STRING_TYPE));
            final String path = properties.get(Block.PATH_LABEL);
            if (path == null)
                throw new IllegalStateException("Path of block starting at " + offset + " is not set");

            if (blocks.containsKey(path.toUpperCase(Environment.getLocale())))
                throw new IllegalStateException("Block path " + path + " already exist");

            final long dataLength = this.original.readLong();
            final long dataOffset = this.original.getFilePointer();

            final String [] tokens = path.split(Folder.REGEX_DELIMITER);

            this.root.mkdir(tokens, 1, true);

            final org.ortis.jsafebox.SafeFile dstFile;

            if (tokens.length == 2)
                dstFile = this.root;
            else
                dstFile = this.root.get(tokens, 1, tokens.length - 1);

            if (dstFile == null)
                throw new Exception("Could not find destination folder for block path " + path);

            if (!dstFile.isFolder())
                throw new Exception("Destination folder " + dstFile + " is a block");

            final Folder destinationFolder = ((Folder) dstFile);

            final Block block = new Block(path, properties, offset, dataOffset + dataLength - offset, metaOffset, metaLength, dataOffset, dataLength, destinationFolder);

            destinationFolder.add(block);

            blocks.put(block.getComparablePath(), block);
            this.original.seek(block.getOffset() + block.getLength());
        }

    }

    /**
     * Add data into the {@link Safe}. <b>Note that the data will be stored into the temporary safe file</b>. Use {@link Safe#save()} to save all temporary data
     * 
     * @param properties:
     *            metadata
     * @param data:
     *            data to encrypt
     * @return
     * @throws Exception
     */
    public Block add(final Map<String, String> properties, final InputStream data, TaskProbe probe) throws Exception
    {
        if (probe == null)
            probe = TaskProbe.DULL_PROBE;

        try
        {
            final String path = properties.get(Block.PATH_LABEL);

            if (path == null)
                throw new IllegalArgumentException("Property " + Block.PATH_LABEL + " is missing");

            org.ortis.jsafebox.SafeFile destinationFile = SafeFiles.get(path, this.root, this.root);

            if (destinationFile != null)
                throw new Exception("Block file " + destinationFile + " already exist");

            final String comparablePath = properties.get(Block.PATH_LABEL).toUpperCase(Environment.getLocale());

            final String [] comparableTokens = comparablePath.split(Folder.REGEX_DELIMITER);

            if (comparableTokens.length == 2 && root.getComparableName().equals(comparableTokens[0]))
                destinationFile = this.root;
            else
                destinationFile = this.root.get(comparableTokens, 1, comparableTokens.length - 1);

            if (destinationFile == null)
                throw new Exception("Destination folder " + destinationFile + " does not exists");

            if (!destinationFile.isFolder())
                throw new Exception("Destination " + destinationFile + " is not a folder");

            final Folder destinationFolder = (Folder) destinationFile;

            if (this.roBlocks.containsKey(path) || this.tempBlocks.containsKey(path))
                throw new Exception("Block path " + path + " already exist");

            final String name = properties.get(Block.NAME_LABEL);

            if (name == null)
                throw new IllegalArgumentException("Property " + Block.NAME_LABEL + " is missing");

            cipher.init(Cipher.ENCRYPT_MODE, keySpec, this.algoSpec);

            if (probe.isCancelRequested())
            {
                probe.fireCanceled();
                throw new CancellationException();
            }

            final String metadataserial = GSON.toJson(properties);

            final byte [] metaBuffer = metadataserial.getBytes();

            final RandomAccessFile temp = getTemp();

            final long offset = temp.getFilePointer();

            temp.writeLong(0);
            final long metaOffset = temp.getFilePointer();

            final long metaLength = encrypt(new ByteArrayInputStream(metaBuffer), this.cipher, temp, this.bufferSize, probe);

            long position = temp.getFilePointer();
            temp.seek(offset);
            temp.writeLong(metaLength);
            temp.seek(position);

            position = temp.getFilePointer();
            temp.writeLong(0);
            final long dataOffset = temp.getFilePointer();

            final long dataLength = encrypt(data, this.cipher, temp, this.bufferSize, probe);

            temp.seek(position);
            temp.writeLong(dataLength);

            temp.seek(temp.length());

            final Block block = new Block(path, properties, offset, temp.getFilePointer() - offset, metaOffset, metaLength, dataOffset, dataLength, destinationFolder);
            this.tempBlocks.put(block.getComparablePath(), block);

            destinationFolder.add(block);

            return block;

        } catch (final CancellationException e)
        {
            throw e;
        } catch (final Exception e)
        {
            probe.fireException(e);
            throw e;
        } finally
        {
            probe.fireTerminated();
        }
    }

    /**
     * Delete data from the {@link Safe}. <b>Note that the data wont be deleted until a call to {@link Safe#save()} is made</b>
     * 
     * @param path:
     *            path of the data to delete
     */
    public void delete(final String path)
    {

        final String comparablePath = path.toUpperCase(Environment.getLocale());
        Block deleted = this.blocks.get(comparablePath);

        if (deleted != null)
        {
            final Folder folder = deleted.getParent();
            folder.remove(deleted.getName());
            this.deletedBlocks.put(comparablePath, deleted);
        }

        deleted = this.tempBlocks.remove(comparablePath);

        if (deleted != null)
        {
            final Folder folder = deleted.getParent();
            folder.remove(deleted.getName());
            this.deletedBlocks.put(comparablePath, deleted);
        }
    }

    /**
     * Extract data from the {@link Safe}
     * 
     * @param block:
     *            block to extract
     * @param outputStream:
     *            destination of extracted block
     * @throws Exception
     */
    public void extract(final Block block, final OutputStream outputStream) throws Exception
    {
        extract(block.getPath(), outputStream);
    }

    /**
     * Extract data from the {@link Safe}
     * 
     * @param path:
     *            path of the block to extract
     * @param outputStream:
     *            destination of extracted block
     * @throws Exception
     */
    public void extract(String path, final OutputStream outputStream) throws Exception
    {

        path = path.toUpperCase(Environment.getLocale());

        Block block = this.roBlocks.get(path);

        final RandomAccessFile raf;
        if (block == null)
        {
            block = this.tempBlocks.get(path);
            raf = this.temp;
        } else
            raf = this.original;

        if (block == null)
            throw new Exception("Block " + path + " not found");

        raf.seek(block.getDataOffset());
        this.cipher.init(Cipher.DECRYPT_MODE, this.keySpec, this.algoSpec);
        decrypt(raf, block.getDataLength(), cipher, outputStream, this.bufferSize);

    }

    /**
     * Read the metadata of a {@link Block}
     * 
     * @param block:
     *            block to read
     * @return
     * @throws Exception
     */
    public Map<String, String> readMetadata(final Block block) throws Exception
    {

        this.original.seek(block.getMetaOffset());
        this.cipher.init(Cipher.DECRYPT_MODE, this.keySpec, this.algoSpec);
        final ByteArrayOutputStream baos = new ByteArrayOutputStream();
        decrypt(this.original, block.getMetaLength(), this.cipher, baos, this.bufferSize);

        final String metadata = new String(baos.toByteArray());

        final Map<String, String> jsonMap = GSON.fromJson(metadata, MAP_STRING_STRING_TYPE);
        return new TreeMap<>(jsonMap);
    }

    /**
     * Discard pending modification
     */
    public void discardChanges() throws Exception
    {

        for (final Map.Entry<String, Block> temp : this.tempBlocks.entrySet())
        {

            Folder folder = temp.getValue().getParent();
            folder.remove(temp.getValue().getName());
        }

        this.tempBlocks.clear();

        for (final Map.Entry<String, Block> deleted : this.deletedBlocks.entrySet())
        {
            Folder folder = deleted.getValue().getParent();
            folder.add(deleted.getValue());
        }
        this.deletedBlocks.clear();

    }

    /**
     * Save the modification into the safe file. The current file is renamed and a new file is written. This is to reduce the risk of data loss. This method calls the {@link Safe#close()} before returning
     * 
     * @return
     * @throws Exception
     */
    public Safe save() throws Exception
    {
        return save(null);
    }

    public Safe save(TaskProbe probe) throws Exception
    {

        if (probe == null)
            probe = TaskProbe.DULL_PROBE;
        try
        {
            double progress = 0;
            probe.fireProgress(progress);
            // add non deleted
            probe.fireMessage("Creating temporary file");
            final File newFile = Files.createTempFile(originalFile.getParentFile().toPath(), null, null).toFile();

            try (RandomAccessFile destination = new RandomAccessFile(newFile, "rw"))
            {
                this.cipher.init(Cipher.ENCRYPT_MODE, this.keySpec, this.algoSpec);

                if (probe.isCancelRequested())
                {
                    probe.fireCanceled();
                    throw new CancellationException();
                }

                // public properties
                probe.fireMessage("Writing public header");
                String json = GSON.toJson(this.publicHeader) + "\n";

                long previousPosition = destination.getFilePointer();
                destination.writeLong(0);
                long total = write(new ByteArrayInputStream(json.getBytes()), destination, this.bufferSize, probe);
                long position = destination.getFilePointer();
                destination.seek(previousPosition);
                destination.writeLong(total);
                destination.seek(position);

                if (probe.isCancelRequested())
                {
                    probe.fireCanceled();
                    throw new CancellationException();
                }

                // private properties
                probe.fireMessage("Writing private properties");
                json = GSON.toJson(this.privateProperties);

                previousPosition = destination.getFilePointer();
                destination.writeLong(0);
                total = encrypt(new ByteArrayInputStream(json.getBytes()), this.cipher, destination, this.bufferSize, probe);
                position = destination.getFilePointer();
                destination.seek(previousPosition);
                destination.writeLong(total);
                destination.seek(position);

                if (probe.isCancelRequested())
                {
                    probe.fireCanceled();
                    throw new CancellationException();
                }

                final double steps = this.roBlocks.size() + this.tempBlocks.size() + 1;
                int completed = 0;

                for (final Block block : this.roBlocks.values())
                {

                    if (this.deletedBlocks.containsKey(block.getComparablePath()))
                    {
                        probe.fireMessage("Skipping deleted block " + block.getPath());
                        continue;
                    }

                    probe.fireMessage("Writing block " + block.getPath());
                    this.original.seek(block.getOffset());
                    write(this.original, block.getLength(), destination, this.bufferSize, probe);
                    completed++;
                    progress = completed / steps;
                    probe.fireProgress(progress);
                }

                final RandomAccessFile temp = getTemp();
                for (final Block block : this.tempBlocks.values())
                {

                    if (this.deletedBlocks.containsKey(block.getComparablePath()))
                    {
                        probe.fireMessage("Skipping deleted block " + block.getPath());
                        continue;
                    }

                    probe.fireMessage("Writing block " + block.getPath());
                    temp.seek(block.getOffset());
                    write(temp, block.getLength(), destination, this.bufferSize, probe);
                    completed++;
                    progress = completed / steps;
                    probe.fireProgress(progress);

                }

                probe.fireMessage("Closing IO streams");
                destination.close();

                close();

                probe.fireMessage("Deleting old file");

                if (!this.originalFile.delete())
                    throw new IOException("Unable to delete " + this.originalFile.getAbsolutePath());

                // if (!this.originalFile.renameTo(new File(this.originalFile.getAbsolutePath() + "." + DateTimeFormatter.ofPattern("yyyyMMddHHmmss").format(LocalDateTime.now()))))
                // throw new IOException("Unable to rename " + this.originalFile.getAbsolutePath());

                probe.fireMessage("Renaming file");

                if (!newFile.renameTo(this.originalFile))
                    throw new IOException("Unable to rename " + newFile.getAbsolutePath());

                if (probe.isCancelRequested())
                {
                    probe.fireCanceled();
                    throw new CancellationException();
                }

                probe.fireMessage("Opening new safe");
                probe.fireProgress(1);

                return new Safe(this.originalFile, cipher, keySpec, this.algoSpec, this.bufferSize);
            }
        } catch (final CancellationException e)
        {
            throw e;
        } catch (final Exception e)
        {
            probe.fireException(e);
            throw e;
        } finally
        {
            probe.fireTerminated();
        }

    }

    @Override
    public void close() throws IOException
    {
        this.original.close();

        final RandomAccessFile temp = getTemp();
        if (temp != null)
        {
            temp.close();
            tempFile.delete();
        }

    }

    /**
     * Get the properties of the {@link Safe}
     * 
     * @return
     */
    public Map<String, String> getPrivateProperties()
    {
        return privateProperties;
    }

    /**
     * Get the header of the {@link Safe}
     * 
     * @return
     */
    public Map<String, String> getPublicHeader()
    {
        return publicHeader;
    }

    /**
     * Get all {@link Block} contained in the {@link Safe}
     * 
     * @return
     */
    public Map<String, Block> getBlocks()
    {
        return this.roBlocks;
    }

    /**
     * Get a {@link Block} from the {@link Safe}
     * 
     * @param path:
     *            path of the {@link Block} to retrieve
     * @return
     */
    public Block getBlock(final String path)
    {

        final String comparablePath = path.toUpperCase(Environment.getLocale());

        return this.roBlocks.get(comparablePath);

    }

    /**
     * Get a {@link Block} from the temporary {@link Safe}
     * 
     * @param path:
     *            path of the {@link Block} to retrieve
     * @return
     */
    public Block getTempBlock(final String path)
    {
        final String comparablePath = path.toUpperCase(Environment.getLocale());

        return this.tempBlocks.get(comparablePath);

    }

    /**
     * Get all {@link Block} contained in the temporary {@link Safe}
     * 
     * @return
     */
    public Map<String, Block> getTempBlocks()
    {
        return tempBlocks;
    }

    /**
     * Get deleted {@link Block}
     * 
     * @return
     */
    public Map<String, Block> getDeletedBlocks()
    {
        return deletedBlocks;
    }

    /**
     * Get root {@link Folder}
     * 
     * @return
     */
    public Folder getRootFolder()
    {
        return root;
    }

    public File getFile()
    {
        return this.originalFile;
    }

    /**
     * Get the temporary safe file
     * 
     * @return
     */
    public File getTempFile()
    {
        return tempFile;
    }

    /**
     * Get the temporary safe file
     * 
     * @return
     */
    public RandomAccessFile getTemp() throws IOException
    {/*
        if (this.temp == null)
        {

            if (this.tempFile.exists())
                throw new IOException("File " + this.tempFile + " already exist");

            this.temp = new RandomAccessFile(this.tempFile, "rw");
            this.tempFile.deleteOnExit();

        }
        */
        return this.temp;
    }

    private static long encrypt(final InputStream data, final Cipher cipher, final RandomAccessFile destination, final int bufferSize, final TaskProbe probe) throws Exception
    {

        final byte [] buffer = new byte[bufferSize];
        final byte [] bufferOut = new byte[bufferSize];
        // ByteBuffer in;

        // final ByteBuffer out = ByteBuffer.allocateDirect(buffer.length);

        long total = 0;
        int read;
        while ((read = data.read(buffer)) > -1)
        {

            read = cipher.update(buffer, 0, read, bufferOut);
            total += read;
            destination.write(bufferOut, 0, read);

            if (probe.isCancelRequested())
            {
                probe.fireCanceled();
                throw new CancellationException();
            }

        }

        read = cipher.doFinal(bufferOut, 0);
        destination.write(bufferOut, 0, read);
        total += read;

        return total;

    }

    private static void decrypt(final RandomAccessFile source, final long length, final Cipher cipher, final OutputStream destination, final int bufferSize) throws Exception
    {

        final byte [] buffer = new byte[bufferSize];
        final byte [] bufferOut = new byte[bufferSize];
        // ByteBuffer in;

        // final ByteBuffer out = ByteBuffer.allocateDirect(buffer.length);
        long remaining = length;
        int read;
        while (remaining > 0)
        {
            if (remaining < buffer.length)
                read = source.read(buffer, 0, (int) remaining);
            else
                read = source.read(buffer, 0, buffer.length);

            remaining -= read;

            read = cipher.update(buffer, 0, read, bufferOut);
            destination.write(bufferOut, 0, read);

        }

        read = cipher.doFinal(bufferOut, 0);
        destination.write(bufferOut, 0, read);

    }

    private static long write(final InputStream data, final RandomAccessFile destination, final int bufferSize, final TaskProbe probe) throws Exception
    {

        final byte [] buffer = new byte[bufferSize];

        long total = 0;
        int read;
        while ((read = data.read(buffer)) > -1)
        {
            destination.write(buffer, 0, read);
            total += read;

            if (probe.isCancelRequested())
            {
                probe.fireCanceled();
                throw new CancellationException();
            }
        }

        return total;

    }

    private static void write(final RandomAccessFile source, final long length, final RandomAccessFile destination, final int bufferSize, final TaskProbe probe) throws Exception
    {

        final byte [] buffer = new byte[bufferSize];

        long remaining = length;
        int read;
        while (remaining > 0)
        {
            if (remaining < buffer.length)
                read = source.read(buffer, 0, (int) remaining);
            else
                read = source.read(buffer, 0, buffer.length);

            destination.write(buffer, 0, read);

            remaining -= read;

            if (probe.isCancelRequested())
            {
                probe.fireCanceled();
                throw new CancellationException();
            }
        }

    }

    /**
     * Read the header of the {@link Safe}
     * 
     * @param file:
     *            safe file to read
     * @param bufferSize:
     *            size of the <code>byte</code> buffer to be used in IO operation
     * @return
     * @throws IOException
     */
    public static Map<String, String> readHeader(final File file, final int bufferSize) throws IOException
    {
        RandomAccessFile raf = null;

        try
        {
            raf = new RandomAccessFile(file, "rw");
            final byte [] buffer = new byte[bufferSize];
            final ByteArrayOutputStream baos = new ByteArrayOutputStream(buffer.length);

            long length = raf.readLong();
            int read;
            while (length > 0)

            {

                if (length < buffer.length)
                    read = raf.read(buffer, 0, (int) length);
                else
                    read = raf.read(buffer);

                baos.write(buffer, 0, read);
                length -= read;

            }
            final String header = new String(baos.toByteArray(), UTF8);
            return GSON.fromJson(header, MAP_STRING_STRING_TYPE);
        } finally
        {
            if (raf != null)
                raf.close();
        }

    }

    /**
     * Create a new {@link Safe}
     * 
     * @param file
     * @param key
     * @param publicHeader
     * @param privateProperties
     * @param bufferSize
     * @return
     * @throws Exception
     */
    public static Safe create(final File file, final byte [] key, final Map<String, String> publicHeader, final Map<String, String> privateProperties, final int bufferSize) throws Exception
    {

        final String encryption = publicHeader.get(ENCRYPTION_LABEL);

        if (encryption == null)
            throw new Exception("Public property '" + ENCRYPTION_LABEL + "' must be set");

        Cipher cipher = javax.crypto.Cipher.getInstance(encryption);

        final String keyAlgo = publicHeader.get(KEY_ALGO_LABEL);
        if (keyAlgo == null)
            throw new Exception("Public property '" + KEY_ALGO_LABEL + "' must be set");

        final IvParameterSpec iv;

        if (publicHeader.containsKey(ENCRYPTION_IV_LABEL))
            iv = new IvParameterSpec(GSON.fromJson(publicHeader.get(ENCRYPTION_IV_LABEL), BYTE_ARRAY_TYPE));
        else
            iv = null;

        final SecretKeySpec keySpec = new SecretKeySpec(key, keyAlgo);

        cipher.init(Cipher.ENCRYPT_MODE, keySpec, iv);

        if (file.exists())
            throw new IOException("File " + file + " already exist");

        if (!file.createNewFile())
            throw new IOException("Could not create file " + file);

        final RandomAccessFile raf = new RandomAccessFile(file, "rw");

        long total, position, previousPosition;
        // header
        final String header = GSON.toJson(publicHeader) + "\n";

        previousPosition = raf.getFilePointer();
        raf.writeLong(0);
        total = write(new ByteArrayInputStream(header.getBytes(UTF8)), raf, bufferSize, TaskProbe.DULL_PROBE);
        position = raf.getFilePointer();
        raf.seek(previousPosition);
        raf.writeLong(total);
        raf.seek(position);

        // properties
        final String privatePropsJson = GSON.toJson(privateProperties == null ? new HashMap<>() : privateProperties);
        previousPosition = raf.getFilePointer();
        raf.writeLong(0L);
        total = encrypt(new ByteArrayInputStream(privatePropsJson.getBytes(UTF8)), cipher, raf, bufferSize, TaskProbe.DULL_PROBE);
        position = raf.getFilePointer();
        raf.seek(previousPosition);
        raf.writeLong(total);
        raf.seek(position);

        raf.close();

        return new Safe(file, cipher, keySpec, iv, bufferSize);

    }

}

org/ortis/jsafebox/Utils.java - Especially methods passwordToBytes, open and parseSystemPath

/*******************************************************************************
 * Copyright 2018 Ortis ([email protected])
 * 
 * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
 * 
 * http://www.apache.org/licenses/LICENSE-2.0
 * 
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 * License for the specific language governing permissions and limitations under the License.
 ******************************************************************************/

package org.ortis.jsafebox;

import java.awt.GraphicsDevice;
import java.awt.GraphicsEnvironment;
import java.awt.HeadlessException;
import java.io.File;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.nio.CharBuffer;
import java.nio.charset.Charset;
import java.nio.file.FileSystems;
import java.nio.file.FileVisitResult;
import java.nio.file.FileVisitor;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.PathMatcher;
import java.nio.file.Paths;
import java.nio.file.attribute.BasicFileAttributes;
import java.security.MessageDigest;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.logging.Logger;
import java.util.regex.Pattern;

import javax.crypto.Cipher;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.SecretKeySpec;

/**
 * Utility class
 * 
 * @author Ortis <br>
 *         2018 Apr 26 8:06:47 PM <br>
 */
public class Utils
{

    public final static String SEPARATOR_REGEX = "[/|" + Pattern.quote(java.io.File.separator) + "]";

    public static byte [] passwordToBytes(final char [] chars)
    {
        final CharBuffer charBuffer = CharBuffer.wrap(chars);
        final ByteBuffer byteBuffer = Charset.forName(Safe.UTF8).encode(charBuffer);
        final byte [] bytes = Arrays.copyOfRange(byteBuffer.array(), byteBuffer.position(), byteBuffer.limit());
        Arrays.fill(charBuffer.array(), '\u0000'); // clear sensitive data
        Arrays.fill(byteBuffer.array(), (byte) 0); // clear sensitive data
        return bytes;
    }

    /**
     * Open a {@link Safe}
     * 
     * @param safeFilePath:
     *            system path to the safe file
     * @param password:
     *            the encryption password
     * @param bufferSize:size
     *            of the <code>byte</code> buffer to be used in IO operation
     * @param log
     * @return
     * @throws Exception
     */
    public static Safe open(final String safeFilePath, final char [] password, final int bufferSize, final Logger log) throws Exception
    {
        final File file = new File(safeFilePath);

        if (!file.exists())
            throw new IOException("Safe file " + file + " doest not exist");

        final Map<String, String> header = Safe.readHeader(file, bufferSize);

        final String encyption = header.get(Safe.ENCRYPTION_LABEL);
        if (encyption == null)
            throw new Exception("Could not read property '" + Safe.ENCRYPTION_LABEL + "' from header");

        if (log != null)
            log.fine("Encryption type " + encyption);

        final Cipher cipher = javax.crypto.Cipher.getInstance(encyption);

        if (!header.containsKey(Safe.KEY_ALGO_LABEL))
            throw new Exception("Could not read property '" + Safe.KEY_ALGO_LABEL + "' from header");

        if (log != null)
            log.fine("Key algorithm " + header.get(Safe.KEY_ALGO_LABEL));

        final MessageDigest md = MessageDigest.getInstance("SHA-256");
        final byte [] key = Arrays.copyOf(md.digest(md.digest(Utils.passwordToBytes(password))), 128 >> 3);
        final SecretKeySpec keySpec = new SecretKeySpec(key, header.get(Safe.KEY_ALGO_LABEL));

        final IvParameterSpec iv;

        if (header.containsKey(Safe.ENCRYPTION_IV_LABEL))
            iv = new IvParameterSpec(Safe.GSON.fromJson(header.get(Safe.ENCRYPTION_IV_LABEL), Safe.BYTE_ARRAY_TYPE));
        else
            iv = null;

        return new Safe(file, cipher, keySpec, iv, bufferSize);
    }

    private final static String SYSTEM_PATH_DELIMITER_REGEX = Pattern.quote(File.separator) + "|" + Pattern.quote("/") + "|" + Pattern.quote("\\");

    public static List<java.io.File> parseSystemPath(String query, final List<java.io.File> destination) throws IOException
    {
        final String [] tokens = query.split(SYSTEM_PATH_DELIMITER_REGEX);

        Path baseDirectory = null;

        if (tokens[0].equals(".") || tokens[0].equals(".."))
        {
            baseDirectory = new File(tokens[0]).toPath();

            final StringBuilder sb = new StringBuilder();
            for (int i = 1; i < tokens.length; i++)
                if (sb.length() == 0)
                    sb.append(tokens[i]);
                else
                    sb.append(File.separator + tokens[i]);

            query = "**" + File.separator + sb.toString();

        } else
        {

            final String comparableToken = tokens[0].toUpperCase();
            for (final File root : File.listRoots())
                if (root.getAbsolutePath().toUpperCase().equals(comparableToken))
                {
                    // perfect match
                    baseDirectory = root.toPath();
                    break;

                }

            if (baseDirectory == null)
                for (final File root : File.listRoots())
                {
                    String rootPath = root.getAbsolutePath().toUpperCase();
                    rootPath = rootPath.substring(0, rootPath.length() - 1);
                    if (rootPath.equals(comparableToken))
                    {
                        baseDirectory = root.toPath();
                        break;
                    }

                }
        }

        if (baseDirectory == null)
            throw new IOException("Could not locate base directory '" + tokens[0] + "'");

        Path path = baseDirectory;
        for (int i = 1; i < tokens.length; i++)
        {

            try
            {
                path = Paths.get(path.toString(), tokens[i]);
            } catch (final Exception e)
            {
                // here, we have reach a special character and the start point for the search is
                // in path
            }
        }

        final String escapedQuery = query.replace("\\", "\\\\");// PathMatcher does not escape backslash properly. Need to do the escape manually for Windows OS path handling. This might be a bug of Java implentation.
        // Need to check on Oracle bug report database.

        final PathMatcher pathMatcher = FileSystems.getDefault().getPathMatcher("glob:" + escapedQuery);
        Files.walkFileTree(path, new FileVisitor<Path>()
        {

            @Override
            public FileVisitResult postVisitDirectory(final Path dir, final IOException exc) throws IOException
            {

                return FileVisitResult.CONTINUE;
            }

            @Override
            public FileVisitResult preVisitDirectory(final Path dir, final BasicFileAttributes attrs) throws IOException
            {
                if (pathMatcher.matches(dir))
                {
                    destination.add(dir.toFile());
                    return FileVisitResult.SKIP_SUBTREE;
                }

                return FileVisitResult.CONTINUE;
            }

            @Override
            public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) throws IOException
            {

                if (pathMatcher.matches(file))
                    destination.add(file.toFile());

                return FileVisitResult.CONTINUE;
            }

            @Override
            public FileVisitResult visitFileFailed(final Path file, final IOException exc) throws IOException
            {
                return FileVisitResult.CONTINUE;
            }
        });

        return destination;

    }

    /**
     * Return the MIME type of a file
     * 
     * @param file
     * @return
     */
    public static String getMIMEType(final java.io.File file)
    {

        final String name = file.getName().toUpperCase();

        if (name.endsWith(".TXT"))
            return "text/plain";
        else if (name.endsWith(".CSV"))
            return "text/csv";
        else if (name.endsWith(".HTM") || name.endsWith(".HTML"))
            return "text/html";
        else if (name.endsWith(".JPG") || name.endsWith(".JPEG"))
            return "image/jpg";
        else if (name.endsWith(".PNG"))
            return "image/png";
        else if (name.endsWith(".BM") || name.endsWith(".BMP"))
            return "image/bmp";
        else if (name.endsWith(".PDF"))
            return "application/pdf";
        else if (name.endsWith(".AVI"))
            return "video/x-msvideo";
        else if (name.endsWith(".MPEG"))
            return "video/mpeg";
        else if (name.endsWith(".MP4"))
            return "video/mp4";
        else if (name.endsWith(".MKV"))
            return "video/x-matroska";
        else if (name.endsWith(".MP3"))
            return "audio/mpeg";
        else
            return "application/octet-stream";
    }

    /**
     * Format the exception message
     * 
     * @param t
     * @return
     */
    public static String formatException(final Throwable t)
    {
        if (t == null)
            return null;

        final Throwable cause = t.getCause();
        final String msg = cause == null ? null : formatException(cause);
        return formatException(t.getClass(), msg, t.toString(), t.getStackTrace());

    }

    private static String formatException(final Class<?> exceptionClass, final String cause, final String msg, final StackTraceElement [] exceptionStack)
    {
        final StringBuilder builder = new StringBuilder();

        if (msg != null)
            builder.append(msg);

        if (exceptionStack != null)
        {
            builder.append(System.lineSeparator());
            for (int i = 0; i < exceptionStack.length; i++)
            {
                final String stackElement = exceptionStack[i].toString();

                builder.append(stackElement + System.lineSeparator());
            }
        }

        if (cause != null)
            builder.append("Caused by " + cause);

        return builder.toString();
    }

    /**
     * Remove forbidden <code>char</code> from the path and replace them with <code>substitute</code>
     * 
     * @param path:
     *            the path to sanitize
     * @param delimiter:
     *            delimiter of the path
     * @param substitute:
     *            replacement char
     * @return
     */
    public static String sanitize(final String path, final Character delimiter, final Character substitute)
    {
        final String [] tokens = path.split(Pattern.quote(Character.toString(delimiter)));

        final StringBuilder sb = new StringBuilder();
        for (int i = 0; i < tokens.length; i++)
        {
            if (i < tokens.length - 1)
                sb.append(sanitizeToken(tokens[i], substitute) + delimiter);
            else
                sb.append(sanitizeToken(tokens[i], substitute));

        }

        return sb.toString();
    }

    public static String sanitizeToken(final String token, final Character substitute)
    {

        final StringBuilder sb = new StringBuilder(token);

        final Character replacement = substitute;

        c: for (int i = 0; i < sb.length(); i++)
        {

            if (sb.charAt(i) == java.io.File.separatorChar || sb.charAt(i) == Folder.DELIMITER)
            {
                if (replacement == null)
                    sb.deleteCharAt(i--);
                else
                    sb.setCharAt(i, replacement);
                continue c;
            }

            for (final char c : Environment.getForbidenChars())
                if (sb.charAt(i) == c)
                {
                    if (replacement == null)
                        sb.deleteCharAt(i--);
                    else
                        sb.setCharAt(i, replacement);
                    continue c;
                }
        }
        return sb.toString();

    }

    public static boolean isHeadless()
    {
        if (GraphicsEnvironment.isHeadless())
            return true;

        try
        {
            GraphicsDevice [] screenDevices = GraphicsEnvironment.getLocalGraphicsEnvironment().getScreenDevices();
            return screenDevices == null || screenDevices.length == 0;
        } catch (HeadlessException e)
        {
            return true;
        }
    }

}

I will gladly take any feedback from other classes as well :)

EDIT:

An new post have been made for the latest version of the project.

Several point have been raised:

@Maarten Bodewes

Design:

The Safe class has too many fields and too many imports -> Safe is the core logic class. The lastest version has 32 imports and 15 field which does not sound excessive

The header part could be factored out into a Header class -> The header is just a specific array of byte in the Safe's file

Calling the add method with properties as parameter requires you to check all the properties within the method; it should be given e.g. an interface with getters instead. -> properties is a set of custom entries provided y the user. Only a couple are mandatory and must be checked

It seems that too many parameters simply contain of strictly formatted strings -> These are user inputs

The add method doesn't actually add anything; it just creates a block it seems. -> Add is writing a block on the temp file

Your class doesn't seem re-entrant. -> Fixed. Safe is now thread safe Exposing the internals through getters is a very bad idea; -> JSafebox could be embeded in another application as a dependency. We must provide control over the class

Methods such as open are specific to class Safe and should therefore not be in a Util class (do not create circular dependencies!). -> There is no circular dependency: Utils is using Safe

Java-specific:

StandardCharsets.UTF_8, no need to define your own -> Fixed

Exception handling: don't just throw Exception but handle all exceptions locally, except possibly those that depend on the input -> All exception would need to be reported tothe user. Specific handling would require a lots of code for little benefit

All your constants are public. Only expose those constants that are specific to your class and useful to the outside. -> Safe is the core logic class and contains may constant that are used in toher classes

You need inline comments in long methods such as add to at least indicate what you are trying to do. -> Fixed

Don't leave commented out code without TODO: task or similar and always explain why it is commented out. -> Fixed

Use Java New IO (NIO) and ByteBuffer's instead of RandomAccessFile and byte arrays. -> RandomAccessFile is a duplex IO. There is no equivalent in the NIO package

Security & Cryptography

The header doesn't seem to be authenticated or encrypted, leaking info and allowing alteration. -> Fixed

passwordToBytes should be using a password hash such as PBKDF2, scrypt or a secure variant of Argon2. -> Fixed

It doesn't make too much sense to pass a specific Cipher and AlgorithmParameterSpec; better just use a protocol version and use a specific cipher / parameters. -> Fixed

SecretKeySpec parameter should just be SecretKey. -> Fixed

There doesn't seem to be any integrity protection (although using GCM as cipher may be possible). -> Fixed

Each block should use a different IV, so using a statically set IV will break the confidentiality of your plaintext message (i.e. the stored files). -> Fixed

The cipher class should always be local to a method; you don't want to share the cipher state over different public methods. -> Fixed

@TwiN

StringBuilder concat -> Fixed

Use switch in getMIMEType() -> Fixed

\$\endgroup\$
2
  • \$\begingroup\$ What is your use case (the goal of the application) and what is the threat model (the perceived threats that you are trying to protect against)? \$\endgroup\$ May 28, 2018 at 12:53
  • \$\begingroup\$ @MaartenBodewes Use case: 1) easy access to sensitive data like ID scan, financial report, password, crypto wallet key 2) securely store data on the cloud without having to trust third party . I am not sure about the threat but I think the main one would be would be unauthorized access to the JSafebox file containing encrypted data. \$\endgroup\$
    – ortis
    May 28, 2018 at 13:10

2 Answers 2

4
\$\begingroup\$

Disclaimer: I'm focusing here on the things that can be improved. There are certainly good things about your code, e.g. the naming of identifiers is generally good.

Design:

  1. The Safe class has too many fields; either split it up into multiple classes or you should keep your variables local. Decide how much state your class really requires and keep the number of fields to a minimum.
  2. The Safe class has too many imports, which means you should split up your class, e.g. using implementation classes for specific functionality.
  3. The header part could be factored out into a Header class.
  4. Cancelation is allowed for the header and before writing the blocks, but the operation that takes most of the time / resources apparently cannot be cancelled.
  5. Calling the add method with properties as parameter requires you to check all the properties within the method; it should be given e.g. an interface with getters instead.
  6. It seems that too many parameters simply contain of strictly formatted strings, do not use Stringly Typed code: convert to a parameter class or simply use multiple parameters. If you use a string parameter with structured content, then at least create a method to validate it rather than validating it inline.
  7. The add method doesn't actually add anything; it just creates a block it seems. The only side effect is that to cipher, and that's one that should not be altered.
  8. Your class doesn't seem re-entrant. Having a single object be able to be used by multiple threads seems a good idea.
  9. Exposing the internals through getters is a very bad idea; implementation specific details should be hidden.
  10. Methods such as open are specific to class Safe and should therefore not be in a Util class (do not create circular dependencies!).
  11. Similarly, isHeadless contains code that is package specific and should therefore probably be in another class / location. Don't overuse a class like Util; just use private (static) methods instead if it is specific to a class.

Java-specific:

  1. StandardCharsets.UTF_8, no need to define your own (note also that using that enum within String doesn't generate an exception).
  2. Exception handling: don't just throw Exception but handle all exceptions locally, except possibly those that depend on the input. Use RuntimeException or one of its derived classes to handle code specific exceptions. Do not use Pokémon Exception Handling.
  3. All your constants are public. Only expose those constants that are specific to your class and useful to the outside.
  4. Using if and else without braces is considered bad coding practice.
  5. You need inline comments in long methods such as add to at least indicate what you are trying to do.
  6. if (remaining < buffer.length) use Math.min instead.
  7. Do not re-purpose local variables such as read to mean message length in decrypt() function.
  8. Don't leave commented out code without TODO: task or similar and always explain why it is commented out.
  9. Use Java New IO (NIO) and ByteBuffer's instead of RandomAccessFile and byte arrays.

Security & Cryptography

  1. The header doesn't seem to be authenticated or encrypted, leaking info and allowing alteration.
  2. passwordToBytes should be using a password hash such as PBKDF2, scrypt or a secure variant of Argon2.
  3. It doesn't make too much sense to pass a specific Cipher and AlgorithmParameterSpec; better just use a protocol version and use a specific cipher / parameters.
  4. SecretKeySpec parameter should just be SecretKey.
  5. There doesn't seem to be any integrity protection (although using GCM as cipher may be possible).
  6. Each block should use a different IV, so using a statically set IV will break the confidentiality of your plaintext message (i.e. the stored files).
  7. The cipher class should always be local to a method; you don't want to share the cipher state over different public methods.

Running your class through a static code analyzer or two (CheckStyle & FindBugs, to name just two) seems to be a great idea, just like using a code beautifier.


Example use of configuration through an interface:

interface SafeConfig {
    int getProperty1();
    Optional<String> getProperty2();
    SafeType getProperty3();
    // etc.
}

This lets you easily create a parser that returns an implementation of the interface. But if you just need to test your code you can simply create a direct implementation of the interface, if required using an internal or anonymous class.

\$\endgroup\$
6
  • \$\begingroup\$ Beware that I had some issues when it comes to moving a file from temp to another file system using Java. You may want to test that functionality; I'm not sure what the current state of Java runtimes is at this moment regarding that. \$\endgroup\$ May 28, 2018 at 19:37
  • \$\begingroup\$ Thanks for the feebacks, I will update the code - Regarding security point (1) : the header is in clear text to provide a manual for decrytping. I will add a checksum of the file in the private property to check for file tempering. Regarding Desgin point (4): which part cannot be canceled ? \$\endgroup\$
    – ortis
    May 29, 2018 at 0:48
  • \$\begingroup\$ I don't see any cancellation option within the for loops in the save method, and the loops are generally the places where most of the work takes place. \$\endgroup\$ May 29, 2018 at 1:13
  • \$\begingroup\$ Cancel request are check in method write \$\endgroup\$
    – ortis
    May 29, 2018 at 1:50
  • \$\begingroup\$ Ah, great, I'll remove that later on. You may want to document that part - although you do pass the probe so I should have figured it out myself, I give you that :) G'night! \$\endgroup\$ May 29, 2018 at 2:09
2
\$\begingroup\$

Maarten Bodewes has said a lot, so instead, I'll focus on smaller pieces.

The method sanitize uses a StringBuilder, which is great, but you're concatenating two strings inside the StringBuilder append method, which ruins the point of using a StringBuilder:

sb.append(sanitizeToken(tokens[i], substitute) + delimiter);

should be

sb.append(sanitizeToken(tokens[i], substitute)).append(delimiter);

getMIMEType() should use a switch rather than multiple if statements.


The following piece of code:

Path path = baseDirectory;
for (int i = 1; i < tokens.length; i++)
{

    try
    {
        path = Paths.get(path.toString(), tokens[i]);
    } catch (final Exception e)
    {
        // here, we have reach a special character and the start point for the search is
        // in path
    }
}

would look much better if:

  • The useless newline was removed
  • The bracket styling was consistent
  • The comments did not contain useless words (which would also allow you to fit it in one line, assuming you have a certain column length limit you're following)
  • There is no advantage to marking the exception as final when catching it (no disadvantage either, afaik)

e.g.

Path path = baseDirectory;
for (int i = 1; i < tokens.length; i++) {
    try {
        path = Paths.get(path.toString(), tokens[i]);
    } catch (Exception e) {
        // Special character reached + start point for the search is in path
    }
}

Furthermore, an empty catch block is frowned upon, you should do something with it.


I like to think that one of the most important aspect when trying to write clean code is consistency.

If your code is consistent, even if other developers use different conventions, they can get used to it fairly easy because it is consistent.

In your case, you have some exceptions that are final, some that aren't.

In some places, you leave an extra blank line before of after a block of code, but not everywhere.

e.g.:

   });

    return destination;

}
\$\endgroup\$
1
  • \$\begingroup\$ I appreciate the feedbacks - I definitely missed that StringBuilder concat. Regarding the silent exception: this is the only reliable way for path building I have found so far. But yes, it is ugly and need to be improved. \$\endgroup\$
    – ortis
    May 29, 2018 at 1:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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