Module architecture refactoring

[stanbar]

During PIN Module implementation #32 I was confused with the pattern chosen by @pfoof .
Could you please explain what is the reason for using hook method for encryption
public byte[] encryptInput(byte[] data) and functional interface for decryption

protected interface Decrypter {
    public byte[] decrypt(byte[] payload) throws DecryptionException;
}

It looks like your idea is not finished because

protected interface Encrypter {
    public byte[] encrypt(byte[] data);
}

is unused.
Both patterns are good, but they shouldn't be mixed in symetric operations.

Additionally

/**
 * used for decryption, should include `this.decrypt(Decrypter)`
 */
public abstract void process();

is very error-prone, since the decrypt(Decrypter) MUST be called in order to generate decryptedValue. The architecture must force user to implement methods that are required to correctly working system.

And here I faced with another consideration, do we need to keep decryptedValue in memory ? Can't we just calculate this value on-demand ? For example when all modules are check() == true then manager would call encrypt(keyPart) on every module, combine the parts and restore seed. No memory clearing would be necessary.

I'm not forcing this change, I just want to make our code consistent

Let's debate !

Took 29 minutes

[stanbar]

ExampleModule before refactoring

public class ExampleModule extends Module {

    @Override
    public String getDescription() {
        return "An example waiting and xoring module to show how things work.";
    }

    public static final byte[] KEY = "EXAMPLEKEY".getBytes();

    private long lastTime = 1000;

    @Override
    public void register() {
        lastTime = System.currentTimeMillis();
        setStatusString("Wait 5 seconds for decryption to start");
    }

    @Override
    public String getHtmlUi() {
        return null;
    }

    @Override
    public boolean check() {
        return System.currentTimeMillis() - lastTime > 5000;
    }

    @Override
    public byte[] encryptInput(byte[] data) {
        byte[] r = data.clone();
        for (int i = 0; i < r.length; ++i)
            r[i] = (byte) (r[i] ^ KEY[i % KEY.length]);
        return r;
    }

    @Override
    public void process() {
        decrypt(new Decrypter() {
            @Override
            public synchronized byte[] decrypt(byte[] payload) throws DecryptionException {
                if (payload == null) throw new Module.DecryptionException(Module.DecryptionException.NO_DATA);
                byte[] r = payload.clone();
                for (int i = 0; i < r.length; ++i)
                    r[i] = (byte) (r[i] ^ KEY[i % KEY.length]);
                return r;
            }
        });
    }
}

after refactoring

public class ExampleModule extends Module {

    private static final byte[] KEY = "EXAMPLEKEY".getBytes();

    private long lastTime = 1000;

    @Override
    public void register() {
        lastTime = System.currentTimeMillis();
        setStatusString("Wait 5 seconds for decryption to start");
    }
    
    @Override
    public String getDescription() {
        return "An example waiting and xoring module to show how things work.";
    }

    @Override
    public String getHtmlUi() {
        return null;
    }

    @Override
    public boolean check() {
        return System.currentTimeMillis() - lastTime > 5000;
    }

    @Override
    public byte[] encrypt(byte[] data) {
        byte[] r = data.clone();
        for (int i = 0; i < r.length; ++i)
            r[i] = (byte) (r[i] ^ KEY[i % KEY.length]);
        return r;
    }

    @Override
    public byte[] decrypt(byte[] payload) throws DecryptionException {
        if (payload == null) throw new Module.DecryptionException(Module.DecryptionException.NO_DATA);
        byte[] r = payload.clone();
        for (int i = 0; i < r.length; ++i)
            r[i] = (byte) (r[i] ^ KEY[i % KEY.length]);
        return r;
    }
}

[pfoof]

decrypt(<anonymous class>) is safer from being debugged/disassembled. Also synchronized is important. I left out encrypt as normal function because I didn't feel encryption of data is interesting for hackers to target.

As about zeroFill, even if you decrypt in manager on the go, it's even less safe because registers or stack can have the values left before gc activates and then you have no access to even zero them.

We might ask about above two our advisor.

params for encrypt/decrypt are not the same as input from the frontend.

[stanbar]

decrypt() is safer from being debugged/disassembled

I don't know how you connect anonymous classes with security. Could you please provide some proofs ?

Also synchronized is important.

Thats why I left the synchronized modifier, but on the caller side private synchronized void process(byte[] payload) which turns out to be the only caller for this method.

registers or stack can have the values left before gc activates and then you have no access to even zero them.

I don't get it, GC won't clear data until you keep reference to it. And I don't see any reason to lose reference to this data before clearing it manually.

params for encrypt/decrypt are not the same as input from the frontend.

Ok, so could you please describe what they are, and where may be required ? And why can't be stored as class memebers ?

[pfoof]

1, 3. We might ask about above two our advisor.
3.

class X {public int a;}
class Main {
    X x1 = new X(); //Memory: [&x1 <0x1234>, 0]
    x1.a = 100; //Memory: [&x1 <0x1234>, 100]
    x1 = new X(); //Memory: [&x1 <0x2345>, 100, 0]
    x1.a = 200; //Memory: [&x1 <0x2345>, 100, 200], meltdown/spectre anyone?
    x1 = null; //Memory: [&x1 <0x0000>, 100, 200]
    System.gc(); //Memory: [&x1 <0x0000>, 0, 200], tried my best!
}

4. Dunno, like a private key or two, once rolled random number during installation. It's for developers after all, so they can use it. Both database and user input can provide params., low importance, can be dropped actually

[stanbar]

class X {public int a;}
class Main {
X x1 = new X(); //Memory: [&x1 <0x1234>, 0]
x1.a = 100; //Memory: [&x1 <0x1234>, 100]
x1 = new X(); //Memory: [&x1 <0x2345>, 100, 0]
x1.a = 200; //Memory: [&x1 <0x2345>, 100, 200], meltdown/spectre anyone?
x1 = null; //Memory: [&x1 <0x0000>, 100, 200]
System.gc(); //Memory: [&x1 <0x0000>, 0, 200], tried my best!
}

Strange, I didn't know that. Anyway, I don't see this problem in our case. What I thought was something like that:

val encryptedKeyParts : Map<String, byte[]> = getKeyPartsFromDb() // module.id -> encryptedKeyPart
val modules = listOf(PinModule(), ButtonModule(), ServerModule())
var decryptedKeys = modules.map{ module -> module.decrypt(encryptedKeyParts[module.id]) }
var privateKey = shamir.restoreSecretFromKeyParts(decryptedKeys)
bitcoin.restoreWallet(privateKey)
decryptedKeys = null
privateKey = null
System.gc()

But as I said, I don't push on this change, it was just proposal. I care about the others changes. Do you think that we can merge it ?

[PatrykMilewski]

I think that we should stay away from things like trying to improve security by adding annonymous classes and other tricks. Let's keep code clean and understandable, it's more important.