HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Securing a file encryption(ECIES) protocol using Scrypt + Inferno

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
encryptioneciesfileprotocolsecuringinfernoscryptusing

Problem

I'm evaluating the security level of a file encryption app, which implements Scrypt and Inferno, in an ECIES context. The encrypted files are either stored on the user's PC/laptop, either sent with gmail. I must reach a security level appropriate for files containing sensitive data like credit card number, medical infos etc... Nowadays about any hacker can intercept an email, and a laptop can be stolen or seized, so I'm assuming that about anyone could have physical access to the encrypted file(s).

The workflow is: (assuming pre-distribution of public keys)

  • start app on user PC/laptop(offline, no server).



  • create/load master key pair to/from file + encrypt/decrypt using hashed password.



  • create/load a file to work on.



  • ETM with receiver's public key.



  • send as gmail attachment(or store on PC if receiver is sender)



The current implementation works well on the surface, but in the given context, what should I improve/change for even better security?

DHM key storage/retrieval:

```
const int _saltSize = 48; // 16 is often recommended, better safe than sorry
const int _keySize = 48;
const int _iterations = 1048576; // maybe randomize + store?

private static byte[] Hash(string password, int iterations, byte[] salt = null)
{
var pass = password.ToBytes();
var saltAndHash = new byte[0];

if (salt == null) // if encrypting, store salt
{
salt = new CryptoRandom().NextBytes(_saltSize);
saltAndHash = Utils.Combine(saltAndHash, salt);
}

int blockSize = 8; // maybe 16 with less iterations considering modern GPUs?
int parallel = 1;
return Utils.Combine(saltAndHash, SCrypt.ComputeDerivedKey(pass, salt, iterations, blockSize, parallel, null, _keySize));
}

internal static void SaveMasterKey(Keyring k, string password, string file)
{
// Keyring is simply holding the session keys in CngKey props

var hashed = Hash(pas

Solution

The biggest issues I see are some conformity/readability issues.

For example, the following if/else structure:

if (forSender) ephemeralBundle = k.SenderDHM.GetSharedEphemeralDhmSecret();
else ephemeralBundle = k.ReceiverDHM.GetSharedEphemeralDhmSecret();


Please don't do that. It makes it significantly harder to follow. If you want to omit braces I understand but don't inline them like that.

var ephemeralPublic = new byte[104];

using (var fs = new FileStream(file, FileMode.Open, FileAccess.Read))
{                
    fs.Read(ephemeralPublic, 0, 104);
    var ephemeralSymmetric = k.SenderDHM.GetSharedDhmSecret(ephemeralPublic.ToPublicKeyFromBlob());
    if (Authenticate(file, ephemeralSymmetric, 104))
    {                    
        using (var etm = new EtM_DecryptTransform(ephemeralSymmetric))
        using (var cs = new CryptoStream(fs, etm, CryptoStreamMode.Read))
        {
            var decrypt = new byte[fs.Length - 104];


I don't know what 104 means, but you use it a lot and now to me it's very magic. Try to get it in a const that reflects the meaning of it.

Code Snippets

if (forSender) ephemeralBundle = k.SenderDHM.GetSharedEphemeralDhmSecret();
else ephemeralBundle = k.ReceiverDHM.GetSharedEphemeralDhmSecret();
var ephemeralPublic = new byte[104];

using (var fs = new FileStream(file, FileMode.Open, FileAccess.Read))
{                
    fs.Read(ephemeralPublic, 0, 104);
    var ephemeralSymmetric = k.SenderDHM.GetSharedDhmSecret(ephemeralPublic.ToPublicKeyFromBlob());
    if (Authenticate(file, ephemeralSymmetric, 104))
    {                    
        using (var etm = new EtM_DecryptTransform(ephemeralSymmetric))
        using (var cs = new CryptoStream(fs, etm, CryptoStreamMode.Read))
        {
            var decrypt = new byte[fs.Length - 104];

Context

StackExchange Code Review Q#149451, answer score: 8

Revisions (0)

No revisions yet.