From 25065a019858e150594200785265e2f1985a52ca Mon Sep 17 00:00:00 2001 From: Rauno Ots Date: Wed, 7 Oct 2020 08:39:16 +0200 Subject: [PATCH] Config: do to not store decryption variables globally (#571) * Config: do to not store decryption variables globally * save session variables as unexported fields in config * unexport internal encryption helper functions * add encryption/decryption tests * fix yes/no prompt returns flipped boolean * add a test to ensure prompting for encryption works * fix possible NPE when failing * don't return salt if there's error --- config/config.go | 42 ++++--- config/config_encryption.go | 74 +++++------ config/config_encryption_test.go | 206 +++++++++++++++++++++++++++++-- config/config_types.go | 8 +- 4 files changed, 265 insertions(+), 65 deletions(-) diff --git a/config/config.go b/config/config.go index e7fcb769..97c358f3 100644 --- a/config/config.go +++ b/config/config.go @@ -1577,6 +1577,7 @@ func GetFilePath(configfile string) (string, error) { // ReadConfig verifies and checks for encryption and verifies the unencrypted // file contains JSON. +// Prompts for decryption key, if target file is encrypted func (c *Config) ReadConfig(configPath string, dryrun bool) error { defaultPath, err := GetFilePath(configPath) if err != nil { @@ -1599,12 +1600,18 @@ func (c *Config) ReadConfig(configPath string, dryrun bool) error { } if c.EncryptConfig == fileEncryptionPrompt { - m.Lock() - IsInitialSetup = true - m.Unlock() - if c.PromptForConfigEncryption(configPath, dryrun) { - c.EncryptConfig = fileEncryptionEnabled - return c.SaveConfig(defaultPath, dryrun) + confirm, err := promptForConfigEncryption() + if err == nil { + if confirm { + c.EncryptConfig = fileEncryptionEnabled + return c.SaveConfig(defaultPath, dryrun) + } + + c.EncryptConfig = fileEncryptionDisabled + err := c.SaveConfig(configPath, dryrun) + if err != nil { + log.Errorf(log.ConfigMgr, "Cannot save config. Error: %s\n", err) + } } } return nil @@ -1615,7 +1622,7 @@ func (c *Config) ReadConfig(configPath string, dryrun bool) error { if errCounter >= maxAuthFailures { return errors.New("failed to decrypt config after 3 attempts") } - key, err := PromptForConfigKey(IsInitialSetup) + key, err := PromptForConfigKey(false) if err != nil { log.Errorf(log.ConfigMgr, "PromptForConfigKey err: %s", err) errCounter++ @@ -1624,9 +1631,9 @@ func (c *Config) ReadConfig(configPath string, dryrun bool) error { var f []byte f = append(f, fileData...) - data, err := DecryptConfigFile(f, key) + data, err := c.decryptConfigData(f, key) if err != nil { - log.Errorf(log.ConfigMgr, "DecryptConfigFile err: %s", err) + log.Errorf(log.ConfigMgr, "decryptConfigData err: %s", err) errCounter++ continue } @@ -1645,6 +1652,7 @@ func (c *Config) ReadConfig(configPath string, dryrun bool) error { } // SaveConfig saves your configuration to your desired path +// prompts for encryption key, if necessary func (c *Config) SaveConfig(configPath string, dryrun bool) error { if dryrun { return nil @@ -1661,17 +1669,21 @@ func (c *Config) SaveConfig(configPath string, dryrun bool) error { } if c.EncryptConfig == fileEncryptionEnabled { - var key []byte - - if IsInitialSetup { + // Ensure we have the key from session or from user + if len(c.sessionDK) == 0 { + var key []byte key, err = PromptForConfigKey(true) if err != nil { return err } - IsInitialSetup = false + var sessionDK, storedSalt []byte + sessionDK, storedSalt, err = makeNewSessionDK(key) + if err != nil { + return err + } + c.sessionDK, c.storedSalt = sessionDK, storedSalt } - - payload, err = EncryptConfigFile(payload, key) + payload, err = c.encryptConfigFile(payload) if err != nil { return err } diff --git a/config/config_encryption.go b/config/config_encryption.go index c2ba139a..d84d8ba2 100644 --- a/config/config_encryption.go +++ b/config/config_encryption.go @@ -27,33 +27,22 @@ const ( errAESBlockSize = "config file data is too small for the AES required block size" ) -var ( - storedSalt []byte - sessionDK []byte -) - -// PromptForConfigEncryption asks for encryption key -func (c *Config) PromptForConfigEncryption(configPath string, dryrun bool) bool { +// promptForConfigEncryption asks for encryption confirmation +// returns true if encryption was desired, false otherwise +func promptForConfigEncryption() (bool, error) { log.Println("Would you like to encrypt your config file (y/n)?") input := "" _, err := fmt.Scanln(&input) if err != nil { - return false + return false, err } - if !common.YesOrNo(input) { - c.EncryptConfig = fileEncryptionDisabled - err := c.SaveConfig(configPath, dryrun) - if err != nil { - log.Printf("Cannot save config. Error: %s\n", err) - } - return false - } - return true + return common.YesOrNo(input), nil } // PromptForConfigKey asks for configuration key +// if initialSetup is true, the password needs to be repeated func PromptForConfigKey(initialSetup bool) ([]byte, error) { var cryptoKey []byte @@ -94,16 +83,21 @@ func PromptForConfigKey(initialSetup bool) ([]byte, error) { // EncryptConfigFile encrypts configuration data that is parsed in with a key // and returns it as a byte array with an error func EncryptConfigFile(configData, key []byte) ([]byte, error) { - var err error - - if len(sessionDK) == 0 { - sessionDK, err = makeNewSessionDK(key) - if err != nil { - return nil, err - } + sessionDK, salt, err := makeNewSessionDK(key) + if err != nil { + return nil, err } + c := &Config{ + sessionDK: sessionDK, + storedSalt: salt, + } + return c.encryptConfigFile(configData) +} - block, err := aes.NewCipher(sessionDK) +// EncryptConfigFile encrypts configuration data that is parsed in with a key +// and returns it as a byte array with an error +func (c *Config) encryptConfigFile(configData []byte) ([]byte, error) { + block, err := aes.NewCipher(c.sessionDK) if err != nil { return nil, err } @@ -118,15 +112,21 @@ func EncryptConfigFile(configData, key []byte) ([]byte, error) { stream.XORKeyStream(ciphertext[aes.BlockSize:], configData) appendedFile := []byte(EncryptConfirmString) - appendedFile = append(appendedFile, storedSalt...) + appendedFile = append(appendedFile, c.storedSalt...) appendedFile = append(appendedFile, ciphertext...) return appendedFile, nil } // DecryptConfigFile decrypts configuration data with the supplied key and -// returns the un-encrypted file as a byte array with an error +// returns the un-encrypted data as a byte array with an error func DecryptConfigFile(configData, key []byte) ([]byte, error) { - configData = RemoveECS(configData) + return (&Config{}).decryptConfigData(configData, key) +} + +// decryptConfigData decrypts configuration data with the supplied key and +// returns the un-encrypted data as a byte array with an error +func (c *Config) decryptConfigData(configData, key []byte) ([]byte, error) { + configData = removeECS(configData) origKey := key if ConfirmSalt(configData) { @@ -158,10 +158,11 @@ func DecryptConfigFile(configData, key []byte) ([]byte, error) { stream.XORKeyStream(configData, configData) result := configData - sessionDK, err = makeNewSessionDK(origKey) + sessionDK, storedSalt, err := makeNewSessionDK(origKey) if err != nil { return nil, err } + c.sessionDK, c.storedSalt = sessionDK, storedSalt return result, nil } @@ -176,8 +177,8 @@ func ConfirmECS(file []byte) bool { return bytes.Contains(file, []byte(EncryptConfirmString)) } -// RemoveECS removes encryption confirmation string -func RemoveECS(file []byte) []byte { +// removeECS removes encryption confirmation string +func removeECS(file []byte) []byte { return bytes.Trim(file, EncryptConfirmString) } @@ -188,17 +189,16 @@ func getScryptDK(key, salt []byte) ([]byte, error) { return scrypt.Key(key, salt, 32768, 8, 1, 32) } -func makeNewSessionDK(key []byte) ([]byte, error) { - var err error +func makeNewSessionDK(key []byte) (dk, storedSalt []byte, err error) { storedSalt, err = crypto.GetRandomSalt([]byte(SaltPrefix), SaltRandomLength) if err != nil { - return nil, err + return nil, nil, err } - dk, err := getScryptDK(key, storedSalt) + dk, err = getScryptDK(key, storedSalt) if err != nil { - return nil, err + return nil, nil, err } - return dk, nil + return dk, storedSalt, nil } diff --git a/config/config_encryption_test.go b/config/config_encryption_test.go index 23ceef30..3b807555 100644 --- a/config/config_encryption_test.go +++ b/config/config_encryption_test.go @@ -1,14 +1,22 @@ package config import ( + "bytes" + "io/ioutil" + "os" + "path/filepath" "testing" ) func TestPromptForConfigEncryption(t *testing.T) { t.Parallel() - if Cfg.PromptForConfigEncryption("", true) { - t.Error("PromptForConfigEncryption return incorrect bool") + confirm, err := promptForConfigEncryption() + if confirm { + t.Error("promptForConfigEncryption return incorrect bool") + } + if err == nil { + t.Error("Expected error as there is no input") } } @@ -32,26 +40,30 @@ func TestEncryptConfigFile(t *testing.T) { t.Fatal("Expected error") } - sessionDK = []byte("a") - _, err = EncryptConfigFile([]byte("test"), nil) + c := &Config{ + sessionDK: []byte("a"), + } + _, err = c.encryptConfigFile([]byte("test")) if err == nil { t.Fatal("Expected error") } - sessionDK, err = makeNewSessionDK([]byte("asdf")) + sessDk, salt, err := makeNewSessionDK([]byte("asdf")) if err != nil { t.Fatal(err) } - _, err = EncryptConfigFile([]byte("test"), []byte("key")) + c = &Config{ + sessionDK: sessDk, + storedSalt: salt, + } + _, err = c.encryptConfigFile([]byte("test")) if err != nil { t.Fatal(err) } } func TestDecryptConfigFile(t *testing.T) { - sessionDK = nil - result, err := EncryptConfigFile([]byte("test"), []byte("key")) if err != nil { t.Fatal(err) @@ -96,7 +108,7 @@ func TestRemoveECS(t *testing.T) { t.Parallel() ECStest := []byte(EncryptConfirmString) - isremoved := RemoveECS(ECStest) + isremoved := removeECS(ECStest) if string(isremoved) != "" { t.Errorf("TestConfirmECS: Error ECS not deleted.") @@ -106,8 +118,182 @@ func TestRemoveECS(t *testing.T) { func TestMakeNewSessionDK(t *testing.T) { t.Parallel() - _, err := makeNewSessionDK(nil) + _, _, err := makeNewSessionDK(nil) if err == nil { t.Fatal("makeNewSessionDK passed with nil key") } } + +func TestEncryptTwiceReusesSaltButNewCipher(t *testing.T) { + c := &Config{} + c.EncryptConfig = 1 + tempDir, err := ioutil.TempDir("", "") + if err != nil { + t.Fatalf("Problem creating temp dir at %s: %s\n", tempDir, err) + } + defer os.RemoveAll(tempDir) + + // Prepare input + passFile, err := ioutil.TempFile(tempDir, "*.pw") + if err != nil { + t.Fatalf("Problem creating temp file at %s: %s\n", tempDir, err) + } + passFile.WriteString("pass\npass\n") + passFile.Close() + + // Temporarily replace Stdin with a custom input + oldIn := os.Stdin + defer func() { os.Stdin = oldIn }() + os.Stdin, err = os.Open(passFile.Name()) + if err != nil { + t.Fatalf("Problem opening temp file at %s: %s\n", passFile.Name(), err) + } + + // Save encrypted config + enc1 := filepath.Join(tempDir, "encrypted.dat") + err = c.SaveConfig(enc1, false) + if err != nil { + t.Fatalf("Problem storing config in file %s: %s\n", enc1, err) + } + // Save again + enc2 := filepath.Join(tempDir, "encrypted2.dat") + err = c.SaveConfig(enc2, false) + if err != nil { + t.Fatalf("Problem storing config in file %s: %s\n", enc2, err) + } + data1, err := ioutil.ReadFile(enc1) + if err != nil { + t.Fatalf("Problem reading file %s: %s\n", enc1, err) + } + data2, err := ioutil.ReadFile(enc2) + if err != nil { + t.Fatalf("Problem reading file %s: %s\n", enc2, err) + } + // legth of prefix + salt + l := len(EncryptConfirmString+SaltPrefix) + SaltRandomLength + // Even though prefix, including salt with the random bytes is the same + if !bytes.Equal(data1[:l], data2[:l]) { + t.Error("Salt is not reused.") + } + // the cipher text should not be + if bytes.Equal(data1, data2) { + t.Error("Encryption key must have been reused as cipher texts are the same") + } +} + +func TestSaveAndReopenEncryptedConfig(t *testing.T) { + c := &Config{} + c.Name = "myCustomName" + c.EncryptConfig = 1 + tempDir, err := ioutil.TempDir("", "") + if err != nil { + t.Fatalf("Problem creating temp dir at %s: %s\n", tempDir, err) + } + defer os.RemoveAll(tempDir) + + // Prepare password + passFile, err := ioutil.TempFile(tempDir, "*.pw") + if err != nil { + t.Fatalf("Problem creating temp file at %s: %s\n", tempDir, err) + } + passFile.WriteString("pass\npass\n") + passFile.Close() + + // Temporarily replace Stdin with a custom input + cleanup := setAnswersFile(t, passFile.Name()) + defer cleanup() + + // Save encrypted config + enc := filepath.Join(tempDir, "encrypted.dat") + err = c.SaveConfig(enc, false) + if err != nil { + t.Fatalf("Problem storing config in file %s: %s\n", enc, err) + } + + // Prepare password input for decryption + passFile, err = os.Open(passFile.Name()) + if err != nil { + t.Fatalf("Problem opening temp file at %s: %s\n", passFile.Name(), err) + } + defer passFile.Close() + os.Stdin = passFile + + // Clean session + readConf := &Config{} + // Load with no existing state, key is read from the prepared file + err = readConf.ReadConfig(enc, true) + + // Verify + if err != nil { + t.Fatalf("Problem reading config in file %s: %s\n", enc, err) + } + + if c.Name != readConf.Name || c.EncryptConfig != readConf.EncryptConfig { + t.Error("Loaded conf not the same as original") + } +} + +// setAnswersFile sets the given file as the current stdin +// returns the close function to defer for reverting the stdin +func setAnswersFile(t *testing.T, answerFile string) func() { + oldIn := os.Stdin + + inputFile, err := os.Open(answerFile) + if err != nil { + t.Fatalf("Problem opening temp file at %s: %s\n", answerFile, err) + } + os.Stdin = inputFile + return func() { + inputFile.Close() + os.Stdin = oldIn + } +} + +func TestReadConfigWithPrompt(t *testing.T) { + // Prepare temp dir + tempDir, err := ioutil.TempDir("", "") + if err != nil { + t.Fatalf("Problem creating temp dir at %s: %s\n", tempDir, err) + } + defer os.RemoveAll(tempDir) + + // Ensure we'll get the prompt when loading + c := &Config{ + EncryptConfig: 0, + } + + // Save config + testConfigFile := filepath.Join(tempDir, "config.json") + err = c.SaveConfig(testConfigFile, false) + if err != nil { + t.Fatalf("Problem saving config file in %s: %s\n", tempDir, err) + } + + // Answers to the prompt + responseFile, err := ioutil.TempFile(tempDir, "*.in") + if err != nil { + t.Fatalf("Problem creating temp file at %s: %s\n", tempDir, err) + } + responseFile.WriteString("y\npass\npass\n") + responseFile.Close() + + // Temporarily replace Stdin with a custom input + cleanup := setAnswersFile(t, responseFile.Name()) + defer cleanup() + + // Run the test + c = &Config{} + c.ReadConfig(testConfigFile, false) + + // Verify results + data, err := ioutil.ReadFile(testConfigFile) + if err != nil { + t.Fatalf("Problem reading saved file at %s: %s\n", testConfigFile, err) + } + if c.EncryptConfig != fileEncryptionEnabled { + t.Error("Config encryption flag should be set after prompts") + } + if !ConfirmECS(data) { + t.Error("Config file should be encrypted after prompts") + } +} diff --git a/config/config_types.go b/config/config_types.go index d78b27a5..d527fcc2 100644 --- a/config/config_types.go +++ b/config/config_types.go @@ -67,9 +67,8 @@ const ( // Variables here are used for configuration var ( - Cfg Config - IsInitialSetup bool - m sync.Mutex + Cfg Config + m sync.Mutex ) // Config is the overarching object that holds all the information for @@ -99,6 +98,9 @@ type Config struct { FiatDisplayCurrency *currency.Code `json:"fiatDispayCurrency,omitempty"` Cryptocurrencies *currency.Currencies `json:"cryptocurrencies,omitempty"` SMS *SMSGlobalConfig `json:"smsGlobal,omitempty"` + // encryption session values + storedSalt []byte + sessionDK []byte } // ConnectionMonitorConfig defines the connection monitor variables to ensure