From e7d51505c7b10640ccaf724da4266afe8a079c38 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 19 Apr 2022 10:53:44 +0100 Subject: [PATCH] Utility function strbuf_dup. If you already have a string (of potentially-binary data) in the form of a ptrlen reference to somewhere else, and you want to keep a copy somewhere, it's useful to copy it into a strbuf. But it takes a couple of lines of faff to do that, and it's nicer to wrap that up into a tiny helper function. This commit adds that helper function strbuf_dup, and its non-movable sibling strbuf_dup_nm for secret data. Also, gone through the existing code and found a bunch of cases where this makes things less verbose. --- misc.h | 4 ++++ pageant.c | 20 ++++++-------------- ssh/login1.c | 3 +-- ssh/userauth2-client.c | 6 ++---- sshpubk.c | 3 +-- test/testcrypt.c | 36 ++++++++++++------------------------ utils/strbuf.c | 14 ++++++++++++++ 7 files changed, 40 insertions(+), 46 deletions(-) diff --git a/misc.h b/misc.h index 7acd8f41..a2dee9ac 100644 --- a/misc.h +++ b/misc.h @@ -52,6 +52,10 @@ struct strbuf { strbuf *strbuf_new(void); strbuf *strbuf_new_nm(void); +/* Helpers to allocate a strbuf containing an existing string */ +strbuf *strbuf_dup(ptrlen string); +strbuf *strbuf_dup_nm(ptrlen string); + void strbuf_free(strbuf *buf); void *strbuf_append(strbuf *buf, size_t len); void strbuf_shrink_to(strbuf *buf, size_t new_len); diff --git a/pageant.c b/pageant.c index f64026d8..e97eacbe 100644 --- a/pageant.c +++ b/pageant.c @@ -518,11 +518,8 @@ void pageant_passphrase_request_success(PageantClientDialogId *dlgid, BinarySource_BARE_INIT_PL(src, ptrlen_from_strbuf( pk->encrypted_key_file)); - strbuf *ppsb = strbuf_new_nm(); - put_datapl(ppsb, passphrase); - + strbuf *ppsb = strbuf_dup_nm(passphrase); pk->skey = ppk_load_s(src, ppsb->s, &error); - strbuf_free(ppsb); if (!pk->skey) { @@ -840,8 +837,7 @@ static PageantAsyncOp *pageant_make_op( so->pao.reqid = reqid; so->pk = pk; so->pkr.prev = so->pkr.next = NULL; - so->data_to_sign = strbuf_new(); - put_datapl(so->data_to_sign, sigdata); + so->data_to_sign = strbuf_dup(sigdata); so->flags = flags; so->failure_type = failure_type; so->crLine = 0; @@ -1180,8 +1176,7 @@ static PageantAsyncOp *pageant_make_op( * existing record, if it doesn't have one already. */ if (!pk->encrypted_key_file) { - pk->encrypted_key_file = strbuf_new_nm(); - put_datapl(pk->encrypted_key_file, keyfile); + pk->encrypted_key_file = strbuf_dup_nm(keyfile); keylist_update(); put_byte(sb, SSH_AGENT_SUCCESS); @@ -1205,8 +1200,7 @@ static PageantAsyncOp *pageant_make_op( public_blob = NULL; pk->sort.public_blob = ptrlen_from_strbuf(pk->public_blob); pk->comment = dupstr(comment); - pk->encrypted_key_file = strbuf_new_nm(); - put_datapl(pk->encrypted_key_file, keyfile); + pk->encrypted_key_file = strbuf_dup_nm(keyfile); PageantKey *added = add234(keytree, pk); assert(added == pk); (void)added; @@ -2233,8 +2227,7 @@ int pageant_enum_keys(pageant_key_enum_fn_t callback, void *callback_ctx, if (kl1) { for (size_t i = 0; i < kl1->nkeys; i++) { - cbkey.blob = strbuf_new(); - put_datapl(cbkey.blob, kl1->keys[i].blob); + cbkey.blob = strbuf_dup(kl1->keys[i].blob); cbkey.comment = mkstr(kl1->keys[i].comment); cbkey.ssh_version = 1; @@ -2265,8 +2258,7 @@ int pageant_enum_keys(pageant_key_enum_fn_t callback, void *callback_ctx, if (kl2) { for (size_t i = 0; i < kl2->nkeys; i++) { - cbkey.blob = strbuf_new(); - put_datapl(cbkey.blob, kl2->keys[i].blob); + cbkey.blob = strbuf_dup(kl2->keys[i].blob); cbkey.comment = mkstr(kl2->keys[i].comment); cbkey.ssh_version = 2; diff --git a/ssh/login1.c b/ssh/login1.c index 7b345c78..d556f22a 100644 --- a/ssh/login1.c +++ b/ssh/login1.c @@ -521,8 +521,7 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) RSA_SSH1_EXPONENT_FIRST); const char *blobend = get_ptr(s->asrc); - s->agent_keys[i].comment = strbuf_new(); - put_datapl(s->agent_keys[i].comment, get_string(s->asrc)); + s->agent_keys[i].comment = strbuf_dup(get_string(s->asrc)); s->agent_keys[i].blob = make_ptrlen( blobstart, blobend - blobstart); diff --git a/ssh/userauth2-client.c b/ssh/userauth2-client.c index ba97502f..3c760537 100644 --- a/ssh/userauth2-client.c +++ b/ssh/userauth2-client.c @@ -348,10 +348,8 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) s->agent_keys_len = nkeys; s->agent_keys = snewn(s->agent_keys_len, agent_key); for (size_t i = 0; i < nkeys; i++) { - s->agent_keys[i].blob = strbuf_new(); - put_datapl(s->agent_keys[i].blob, get_string(s->asrc)); - s->agent_keys[i].comment = strbuf_new(); - put_datapl(s->agent_keys[i].comment, get_string(s->asrc)); + s->agent_keys[i].blob = strbuf_dup(get_string(s->asrc)); + s->agent_keys[i].comment = strbuf_dup(get_string(s->asrc)); /* Also, extract the algorithm string from the start * of the public-key blob. */ diff --git a/sshpubk.c b/sshpubk.c index 326fe31c..70a64524 100644 --- a/sshpubk.c +++ b/sshpubk.c @@ -199,8 +199,7 @@ static int rsa1_load_s_internal(BinarySource *src, RSAKey *key, bool pub_only, if (enclen & 7) goto end; - buf = strbuf_new_nm(); - put_datapl(buf, get_data(src, enclen)); + buf = strbuf_dup_nm(get_data(src, enclen)); unsigned char keybuf[16]; hash_simple(&ssh_md5, ptrlen_from_asciz(passphrase), keybuf); diff --git a/test/testcrypt.c b/test/testcrypt.c index 3157ed54..a271459e 100644 --- a/test/testcrypt.c +++ b/test/testcrypt.c @@ -763,8 +763,7 @@ strbuf *ssh_cipher_encrypt_wrapper(ssh_cipher *c, ptrlen input) if (input.len % ssh_cipher_alg(c)->blksize) fatal_error("ssh_cipher_encrypt: needs a multiple of %d bytes", ssh_cipher_alg(c)->blksize); - strbuf *sb = strbuf_new(); - put_datapl(sb, input); + strbuf *sb = strbuf_dup(input); ssh_cipher_encrypt(c, sb->u, sb->len); return sb; } @@ -774,8 +773,7 @@ strbuf *ssh_cipher_decrypt_wrapper(ssh_cipher *c, ptrlen input) if (input.len % ssh_cipher_alg(c)->blksize) fatal_error("ssh_cipher_decrypt: needs a multiple of %d bytes", ssh_cipher_alg(c)->blksize); - strbuf *sb = strbuf_new(); - put_datapl(sb, input); + strbuf *sb = strbuf_dup(input); ssh_cipher_decrypt(c, sb->u, sb->len); return sb; } @@ -785,8 +783,7 @@ strbuf *ssh_cipher_encrypt_length_wrapper(ssh_cipher *c, ptrlen input, { if (input.len != 4) fatal_error("ssh_cipher_encrypt_length: needs exactly 4 bytes"); - strbuf *sb = strbuf_new(); - put_datapl(sb, input); + strbuf *sb = strbuf_dup(input); ssh_cipher_encrypt_length(c, sb->u, sb->len, seq); return sb; } @@ -796,8 +793,7 @@ strbuf *ssh_cipher_decrypt_length_wrapper(ssh_cipher *c, ptrlen input, { if (input.len % ssh_cipher_alg(c)->blksize) fatal_error("ssh_cipher_decrypt_length: needs exactly 4 bytes"); - strbuf *sb = strbuf_new(); - put_datapl(sb, input); + strbuf *sb = strbuf_dup(input); ssh_cipher_decrypt_length(c, sb->u, sb->len, seq); return sb; } @@ -997,8 +993,7 @@ strbuf *des_encrypt_xdmauth_wrapper(ptrlen key, ptrlen data) fatal_error("des_encrypt_xdmauth: key must be 7 bytes long"); if (data.len % 8 != 0) fatal_error("des_encrypt_xdmauth: data must be a multiple of 8 bytes"); - strbuf *sb = strbuf_new(); - put_datapl(sb, data); + strbuf *sb = strbuf_dup(data); des_encrypt_xdmauth(key.ptr, sb->u, sb->len); return sb; } @@ -1009,8 +1004,7 @@ strbuf *des_decrypt_xdmauth_wrapper(ptrlen key, ptrlen data) fatal_error("des_decrypt_xdmauth: key must be 7 bytes long"); if (data.len % 8 != 0) fatal_error("des_decrypt_xdmauth: data must be a multiple of 8 bytes"); - strbuf *sb = strbuf_new(); - put_datapl(sb, data); + strbuf *sb = strbuf_dup(data); des_decrypt_xdmauth(key.ptr, sb->u, sb->len); return sb; } @@ -1021,8 +1015,7 @@ strbuf *des3_encrypt_pubkey_wrapper(ptrlen key, ptrlen data) fatal_error("des3_encrypt_pubkey: key must be 16 bytes long"); if (data.len % 8 != 0) fatal_error("des3_encrypt_pubkey: data must be a multiple of 8 bytes"); - strbuf *sb = strbuf_new(); - put_datapl(sb, data); + strbuf *sb = strbuf_dup(data); des3_encrypt_pubkey(key.ptr, sb->u, sb->len); return sb; } @@ -1033,8 +1026,7 @@ strbuf *des3_decrypt_pubkey_wrapper(ptrlen key, ptrlen data) fatal_error("des3_decrypt_pubkey: key must be 16 bytes long"); if (data.len % 8 != 0) fatal_error("des3_decrypt_pubkey: data must be a multiple of 8 bytes"); - strbuf *sb = strbuf_new(); - put_datapl(sb, data); + strbuf *sb = strbuf_dup(data); des3_decrypt_pubkey(key.ptr, sb->u, sb->len); return sb; } @@ -1047,8 +1039,7 @@ strbuf *des3_encrypt_pubkey_ossh_wrapper(ptrlen key, ptrlen iv, ptrlen data) fatal_error("des3_encrypt_pubkey_ossh: iv must be 8 bytes long"); if (data.len % 8 != 0) fatal_error("des3_encrypt_pubkey_ossh: data must be a multiple of 8 bytes"); - strbuf *sb = strbuf_new(); - put_datapl(sb, data); + strbuf *sb = strbuf_dup(data); des3_encrypt_pubkey_ossh(key.ptr, iv.ptr, sb->u, sb->len); return sb; } @@ -1061,8 +1052,7 @@ strbuf *des3_decrypt_pubkey_ossh_wrapper(ptrlen key, ptrlen iv, ptrlen data) fatal_error("des3_encrypt_pubkey_ossh: iv must be 8 bytes long"); if (data.len % 8 != 0) fatal_error("des3_decrypt_pubkey_ossh: data must be a multiple of 8 bytes"); - strbuf *sb = strbuf_new(); - put_datapl(sb, data); + strbuf *sb = strbuf_dup(data); des3_decrypt_pubkey_ossh(key.ptr, iv.ptr, sb->u, sb->len); return sb; } @@ -1075,8 +1065,7 @@ strbuf *aes256_encrypt_pubkey_wrapper(ptrlen key, ptrlen iv, ptrlen data) fatal_error("aes256_encrypt_pubkey: iv must be 16 bytes long"); if (data.len % 16 != 0) fatal_error("aes256_encrypt_pubkey: data must be a multiple of 16 bytes"); - strbuf *sb = strbuf_new(); - put_datapl(sb, data); + strbuf *sb = strbuf_dup(data); aes256_encrypt_pubkey(key.ptr, iv.ptr, sb->u, sb->len); return sb; } @@ -1089,8 +1078,7 @@ strbuf *aes256_decrypt_pubkey_wrapper(ptrlen key, ptrlen iv, ptrlen data) fatal_error("aes256_encrypt_pubkey: iv must be 16 bytes long"); if (data.len % 16 != 0) fatal_error("aes256_decrypt_pubkey: data must be a multiple of 16 bytes"); - strbuf *sb = strbuf_new(); - put_datapl(sb, data); + strbuf *sb = strbuf_dup(data); aes256_decrypt_pubkey(key.ptr, iv.ptr, sb->u, sb->len); return sb; } diff --git a/utils/strbuf.c b/utils/strbuf.c index 636467a4..c636de47 100644 --- a/utils/strbuf.c +++ b/utils/strbuf.c @@ -112,3 +112,17 @@ void strbuf_finalise_agent_query(strbuf *buf_o) assert(buf->visible.len >= 5); PUT_32BIT_MSB_FIRST(buf->visible.u, buf->visible.len - 4); } + +strbuf *strbuf_dup(ptrlen string) +{ + strbuf *buf = strbuf_new(); + put_datapl(buf, string); + return buf; +} + +strbuf *strbuf_dup_nm(ptrlen string) +{ + strbuf *buf = strbuf_new_nm(); + put_datapl(buf, string); + return buf; +}