From ffe5b3ad0ace1bda0a50373b4fc30bd702a62ec7 Mon Sep 17 00:00:00 2001 From: Rob Austein Date: Sun, 23 Apr 2017 18:29:03 -0400 Subject: Log exit conditions --- cryptech_muxd | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cryptech_muxd b/cryptech_muxd index 5b458a4..8e57ef8 100755 --- a/cryptech_muxd +++ b/cryptech_muxd @@ -438,4 +438,8 @@ if __name__ == "__main__": try: tornado.ioloop.IOLoop.current().run_sync(main) except KeyboardInterrupt: - pass + logger.debug("Normal SIGTERM exit") + except: + logger.exception("Unhandled exception") + else: + logger.debug("Main loop exited") -- cgit v1.2.3 From f502844d18282c928cf5fedb483514c1fcfd0b92 Mon Sep 17 00:00:00 2001 From: Rob Austein Date: Sun, 23 Apr 2017 18:30:05 -0400 Subject: Handle error conditions when deleting keys by UUID. --- unit-tests.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/unit-tests.py b/unit-tests.py index 2cdc663..10aa810 100644 --- a/unit-tests.py +++ b/unit-tests.py @@ -563,11 +563,19 @@ class TestPKeyMatch(TestCaseLoggedIn): exportable = HAL_KEY_FLAG_EXPORTABLE) return ", ".join(sorted(k for k, v in names.iteritems() if (flags & v) != 0)) + @staticmethod + def cleanup_key(uuid): + try: + with hsm.pkey_open(uuid) as pkey: + pkey.delete() + except Exception as e: + logger.debug("Problem deleting key %s: %s", uuid, e) + def load_keys(self, flags): uuids = set() for obj in PreloadedKey.db.itervalues(): with hsm.pkey_load(obj.der, flags) as k: - self.addCleanup(lambda uuid: hsm.pkey_open(uuid).delete(), k.uuid) + self.addCleanup(self.cleanup_key, k.uuid) uuids.add(k.uuid) #print k.uuid, k.key_type, k.key_curve, self.key_flag_names(k.key_flags) k.set_attributes(dict((i, a) for i, a in enumerate((str(obj.keytype), str(obj.fn2))))) @@ -637,13 +645,21 @@ class TestPKeyAttribute(TestCaseLoggedIn): Attribute creation/lookup/deletion tests. """ + @staticmethod + def cleanup_key(uuid): + try: + with hsm.pkey_open(uuid) as pkey: + pkey.delete() + except: + logger.debug("Problem deleting key %s: %s", uuid, e) + def load_and_fill(self, flags, n_keys = 1, n_attrs = 2, n_fill = 0): pinwheel = Pinwheel() for i in xrange(n_keys): for obj in PreloadedKey.db.itervalues(): with hsm.pkey_load(obj.der, flags) as k: pinwheel() - self.addCleanup(lambda uuid: hsm.pkey_open(uuid).delete(), k.uuid) + self.addCleanup(self.cleanup_key, k.uuid) k.set_attributes(dict((j, "Attribute {}{}".format(j, "*" * n_fill)) for j in xrange(n_attrs))) pinwheel() -- cgit v1.2.3 From c9fc4a5779db08a6c8a0029b779826a188d8b438 Mon Sep 17 00:00:00 2001 From: Rob Austein Date: Sun, 23 Apr 2017 18:30:50 -0400 Subject: Avoid deadlock triggered by low-probability race condition. Static code analysis (Doxygen call graph) detected a low-probability race condition which could have triggered a deadlock on the keystore mutex if the mkmif code returns with an error like HAL_ERROR_CORE_BUSY when we're trying to fetch the KEK. This is a knock-on effect of the awful kludge of backing up the KEK in the keystore flash as an alternative to powering the MKM with a battery as called for in the design. This code path should not exist at all, but, for now, we avoid the deadlock by making it the caller's responsibility to grab the keystore mutex before looking up the KEK. --- hal_internal.h | 1 + ks_flash.c | 18 +++++++++++------- mkm.c | 10 +++++++++- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/hal_internal.h b/hal_internal.h index f17179c..56d0936 100644 --- a/hal_internal.h +++ b/hal_internal.h @@ -421,6 +421,7 @@ extern hal_error_t hal_mkm_volatile_erase(const size_t len); /* #warning MKM flash backup kludge enabled. Do NOT use this in production! */ extern hal_error_t hal_mkm_flash_read(uint8_t *buf, const size_t len); +extern hal_error_t hal_mkm_flash_read_no_lock(uint8_t *buf, const size_t len); extern hal_error_t hal_mkm_flash_write(const uint8_t * const buf, const size_t len); extern hal_error_t hal_mkm_flash_erase(const size_t len); diff --git a/ks_flash.c b/ks_flash.c index b14e568..8aadc37 100644 --- a/ks_flash.c +++ b/ks_flash.c @@ -2119,7 +2119,7 @@ hal_error_t hal_set_pin(const hal_user_t user, * while re-implementing it on top of the new keystore. */ -hal_error_t hal_mkm_flash_read(uint8_t *buf, const size_t len) +hal_error_t hal_mkm_flash_read_no_lock(uint8_t *buf, const size_t len) { if (buf != NULL && len != KEK_LENGTH) return HAL_ERROR_MASTERKEY_BAD_LENGTH; @@ -2128,18 +2128,22 @@ hal_error_t hal_mkm_flash_read(uint8_t *buf, const size_t len) hal_error_t err; unsigned b; - hal_ks_lock(); - if ((err = fetch_pin_block(&b, &block)) != HAL_OK) - goto done; + return err; if (block->pin.kek_set != FLASH_KEK_SET) - err = HAL_ERROR_MASTERKEY_NOT_SET; + return HAL_ERROR_MASTERKEY_NOT_SET; - else if (buf != NULL) + if (buf != NULL) memcpy(buf, block->pin.kek, len); - done: + return HAL_OK; +} + +hal_error_t hal_mkm_flash_read(uint8_t *buf, const size_t len) +{ + hal_ks_lock(); + const hal_error_t err = hal_mkm_flash_read_no_lock(buf, len); hal_ks_unlock(); return err; } diff --git a/mkm.c b/mkm.c index 2b2141f..05c733d 100644 --- a/mkm.c +++ b/mkm.c @@ -201,7 +201,15 @@ hal_error_t hal_mkm_get_kek(uint8_t *kek, #if HAL_MKM_FLASH_BACKUP_KLUDGE - if (hal_mkm_flash_read(kek, len) == LIBHAL_OK) { + /* + * It turns out that, in every case where this function is called, + * we already hold the keystore lock, so attempting to grab it again + * would deadlock. This almost never happens when the volatile MKM + * is set, but there's a race condition that might drop us here if + * hal_mkm_volatile_read() returns HAL_ERROR_CORE_BUSY. Whee! + */ + + if (hal_mkm_flash_read_no_lock(kek, len) == LIBHAL_OK) { *kek_len = len; return LIBHAL_OK; } -- cgit v1.2.3 From 42aefa36bc89373125f88bb8f9a504b64f7bba0f Mon Sep 17 00:00:00 2001 From: Rob Austein Date: Sun, 23 Apr 2017 19:54:25 -0400 Subject: Wrap keyslot clearing in a critical section. I doubt this change will have any noticable effect, but it's another theoretical race condition, might as well eliminate it. --- rpc_pkey.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/rpc_pkey.c b/rpc_pkey.c index dc930cf..bdf8a7e 100644 --- a/rpc_pkey.c +++ b/rpc_pkey.c @@ -92,6 +92,21 @@ static inline hal_pkey_slot_t *alloc_slot(const hal_key_flags_t flags) return slot; } +/* + * Clear a slot. Probably not necessary to do this in a critical + * section, but be safe. + */ + +static inline void clear_slot(hal_pkey_slot_t *slot) +{ + hal_critical_section_start(); + + if (slot != NULL) + memset(slot, 0, sizeof(*slot)); + + hal_critical_section_end(); +} + /* * Check a caller-supplied handle. Must be in range, in use, and have * the right glop. Returns slot pointer on success, NULL otherwise. @@ -395,7 +410,7 @@ static hal_error_t pkey_local_open(const hal_client_handle_t client, return HAL_OK; fail: - memset(slot, 0, sizeof(*slot)); + clear_slot(slot); return err; } @@ -537,7 +552,7 @@ static hal_error_t pkey_local_close(const hal_pkey_handle_t pkey) if ((slot = find_handle(pkey)) == NULL) return HAL_ERROR_KEY_NOT_FOUND; - memset(slot, 0, sizeof(*slot)); + clear_slot(slot); return HAL_OK; } @@ -566,7 +581,7 @@ static hal_error_t pkey_local_delete(const hal_pkey_handle_t pkey) (void) hal_ks_close(ks); if (err == HAL_OK || err == HAL_ERROR_KEY_NOT_FOUND) - memset(slot, 0, sizeof(*slot)); + clear_slot(slot); return err; } -- cgit v1.2.3 From bf19937394ad4286d4c0e820ca128ea0cc95e35a Mon Sep 17 00:00:00 2001 From: Rob Austein Date: Wed, 26 Apr 2017 20:00:30 -0400 Subject: Don't intefere with sys.exit(). --- cryptech_muxd | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cryptech_muxd b/cryptech_muxd index 8e57ef8..d28f758 100755 --- a/cryptech_muxd +++ b/cryptech_muxd @@ -437,8 +437,8 @@ def main(): if __name__ == "__main__": try: tornado.ioloop.IOLoop.current().run_sync(main) - except KeyboardInterrupt: - logger.debug("Normal SIGTERM exit") + except (SystemExit, KeyboardInterrupt): + pass except: logger.exception("Unhandled exception") else: -- cgit v1.2.3 From 9c9f26fa04882cdc1ddb01410688f4d2651782af Mon Sep 17 00:00:00 2001 From: Rob Austein Date: Wed, 26 Apr 2017 20:01:01 -0400 Subject: Lower PBKDF2 password iterations and add delay on bad PIN. Consistent user complaints about HSM login taking too long. Underlying issue has both superficial and fundamental causes. Superficial: Our PBKDF2 implementation is slow. We could almost certainly make it faster by taking advantage of partial pre-calculation (see notes in code) and by reenabling use of FPGA hash cores when when checking passwords (which mgiht require linking the bootloader against a separate libhal build to avoid chicken-and-egg problem of needing FPGA to log into console to configure FPGA). Fundamental: The PBKDF2 iteration counts we used to use (10,000 minimum, 20,000 default) are in line with current NIST recommendations. The new, faster values (1,000 and 2,000, respectively) are not, or, rather, they're in line with what NIST recommended a decade ago. Well, OK, maybe the Coretex M4 is so slow that it's living in the past, but still. The fundamental issue is that anybody who can capture the encoded PIN can mount an offline dictionary attack on it, so we'd like to make that expensive. But the users are unhappy with the current behavior, so this change falls back to the ancient technique of adding a delay (currently five seconds, configurable at compile time) after a bad PIN, which makes it painful to use the login function as an oracle but does nothing about the offline dictionary attack problem. Feh. Note that users can still choose a higher iteration count, by setting the iteration count via the console. It's just not the default out of the box anymore. --- hal_internal.h | 7 ++++++- last_gasp_pin_internal.h | 6 +++--- rpc_misc.c | 16 +++++++++++++--- utils/last_gasp_default_pin | 2 +- 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/hal_internal.h b/hal_internal.h index 56d0936..36f24d4 100644 --- a/hal_internal.h +++ b/hal_internal.h @@ -99,9 +99,14 @@ extern void hal_ks_lock(void); extern void hal_ks_unlock(void); /* - * Logging. + * Thread sleep. Currently used only for bad-PIN delays. */ +extern void hal_sleep(const unsigned seconds); + +/* + * Logging. + */ typedef enum { HAL_LOG_DEBUG, HAL_LOG_INFO, HAL_LOG_WARN, HAL_LOG_ERROR, HAL_LOG_SILENT diff --git a/last_gasp_pin_internal.h b/last_gasp_pin_internal.h index bbcac76..901f797 100644 --- a/last_gasp_pin_internal.h +++ b/last_gasp_pin_internal.h @@ -3,7 +3,7 @@ */ static const hal_ks_pin_t hal_last_gasp_pin = { - 10000, - {0x06, 0xe2, 0x10, 0x7b, 0xb8, 0x40, 0xb5, 0x90, 0x33, 0xc8, 0xdb, 0xcc, 0xde, 0x3e, 0xb0, 0x33, 0x2b, 0x7c, 0x60, 0x7c, 0xb4, 0x52, 0xb1, 0x43, 0xa2, 0x20, 0x71, 0xdd, 0xbc, 0x95, 0x92, 0x04, 0xe6, 0x51, 0x90, 0xda, 0x6e, 0x2b, 0x6d, 0x8c, 0xb8, 0x63, 0x8d, 0x59, 0xad, 0xc5, 0xae, 0x6c, 0xf5, 0x7c, 0x75, 0x5e, 0x38, 0x72, 0x06, 0xc5, 0xa9, 0x3b, 0xaa, 0xe9, 0x64, 0x6e, 0xb1, 0x1a}, - {0x40, 0x49, 0xe4, 0xb6, 0x18, 0x0e, 0xe2, 0xbf, 0x3b, 0x22, 0xc8, 0xfe, 0xeb, 0xef, 0x09, 0x81} + 1000, + {0xd5, 0xde, 0xe9, 0x9f, 0x0c, 0xd0, 0xc1, 0x72, 0xfe, 0xe1, 0x8e, 0xe2, 0xad, 0x94, 0x9e, 0x9a, 0xb2, 0x11, 0x14, 0xe4, 0xa4, 0x04, 0xf0, 0x98, 0xd1, 0x44, 0x22, 0x8a, 0x7c, 0x23, 0x5d, 0xdb, 0xe4, 0x29, 0xa6, 0x95, 0x4b, 0xbb, 0x34, 0xf7, 0x16, 0x8b, 0x3f, 0x67, 0x65, 0xc9, 0xa2, 0x2b, 0xcc, 0x5a, 0x25, 0xa7, 0xef, 0xd5, 0x2e, 0x99, 0x75, 0xc8, 0x0f, 0xd9, 0xff, 0x76, 0xf6, 0x1c}, + {0x34, 0x3f, 0x18, 0x36, 0x94, 0xeb, 0xda, 0xb6, 0x5a, 0x5c, 0xbe, 0xc7, 0x61, 0xa0, 0x43, 0x5f} }; diff --git a/rpc_misc.c b/rpc_misc.c index cf5e4a0..3f466bb 100644 --- a/rpc_misc.c +++ b/rpc_misc.c @@ -78,15 +78,23 @@ typedef struct { } client_slot_t; #ifndef HAL_PIN_MINIMUM_ITERATIONS -#define HAL_PIN_MINIMUM_ITERATIONS 10000 +#define HAL_PIN_MINIMUM_ITERATIONS 1000 #endif #ifndef HAL_PIN_DEFAULT_ITERATIONS -#define HAL_PIN_DEFAULT_ITERATIONS 20000 +#define HAL_PIN_DEFAULT_ITERATIONS 2000 #endif static uint32_t hal_pin_default_iterations = HAL_PIN_DEFAULT_ITERATIONS; +/* + * Seconds to delay when given a bad PIN. + */ + +#ifndef HAL_PIN_DELAY_ON_FAILURE +#define HAL_PIN_DELAY_ON_FAILURE 5 +#endif + #ifndef HAL_STATIC_CLIENT_STATE_BLOCKS #define HAL_STATIC_CLIENT_STATE_BLOCKS 10 #endif @@ -155,8 +163,10 @@ static hal_error_t login(const hal_client_handle_t client, for (int i = 0; i < sizeof(buf); i++) diff |= buf[i] ^ p->pin[i]; - if (diff != 0) + if (diff != 0) { + hal_sleep(HAL_PIN_DELAY_ON_FAILURE); return HAL_ERROR_PIN_INCORRECT; + } client_slot_t *slot = find_handle(client); diff --git a/utils/last_gasp_default_pin b/utils/last_gasp_default_pin index 50d822f..8a91b8a 100755 --- a/utils/last_gasp_default_pin +++ b/utils/last_gasp_default_pin @@ -54,7 +54,7 @@ parser.add_argument("-p", "--pin", help = "PIN plaintext before PBKDF2 processing") parser.add_argument("-i", "--iterations", type = int, - default = 10000, + default = 1000, help = "PBKDF2 iteration count") parser.add_argument("-d", "--derived-key-length", type = int, -- cgit v1.2.3 From 018b238e7aac5b4a990a28f49323b821246c9f66 Mon Sep 17 00:00:00 2001 From: Rob Austein Date: Thu, 27 Apr 2017 01:16:45 -0400 Subject: Decorator doesn't work properly with --all-tests, use runtime check. --- unit-tests.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/unit-tests.py b/unit-tests.py index 10aa810..f0f5fb2 100644 --- a/unit-tests.py +++ b/unit-tests.py @@ -132,6 +132,10 @@ class TestCase(unittest.TestCase): self.endTime = datetime.datetime.now() super(TestCase, self).tearDown() + def skipUnlessAll(self, reason): + if not args.all_tests: + self.skipTest(reason) + class TextTestResult(unittest.TextTestResult): def addSuccess(self, test): @@ -197,15 +201,12 @@ class TestPIN(TestCase): self.assertRaises(HAL_ERROR_FORBIDDEN, hsm.is_logged_in, user2) hsm.logout() - @unittest.skipUnless(args.all_tests, "Slow") def test_login_wheel(self): self.login_logout(HAL_USER_WHEEL) - @unittest.skipUnless(args.all_tests, "Slow") def test_login_so(self): self.login_logout(HAL_USER_SO) - @unittest.skipUnless(args.all_tests, "Slow") def test_login_user(self): self.login_logout(HAL_USER_NORMAL) @@ -292,18 +293,19 @@ class TestPKeyGen(TestCaseLoggedIn): def test_gen_sign_verify_rsa_1024_sha256(self): self.gen_sign_verify_rsa(HAL_DIGEST_ALGORITHM_SHA256, 1024) - @unittest.skipUnless(args.all_tests, "Slow") def test_gen_sign_verify_rsa_2048_sha384(self): + self.skipUnlessAll("Slow") self.gen_sign_verify_rsa(HAL_DIGEST_ALGORITHM_SHA384, 2048) - @unittest.skipUnless(args.all_tests, "Hideously slow") def test_gen_sign_verify_rsa_4096_sha512(self): + self.skipUnlessAll("Hideously slow") self.gen_sign_verify_rsa(HAL_DIGEST_ALGORITHM_SHA512, 4096) def test_gen_unsupported_length(self): with self.assertRaises(HAL_ERROR_BAD_ARGUMENTS): hsm.pkey_generate_rsa(1028).delete() + class TestPKeyHashing(TestCaseLoggedIn): """ Tests involving various ways of doing the hashing for public key operations. -- cgit v1.2.3