Merge branch 'private/more_bounds'

Author: aveenismail

CHANGELOG

@@ -1,3 +1,19 @@
+=== Version 2.0.3
+
+all: Move away from archaic offensive terms
+all: Update build scripts to account for changes in newer MACOS
+all: Build on Windows with Visual Studio 2019
+pkcs11: Enable .Net to load yubihsm-pkcs11.dylib
+lib: Fix memory leaks
+lib: Security fixes
+lib: Add a session identifier for the backend
+lib: Make the backend more thread-safe on Windows
+shell: Honor the base64 format when returning a public key
+shell: Honor the PEM format when returning a certificate
+shell: Improve parsing of command line arguments when using OAEP decryption
+shell: Add support for special (national) characters
+test: Improve testing
+
 === Version 2.0.2
 
 yhauth: Report touch policy as part of the "list" command

CMakeLists.txt

@@ -95,7 +95,7 @@ endif ()
 
 set (yubihsm_shell_VERSION_MAJOR 2)
 set (yubihsm_shell_VERSION_MINOR 0)
-set (yubihsm_shell_VERSION_PATCH 2)
+set (yubihsm_shell_VERSION_PATCH 3)
 set (VERSION "${yubihsm_shell_VERSION_MAJOR}.${yubihsm_shell_VERSION_MINOR}.${yubihsm_shell_VERSION_PATCH}")
 
 if (${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
@@ -263,4 +263,4 @@ message("             Libraries     ${YUBIHSM_INSTALL_LIB_DIR}")
 message("             Includes      ${YUBIHSM_INSTALL_INC_DIR}")
 message("             Binaries      ${YUBIHSM_INSTALL_BIN_DIR}")
 message("             Manuals       ${YUBIHSM_INSTALL_MAN_DIR}")
-message("             Pkg-config    ${YUBIHSM_INSTALL_PKGCONFIG_DIR}")
+message("             Pkg-config    ${YUBIHSM_INSTALL_PKGCONFIG_DIR}")

common/util.c

@@ -557,10 +557,10 @@ bool base64_decode(const char *in, uint8_t *out, size_t *len) {
 bool hex_decode(const char *in, uint8_t *out, size_t *len) {
   int pos = 0;
   size_t in_len = strlen(in);
-  if (in[in_len - 1] == '\n') {
+  if (in_len > 0 && in[in_len - 1] == '\n') {
     in_len--;
   }
-  if (in[in_len - 1] == '\r') {
+  if (in_len > 0 && in[in_len - 1] == '\r') {
     in_len--;
   }
   if (in_len % 2 != 0) {

lib/yubihsm.c

@@ -307,7 +307,11 @@ static yh_rc _send_secure_msg(yh_session *session, yh_cmd cmd,
   decrypted_data[0] = cmd;
   decrypted_data[1] = (data_len & 0xff00) >> 8;
   decrypted_data[2] = data_len & 0x00ff;
-  memcpy(decrypted_data + 3, data, data_len);
+  if (data != NULL) {
+    // NOTE(adma): when data_len is 0, data can be NULL. This is UB for
+    // memcpy. Explicitly check against that
+    memcpy(decrypted_data + 3, data, data_len);
+  }
   work_buf_len = 3 + data_len;
 
   DBG_DUMPINFO(decrypted_data, data_len + 3,
@@ -357,6 +361,12 @@ static yh_rc _send_secure_msg(yh_session *session, yh_cmd cmd,
 
   // Response is MAC'ed and encrypted. Unwrap it
   out_len = response_msg.st.len;
+  if (out_len < SCP_MAC_LEN - 3 ||
+      (size_t)(3 + out_len - SCP_MAC_LEN) >= sizeof(work_buf)) {
+    DBG_ERR("Received invalid length %u", out_len);
+    yrc = YHR_BUFFER_TOO_SMALL;
+    goto cleanup;
+  }
 
   memcpy(work_buf, session->s.mac_chaining_value, SCP_PRF_LEN);
   response_msg.st.len = htons(response_msg.st.len);
@@ -709,6 +719,11 @@ yh_rc yh_create_session(yh_connector *connector, uint16_t authkey_id,
 
   // Save sid
   new_session->s.sid = (*ptr++);
+  if (new_session->s.sid > YH_MAX_SESSIONS - 1) {
+    DBG_ERR("Received invalid session ID %d", new_session->s.sid);
+    yrc = YHR_GENERIC_ERROR;
+    goto cs_failure;
+  }
 
   // Save card challenge
   memcpy(new_session->context + SCP_HOST_CHAL_LEN, ptr, SCP_CARD_CHAL_LEN);

lib/yubihsm.h

@@ -1489,8 +1489,9 @@ yh_rc yh_util_decrypt_pkcs1v1_5(yh_session *session, uint16_t key_id,
  * @param in_len Length of encrypted data. Must be 256, 384 or 512
  * @param out Decrypted data
  * @param out_len Length of decrypted data
- * @param label OAEP label
- * @param label_len Length of OAEP label. Must be 20, 32, 48 or 64
+ * @param label Hash of OAEP label. Hash function must be SHA-1, SHA-256,
+ *SHA-384 or SHA-512
+ * @param label_len Length of hash of OAEP label. Must be 20, 32, 48 or 64
  * @param mgf1Algo MGF1 algorithm
  *
  * @return #YHR_SUCCESS if successful.

pkcs11/util_pkcs11.c

@@ -39,6 +39,7 @@
 #include "debug_p11.h"
 #include "../common/util.h"
 #include "../common/openssl-compat.h"
+#include "../common/insecure_memzero.h"
 
 #ifdef _MSVC
 #define gettimeofday(a, b) gettimeofday_win(a)
@@ -1459,33 +1460,22 @@ static CK_RV get_attribute_public_key(CK_ATTRIBUTE_TYPE type,
   return CKR_OK;
 }
 
-CK_RV get_attribute(CK_ATTRIBUTE_TYPE type, yh_object_descriptor *object,
-                    CK_VOID_PTR value, CK_ULONG_PTR length,
-                    yh_session *session) {
-
-  CK_BYTE tmp[2048];
-  CK_VOID_PTR ptr;
-  if (value == NULL) {
-    // NOTE(adma): we just need the length, use a scratchpad for the data
-    ptr = tmp;
-    *length = sizeof(tmp);
-  } else {
-    // NOTE(adma): otherwise actually save the data
-    ptr = value;
-  }
+static CK_RV get_attribute(CK_ATTRIBUTE_TYPE type, yh_object_descriptor *object,
+                           CK_BYTE_PTR value, CK_ULONG_PTR length,
+                           yh_session *session) {
 
   switch (object->type) {
     case YH_OPAQUE:
-      return get_attribute_opaque(type, object, ptr, length, session);
+      return get_attribute_opaque(type, object, value, length, session);
 
     case YH_WRAP_KEY:
     case YH_HMAC_KEY:
-      return get_attribute_secret_key(type, object, ptr, length);
+      return get_attribute_secret_key(type, object, value, length);
 
     case YH_ASYMMETRIC_KEY:
-      return get_attribute_private_key(type, object, ptr, length, session);
+      return get_attribute_private_key(type, object, value, length, session);
     case 0x80 | YH_ASYMMETRIC_KEY:
-      return get_attribute_public_key(type, object, ptr, length, session);
+      return get_attribute_public_key(type, object, value, length, session);
 
     case YH_TEMPLATE:
     case YH_AUTHENTICATION_KEY:
@@ -1497,50 +1487,43 @@ CK_RV get_attribute(CK_ATTRIBUTE_TYPE type, yh_object_descriptor *object,
   return CKR_OK;
 }
 
-CK_RV get_attribute_ecsession_key(CK_ATTRIBUTE_TYPE type, ecdh_session_key *key,
-                                  CK_VOID_PTR value, CK_ULONG_PTR length) {
-
-  CK_BYTE tmp[2048];
-  CK_VOID_PTR ptr;
-  if (value == NULL) {
-    ptr = tmp;
-    *length = sizeof(tmp);
-  } else {
-    ptr = value;
-  }
+static CK_RV get_attribute_ecsession_key(CK_ATTRIBUTE_TYPE type,
+                                         ecdh_session_key *key,
+                                         CK_BYTE_PTR value,
+                                         CK_ULONG_PTR length) {
 
   switch (type) {
     case CKA_CLASS:
-      *((CK_OBJECT_CLASS *) ptr) = CKO_SECRET_KEY;
+      *((CK_OBJECT_CLASS *) value) = CKO_SECRET_KEY;
       *length = sizeof(CK_OBJECT_CLASS);
       break;
 
     case CKA_KEY_TYPE:
-      *((CK_KEY_TYPE *) ptr) = CKK_GENERIC_SECRET;
+      *((CK_KEY_TYPE *) value) = CKK_GENERIC_SECRET;
       *length = sizeof(CK_KEY_TYPE);
       break;
 
     case CKA_ID: {
-      CK_OBJECT_HANDLE *id = ptr;
+      CK_OBJECT_HANDLE *id = (CK_OBJECT_HANDLE *) value;
       *id = key->id;
       *length = sizeof(CK_OBJECT_HANDLE);
       break;
     }
 
     case CKA_LABEL:
       *length = strlen(key->label);
-      memcpy(ptr, key->label, *length);
+      memcpy(value, key->label, *length);
       break;
 
     case CKA_LOCAL:
     case CKA_TOKEN:
-      *((CK_BBOOL *) ptr) = CK_FALSE;
+      *((CK_BBOOL *) value) = CK_FALSE;
       *length = sizeof(CK_BBOOL);
       break;
 
     case CKA_DESTROYABLE:
     case CKA_EXTRACTABLE:
-      *((CK_BBOOL *) ptr) = CK_TRUE;
+      *((CK_BBOOL *) value) = CK_TRUE;
       *length = sizeof(CK_BBOOL);
       break;
 
@@ -1558,12 +1541,12 @@ CK_RV get_attribute_ecsession_key(CK_ATTRIBUTE_TYPE type, ecdh_session_key *key,
     case CKA_WRAP_WITH_TRUSTED:
     case CKA_VERIFY:
     case CKA_ENCRYPT:
-      *((CK_BBOOL *) ptr) = CK_FALSE;
+      *((CK_BBOOL *) value) = CK_FALSE;
       *length = sizeof(CK_BBOOL);
       break;
 
     case CKA_VALUE:
-      memcpy(ptr, key->ecdh_key, key->len);
+      memcpy(value, key->ecdh_key, key->len);
       *length = key->len;
       break;
 
@@ -3879,31 +3862,37 @@ CK_RV populate_template(int type, void *object, CK_ATTRIBUTE_PTR pTemplate,
                         CK_ULONG ulCount, yh_session *session) {
 
   CK_RV rv = CKR_OK;
+  CK_BYTE tmp[8192];
 
   for (CK_ULONG i = 0; i < ulCount; i++) {
     DBG_INFO("Getting attribute 0x%lx", pTemplate[i].type);
-
-    CK_VOID_PTR object_ptr;
-    if (pTemplate[i].pValue == NULL) {
-      // NOTE(adma): just asking for the length
-      object_ptr = NULL;
-      DBG_INFO("Retrieving length");
-    } else {
-      // NOTE(adma): actually get the attribute
-      object_ptr = pTemplate[i].pValue;
-      DBG_INFO("Retrieving attribute");
-    }
-
+    CK_ULONG len = sizeof(tmp);
     CK_RV attribute_rc;
+
     if (type == ECDH_KEY_TYPE) {
       ecdh_session_key *key = object;
       attribute_rc =
-        get_attribute_ecsession_key(pTemplate[i].type, key, object_ptr,
-                                    &pTemplate[i].ulValueLen);
+        get_attribute_ecsession_key(pTemplate[i].type, key, tmp, &len);
     } else {
       yubihsm_pkcs11_object_desc *desc = object;
-      attribute_rc = get_attribute(pTemplate[i].type, &desc->object, object_ptr,
-                                   &pTemplate[i].ulValueLen, session);
+      attribute_rc =
+        get_attribute(pTemplate[i].type, &desc->object, tmp, &len, session);
+    }
+
+    if (attribute_rc == CKR_OK) {
+      if (pTemplate[i].pValue == NULL) {
+        DBG_INFO("Retrieving only length which is %lu", len);
+        pTemplate[i].ulValueLen = len;
+      } else if (len > pTemplate[i].ulValueLen) {
+        DBG_WARN("Skipping attribute, buffer to small %lu > %lu", len,
+                 pTemplate[i].ulValueLen);
+        attribute_rc = CKR_BUFFER_TOO_SMALL;
+        pTemplate[i].ulValueLen = CK_UNAVAILABLE_INFORMATION;
+      } else {
+        DBG_INFO("Retrieving attribute value, length is %lu", len);
+        memcpy(pTemplate[i].pValue, tmp, len);
+        pTemplate[i].ulValueLen = len;
+      }
     }
 
     if (attribute_rc != CKR_OK) {
@@ -3913,7 +3902,7 @@ CK_RV populate_template(int type, void *object, CK_ATTRIBUTE_PTR pTemplate,
       } else if (attribute_rc == CKR_BUFFER_TOO_SMALL) {
         DBG_ERR("Skipping attribute because buffer is too small");
       } else {
-        DBG_ERR("Get attribute failed. %s", yh_strerror(attribute_rc));
+        DBG_ERR("Get attribute failed.");
       }
     } else {
       DBG_INFO("Attribute/length successfully returned with length %lu",
@@ -3940,6 +3929,8 @@ CK_RV populate_template(int type, void *object, CK_ATTRIBUTE_PTR pTemplate,
      * type having the CKF_ARRAY_ATTRIBUTE bit set.*/
   }
 
+  insecure_memzero(tmp, sizeof(tmp));
+
   return rv;
 }
 

pkcs11/util_pkcs11.h

@@ -33,12 +33,6 @@ bool parse_hex(CK_UTF8CHAR_PTR hex, CK_ULONG hex_len, uint8_t *parsed);
 
 void destroy_session(yubihsm_pkcs11_context *ctx, CK_SESSION_HANDLE hSession);
 
-CK_RV get_attribute(CK_ATTRIBUTE_TYPE type, yh_object_descriptor *object,
-                    CK_VOID_PTR value, CK_ULONG_PTR length,
-                    yh_session *session);
-CK_RV get_attribute_ecsession_key(CK_ATTRIBUTE_TYPE type, ecdh_session_key *key,
-                                  CK_VOID_PTR value, CK_ULONG_PTR length);
-
 yubihsm_pkcs11_object_desc *get_object_desc(yh_session *session,
                                             yubihsm_pkcs11_object_desc *objects,
                                             CK_OBJECT_HANDLE objectHandle);

src/README.adoc

@@ -6,7 +6,13 @@ For the most part it is a thin wrapper around `libyubihsm` exposing
 most of its functions directly to the user.
 
 YubiHSM Shell can be invoked in interactive mode and from the command
-line.
+line. Special (national) characters are supported on MacOS and Linux
+platforms. On Windows, they are supported in interactive mode and the
+same support can be activated through the OpenSSL environment variable
+`OPENSSL_WIN32_UTF8` for interactive password entry in non-interactive
+mode (i.e if password is not given on the command line). Such characters
+will be encoded according to current locale settings on MacOS/Linux
+(typically utf-8), and always as utf-8 on Windows.
 
 === Interactive Mode
 

src/commands.c

@@ -76,7 +76,7 @@ int yh_com_audit(yubihsm_context *ctx, Argument *argv, cmd_format fmt) {
   uint16_t unlogged_boot = 0;
   uint16_t unlogged_auth = 0;
   yh_log_entry logs[YH_MAX_LOG_ENTRIES];
-  size_t n_items = sizeof(logs);
+  size_t n_items = sizeof(logs) / sizeof(logs[0]);
 
   switch (fmt) {
     case fmt_binary:
@@ -2773,8 +2773,8 @@ int yh_com_generate_otp_aead_key(yubihsm_context *ctx, Argument *argv,
 // arg 0: e:session
 // arg 1: w:key_id
 // arg 2: a:algorithm
-// arg 3: s:label
-// arg 4: f:datafile
+// arg 3: f:datafile
+// arg 4: s:label
 int yh_com_decrypt_oaep(yubihsm_context *ctx, Argument *argv, cmd_format fmt) {
 
   yh_rc yrc;
@@ -2814,7 +2814,8 @@ int yh_com_decrypt_oaep(yubihsm_context *ctx, Argument *argv, cmd_format fmt) {
       return -1;
   }
 
-  if (hash_bytes(argv[4].x, argv[4].len, hash, label, &label_len) == false) {
+  if (hash_bytes((const uint8_t *) argv[4].s, argv[4].len, hash, label,
+                 &label_len) == false) {
     fprintf(stderr, "Unable to hash data\n");
     return -1;
   }