Move password buffer into the library

The design before was not completely fine. The user had to allocate the
buffer and passed ownership to the library.
As of [0] this seems to be a problem in some environments.

[0] #587 (comment)

Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

Author: sjaeckel

demos/openssh-privkey.c

@@ -29,15 +29,11 @@ static void die_(int err, int line)
 #define die(i) do { die_(i, __LINE__); } while(0)
 #define DIE(s, ...) do { print_err("%3d: " s "\n", __LINE__, ##__VA_ARGS__); exit(EXIT_FAILURE); } while(0)
 
-static char* getpassword(const char *prompt, size_t maxlen)
+static int getpassword(const char *prompt, char *pass, unsigned long *len)
 {
-   char *wr, *end, *pass = XCALLOC(1, maxlen + 1);
    struct termios tio;
    tcflag_t c_lflag;
-   if (pass == NULL)
-      return NULL;
-   wr = pass;
-   end = pass + maxlen;
+   unsigned long maxlen = *len, wr = 0;
 
    tcgetattr(0, &tio);
    c_lflag = tio.c_lflag;
@@ -46,24 +42,25 @@ static char* getpassword(const char *prompt, size_t maxlen)
 
    printf("%s", prompt);
    fflush(stdout);
-   while (pass < end) {
+   while (1) {
       int c = getchar();
       if (c == '\r' || c == '\n' || c == -1)
          break;
-      *wr++ = c;
+      if (wr < maxlen)
+         pass[wr] = c;
+      wr++;
    }
+   *len = wr;
    tio.c_lflag = c_lflag;
    tcsetattr(0, TCSAFLUSH, &tio);
    printf("\n");
-   return pass;
+   return wr <= maxlen;
 }
 
-static int password_get(void **p, unsigned long *l, void *u)
+static int password_get(void *p, unsigned long *l, void *u)
 {
    (void)u;
-   *p = getpassword("Enter passphrase: ", 256);
-   *l = strlen(*p);
-   return 0;
+   return getpassword("Enter passphrase: ", p, l);
 }
 
 static void print(ltc_pka_key *k)

src/headers/tomcrypt_custom.h

@@ -627,6 +627,10 @@
    #define LTC_PBES
 #endif
 
+#if defined(LTC_PEM) || defined(LTC_PKCS_8) && !defined(LTC_MAX_PASSWORD_LEN)
+   #define LTC_MAX_PASSWORD_LEN 256
+#endif
+
 #if defined(LTC_CLEAN_STACK)
 /* if you're sure that you want to use it, remove the line below */
    #error LTC_CLEAN_STACK is considered as broken

src/headers/tomcrypt_pk.h

@@ -5,17 +5,12 @@ typedef struct {
    /**
       Callback function that is called when a password is required.
 
-      Please be aware that the library takes ownership of the pointer that is
-      returned to the library via `str`.
-      `str` shall be allocated via the same function as `XMALLOC` points to.
-      The data will be zeroed and `XFREE`'d as soon as it isn't required anymore.
-
-      @param str        Pointer to pointer where the password will be stored.
-      @param len        Pointer to the length of the password.
+      @param str        Pointer to where the password shall be stored.
+      @param len        [in/out] The max length resp. resulting length of the password.
       @param userdata   `userdata` that was passed in the `password_ctx` struct.
       @return CRYPT_OK on success
    */
-   int (*callback)(void **str, unsigned long *len, void *userdata);
+   int (*callback)(void *str, unsigned long *len, void *userdata);
    /** Opaque `userdata` pointer passed when the callback is called */
    void *userdata;
 } password_ctx;

src/headers/tomcrypt_private.h

@@ -83,11 +83,18 @@ typedef struct {
    unsigned long blocklen;
 } pbes_properties;
 
+struct password {
+   /* usually a `char*` but could also contain binary data
+    * so use a `void*` + length to be on the safe side.
+    */
+   unsigned char pw[LTC_MAX_PASSWORD_LEN];
+   unsigned long l;
+};
+
 typedef struct
 {
    pbes_properties type;
-   void *pwd;
-   unsigned long pwdlen;
+   struct password pwd;
    ltc_asn1_list *enc_data;
    ltc_asn1_list *salt;
    ltc_asn1_list *iv;
@@ -259,14 +266,6 @@ enum cipher_mode {
    cm_none, cm_cbc, cm_cfb, cm_ctr, cm_ofb, cm_stream, cm_gcm
 };
 
-struct password {
-   /* usually a `char*` but could also contain binary data
-    * so use a `void*` + length to be on the safe side.
-    */
-   void *pw;
-   unsigned long l;
-};
-
 struct blockcipher_info {
    const char *name;
    const char *algo;

src/misc/crypt/crypt.c

@@ -470,6 +470,9 @@ const char *crypt_build_settings =
     " PBES1 "
     " PBES2 "
 #endif
+#if defined(LTC_MAX_PASSWORD_LEN)
+    " " NAME_VALUE(LTC_MAX_PASSWORD_LEN) " "
+#endif
 #if defined(LTC_PEM)
     " PEM "
     " " NAME_VALUE(LTC_PEM_DECODE_BUFSZ) " "

src/misc/pbes/pbes.c

@@ -50,7 +50,7 @@ int pbes_decrypt(const pbes_arg  *arg, unsigned char *dec_data, unsigned long *d
 
    if (klen > sizeof(k)) return CRYPT_INVALID_ARG;
 
-   if ((err = arg->type.kdf(arg->pwd, arg->pwdlen, arg->salt->data, arg->salt->size, arg->iterations, hid, k, &klen)) != CRYPT_OK) goto LBL_ERROR;
+   if ((err = arg->type.kdf(arg->pwd.pw, arg->pwd.l, arg->salt->data, arg->salt->size, arg->iterations, hid, k, &klen)) != CRYPT_OK) goto LBL_ERROR;
    if ((err = cbc_start(cid, iv, k, keylen, 0, &cbc)) != CRYPT_OK) goto LBL_ERROR;
    if ((err = cbc_decrypt(arg->enc_data->data, dec_data, arg->enc_data->size, &cbc)) != CRYPT_OK) goto LBL_ERROR;
    if ((err = cbc_done(&cbc)) != CRYPT_OK) goto LBL_ERROR;

src/misc/pem/pem_pkcs.c

@@ -25,9 +25,6 @@ static int s_decrypt_pem(unsigned char *pem, unsigned long *l, const struct pem_
    if (hdr->info.keylen > sizeof(key)) {
       return CRYPT_BUFFER_OVERFLOW;
    }
-   if (!hdr->pw->pw) {
-      return CRYPT_INVALID_ARG;
-   }
 
    ivlen = sizeof(iv);
    if ((err = base16_decode(hdr->info.iv, XSTRLEN(hdr->info.iv), iv, &ivlen)) != CRYPT_OK) {
@@ -202,7 +199,7 @@ static int s_decode(struct get_char *g, ltc_pka_key *k, const password_ctx *pw_c
    unsigned long w, l, n;
    int err = CRYPT_ERROR;
    struct pem_headers hdr = { 0 };
-   struct password pw;
+   struct password pw = { 0 };
    enum ltc_pka_id pka;
    XMEMSET(k, 0, sizeof(*k));
    w = LTC_PEM_READ_BUFSIZE * 2;
@@ -241,7 +238,8 @@ static int s_decode(struct get_char *g, ltc_pka_key *k, const password_ctx *pw_c
       }
 
       hdr.pw = &pw;
-      if (pw_ctx->callback(&hdr.pw->pw, &hdr.pw->l, pw_ctx->userdata)) {
+      hdr.pw->l = LTC_MAX_PASSWORD_LEN;
+      if (pw_ctx->callback(hdr.pw->pw, &hdr.pw->l, pw_ctx->userdata)) {
          err = CRYPT_ERROR;
          goto cleanup;
       }
@@ -266,8 +264,7 @@ static int s_decode(struct get_char *g, ltc_pka_key *k, const password_ctx *pw_c
 
 cleanup:
    if (hdr.pw) {
-      zeromem(hdr.pw->pw, hdr.pw->l);
-      XFREE(hdr.pw->pw);
+      zeromem(hdr.pw->pw, sizeof(hdr.pw->pw));
    }
    XFREE(pem);
    return err;

src/misc/pem/pem_ssh.c

@@ -570,7 +570,8 @@ static int s_decode_openssh(struct get_char *g, ltc_pka_key *k, const password_c
             err = CRYPT_PW_CTX_MISSING;
             goto cleanup;
          }
-         if (pw_ctx->callback(&opts.pw.pw, &opts.pw.l, pw_ctx->userdata)) {
+         opts.pw.l = LTC_MAX_PASSWORD_LEN;
+         if (pw_ctx->callback(opts.pw.pw, &opts.pw.l, pw_ctx->userdata)) {
             err = CRYPT_ERROR;
             goto cleanup;
          }
@@ -589,8 +590,8 @@ static int s_decode_openssh(struct get_char *g, ltc_pka_key *k, const password_c
    }
 
 cleanup:
-   if (opts.pw.pw) {
-      XFREE(opts.pw.pw);
+   if (opts.pw.l) {
+      zeromem(&opts.pw, sizeof(opts.pw));
    }
    if (privkey) {
       zeromem(privkey, privkey_len);

src/pk/asn1/pkcs8/pkcs8_decode_flexi.c

@@ -23,13 +23,11 @@ int pkcs8_decode_flexi(const unsigned char  *in,  unsigned long inlen,
    unsigned char *dec_data = NULL;
    ltc_asn1_list *l = NULL;
    int err;
-   pbes_arg pbes;
+   pbes_arg pbes = { 0 };
 
    LTC_ARGCHK(in           != NULL);
    LTC_ARGCHK(decoded_list != NULL);
 
-   XMEMSET(&pbes, 0, sizeof(pbes));
-
    *decoded_list = NULL;
    if ((err = der_decode_sequence_flexi(in, &len, &l)) == CRYPT_OK) {
       /* the following "if" detects whether it is encrypted or not */
@@ -63,7 +61,8 @@ int pkcs8_decode_flexi(const unsigned char  *in,  unsigned long inlen,
             goto LBL_DONE;
          }
 
-         if (pw_ctx->callback(&pbes.pwd, &pbes.pwdlen, pw_ctx->userdata)) {
+         pbes.pwd.l = LTC_MAX_PASSWORD_LEN;
+         if (pw_ctx->callback(pbes.pwd.pw, &pbes.pwd.l, pw_ctx->userdata)) {
             err = CRYPT_ERROR;
             goto LBL_DONE;
          }
@@ -95,9 +94,8 @@ int pkcs8_decode_flexi(const unsigned char  *in,  unsigned long inlen,
 
 LBL_DONE:
    if (l) der_free_sequence_flexi(l);
-   if (pbes.pwd) {
-      zeromem(pbes.pwd, pbes.pwdlen);
-      XFREE(pbes.pwd);
+   if (pbes.pwd.l) {
+      zeromem(&pbes.pwd, sizeof(pbes.pwd));
    }
    if (dec_data) {
       zeromem(dec_data, dec_size);

tests/ecc_test.c

@@ -660,12 +660,14 @@ static int s_ecc_new_api(void)
 }
 
 
-static int password_get(void **p, unsigned long *l, void *u)
+static int password_get(void *p, unsigned long *l, void *u)
 {
+   int ret = *l < 6;
    LTC_UNUSED_PARAM(u);
-   *p = strdup("secret");
+   if (!ret)
+      XMEMCPY(p, "secret", 6);
    *l = 6;
-   return 0;
+   return ret;
 }
 
 static int s_ecc_import_export(void) {