On Wed, Jul 05, 2006 at 03:33:14PM -0700, Erik Hovland wrote:
> This patch removes an #if 0 set of code. And it removes a passphrase
> check which cannot happen.
I have finished an audit of dropbear and decided to reply to my own post
because my current draft of patches expands on the same file (and a few
others.
The patches should be consistent with current mtn repo and should have
annotations in each file. I am willing to rework any patch that doesn't
meet the statisfaction of the devs. So please send feedback.
BTW, almost all of these 'bugs' cause no harm that I can tell. So don't
think there is any serious problems here. It is really just dotting i's
and crossing t's.
E
--
Erik Hovland
mail: erik AT hovland DOT org
web: http://hovland.org/
PGP/GPG public key available on request
BUG: mp_div_2d returns status and it isn't checked.
FIX: Check and return status.
--- libtommath/bn_mp_div.c old
+++ libtommath/bn_mp_div.c new
@@ -269,7 +269,8 @@ int mp_div (mp_int * a, mp_int * b, mp_i
}
if (d != NULL) {
- mp_div_2d (&x, norm, &x, NULL);
+ if ((res = mp_div_2d (&x, norm, &x, NULL)) != MP_OKAY)
+ return res;
mp_exch (&x, d);
}
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "libtommath/bn_mp_div.c"
# from [0f214a2ee7c1cfd6152ebd473d2f04db67f2f86c]
# to [de1182b10274bd4e691094aef260abe21fb8f5cc]
#
BUG: The strings 'name' and 'instruction' are always allocated
but are only freed if the length of the string is greater then
zero. They should always be freed.
FIX: take the m_free(<string>) out of the conditional
--- cli-authinteract.c old
+++ cli-authinteract.c new
@@ -99,13 +99,13 @@ void recv_msg_userauth_info_request() {
if (strlen(name) > 0) {
cleantext(name);
fprintf(stderr, "%s", name);
- m_free(name);
}
+ m_free(name);
if (strlen(instruction) > 0) {
cleantext(instruction);
fprintf(stderr, "%s", instruction);
- m_free(instruction);
}
+ m_free(instruction);
for (i = 0; i < num_prompts; i++) {
unsigned int response_len = 0;
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "cli-authinteract.c"
# from [c65d9c192f42ce4654ec4e8d6765b11e4f5ca9a9]
# to [301ca11246ed5945026d0d1bc4c407ddc5a50519]
#
BUG: keybuf can memory leak.
FIX: Add an m_free(keybuf) once we are done matching the keys.
--- cli-authpubkey.c old
+++ cli-authpubkey.c new
@@ -112,6 +112,7 @@ void recv_msg_userauth_pk_ok() {
/* Success */
break;
}
+ m_free(keybuf);
if (keyitem != NULL) {
TRACE(("matching key"))
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "cli-authpubkey.c"
# from [f4b6c66351e60851c73405d8ecf4676557f0da30]
# to [05a5cc2282123b73b132a498c46279ac93d273a3]
#
BUG: The fingerprint can leak if response is not 'y'.
FIX: Free the fingerprint buffer after we are done printing it.
--- cli-kex.c old
+++ cli-kex.c new
@@ -122,6 +122,7 @@ static void ask_to_confirm(unsigned char
fprintf(stderr, "\nHost '%s' is not in the trusted hosts
file.\n(fingerprint %s)\nDo you want to continue connecting? (y/n)\n",
cli_opts.remotehost,
fp);
+ m_free(fp);
tty = fopen(_PATH_TTY, "r");
if (tty) {
@@ -132,7 +133,6 @@ static void ask_to_confirm(unsigned char
}
if (response == 'y') {
- m_free(fp);
return;
}
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "cli-kex.c"
# from [3e15b2b2d3b42e45f38782414b709a48f11c56d9]
# to [28231814a32f302b4dc82265fecf9d4850e5a001]
#
Remove a stale bit of code since it should be
obvious waht is happening.
--- cli-service.c old
+++ cli-service.c new
@@ -82,6 +82,4 @@ void recv_msg_service_accept() {
}
dropbear_exit("unrecognised service accept");
- /* m_free(servicename); not reached */
-
}
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "cli-service.c"
# from [4b9ba5f1287845d47ba61d813ba53d143de4b804]
# to [c9175c55722051be489796ffa96f171ac70f73c4]
#
BUG: The call find_cipher can return a negative value if things don't
go right. We don't know that find_cipher is the cause of cbc_start() unless
we check find_cipher before calling cbc_start(). BTW, cbc_start() does check
that
the cipher it is handed is OK. So there is no seriouse damage.
FIX: Do the find_cipher() and check it.
NOTE: I consider this fix to be purely pedantic.
--- common-kex.c old
+++ common-kex.c new
@@ -262,6 +262,7 @@ void gen_new_keys() {
hash_state hs;
unsigned int C2S_keysize, S2C_keysize;
char mactransletter, macrecvletter; /* Client or server specific */
+ int recv_cipher = 0, trans_cipher = 0;
TRACE(("enter gen_new_keys"))
/* the dh_K and hash are the start of all hashes, we make use of that */
@@ -298,17 +299,21 @@ void gen_new_keys() {
hashkeys(C2S_key, C2S_keysize, &hs, 'C');
hashkeys(S2C_key, S2C_keysize, &hs, 'D');
+ if ((recv_cipher =
find_cipher(ses.newkeys->recv_algo_crypt->cipherdesc->name)) < 0)
+ dropbear_exit("crypto error");
+
if (cbc_start(
- find_cipher(ses.newkeys->recv_algo_crypt->cipherdesc->name),
- recv_IV, recv_key,
+ recv_cipher, recv_IV, recv_key,
ses.newkeys->recv_algo_crypt->keysize, 0,
&ses.newkeys->recv_symmetric_struct) != CRYPT_OK) {
dropbear_exit("crypto error");
}
+ if ((trans_cipher =
find_cipher(ses.newkeys->trans_algo_crypt->cipherdesc->name)) < 0)
+ dropbear_exit("crypto error");
+
if (cbc_start(
- find_cipher(ses.newkeys->trans_algo_crypt->cipherdesc->name),
- trans_IV, trans_key,
+ trans_cipher, trans_IV, trans_key,
ses.newkeys->trans_algo_crypt->keysize, 0,
&ses.newkeys->trans_symmetric_struct) != CRYPT_OK) {
dropbear_exit("crypto error");
@@ -517,7 +522,8 @@ void kexdh_comb_key(mp_int *dh_pub_us, m
hash_state hs;
/* read the prime and generator*/
- mp_init(&dh_p);
+ if (mp_init(&dh_p) != MP_OKAY)
+ dropbear_exit("Diffie-Hellman error");
bytes_to_mp(&dh_p, dh_p_val, DH_P_LEN);
/* Check that dh_pub_them (dh_e or dh_f) is in the range [1, p-1] */
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "common-kex.c"
# from [b06f424fe466a6d3ecfb7072d59457ebb39c8780]
# to [a5be345f9dd6aa724d95134f13a4fe36bf83320e]
#
BUG: The call to sign_key_free(key) could be called before key is allocated
because of the goto usage.
FIX: Check key before making the call.
--- dropbearkey.c old
+++ dropbearkey.c new
@@ -283,8 +283,10 @@ out:
buf_burn(buf);
buf_free(buf);
buf = NULL;
- sign_key_free(key);
- key = NULL;
+ if (key) {
+ sign_key_free(key);
+ key = NULL;
+ }
exit(err);
}
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "dropbearkey.c"
# from [c2a49c53fa12e4bf57922462e55b243119bb4043]
# to [eb8195c08cabcd2516d8fb5a461b4fa29e933cf0]
#
BUG: We could leak the file descriptor in certain cases.
FIX: Add an fclose to the calls if we had to do an explicit fopen.
BUG: The switch to using a goto instead of the code in the conditional
block makes the following 'if (passphrase)' call redundant.
NOTE: The code in the #if 0 comments is removed as well.
--- keyimport.c old
+++ keyimport.c new
@@ -362,6 +362,7 @@ static struct openssh_key *load_openssh_
{
struct openssh_key *ret;
FILE *fp;
+ int close_fp = 0;
char buffer[256];
char *errmsg = NULL, *p = NULL;
int headers_done;
@@ -377,9 +378,12 @@ static struct openssh_key *load_openssh_
fp = stdin;
} else {
fp = fopen(filename, "r");
+ close_fp = 1;
}
if (!fp) {
errmsg = "Unable to open key file";
+ if (close_fp)
+ close_fp = 0;
goto error;
}
if (!fgets(buffer, sizeof(buffer), fp) ||
@@ -482,6 +486,8 @@ static struct openssh_key *load_openssh_
memset(&ret, 0, sizeof(ret));
m_free(ret);
}
+ if (close_fp)
+ fclose(fp);
if (errmsg) {
fprintf(stderr, "Error: %s\n", errmsg);
}
@@ -926,40 +932,6 @@ static int openssh_write(const char *fil
if (passphrase) {
fprintf(stderr, "Encrypted keys aren't supported currently\n");
goto error;
-#if 0
- /*
- * Invent an iv. Then derive encryption key from passphrase
- * and iv/salt:
- *
- * - let block A equal MD5(passphrase || iv)
- * - let block B equal MD5(A || passphrase || iv)
- * - block C would be MD5(B || passphrase || iv) and so on
- * - encryption key is the first N bytes of A || B
- */
- struct MD5Context md5c;
- unsigned char keybuf[32];
-
- for (i = 0; i < 8; i++) iv[i] = random_byte();
-
- MD5Init(&md5c);
- MD5Update(&md5c, (unsigned char *)passphrase,
strlen(passphrase));
- MD5Update(&md5c, iv, 8);
- MD5Final(keybuf, &md5c);
-
- MD5Init(&md5c);
- MD5Update(&md5c, keybuf, 16);
- MD5Update(&md5c, (unsigned char *)passphrase,
strlen(passphrase));
- MD5Update(&md5c, iv, 8);
- MD5Final(keybuf+16, &md5c);
-
- /*
- * Now encrypt the key blob.
- */
- des3_encrypt_pubkey_ossh(keybuf, iv, outblob, outlen);
-
- memset(&md5c, 0, sizeof(md5c));
- memset(keybuf, 0, sizeof(keybuf));
-#endif
}
/*
@@ -976,12 +948,6 @@ static int openssh_write(const char *fil
goto error;
}
fputs(header, fp);
- if (passphrase) {
- fprintf(fp, "Proc-Type: 4,ENCRYPTED\nDEK-Info: DES-EDE3-CBC,");
- for (i = 0; i < 8; i++)
- fprintf(fp, "%02X", iv[i]);
- fprintf(fp, "\n\n");
- }
base64_encode_fp(fp, outblob, outlen, 64);
fputs(footer, fp);
fclose(fp);
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "keyimport.c"
# from [4d6aa56819151b18d6828b1a9b1db0b9864e40b3]
# to [61087db466ee8df9c0fc12f65affa660f62ab9ea]
#
BUG: The libtommath call mp_invmod does return a value but it is not checked.
FIX: Add a check.
--- rsa.c old
+++ rsa.c new
@@ -286,7 +286,8 @@ void buf_put_rsa_sign(buffer* buf, rsa_k
/* em' = em * r^e mod n */
mp_exptmod(&rsa_tmp2, key->e, key->n, &rsa_s); /* rsa_s used as a temp
var*/
- mp_invmod(&rsa_tmp2, key->n, &rsa_tmp3);
+ if (mp_invmod(&rsa_tmp2, key->n, &rsa_tmp3) != MP_OKAY)
+ dropbear_exit("rsa error");
mp_mulmod(&rsa_tmp1, &rsa_s, key->n, &rsa_tmp2);
/* rsa_tmp2 is em' */
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "rsa.c"
# from [792f3ff06fe126d3b69a067e56be005c57c8c968]
# to [9849a7e08b3d656e9deb9c9330499359a1e3b961]
#
BUG: Potentially dangeruous use of stat/chown/chmod. Since we use the
string tty_name for all calls on the tty device it is possible for
someone to be underhanded and use these calls and that string
to elevate priviledges.
FIX: Switch to using fstat/fchown/fchmod and obtain a file
descriptor instead of using a string.
NOTE: I doubt this can be exploited. But why leave it hanging around.
--- sshpty.c old
+++ sshpty.c new
@@ -356,6 +356,7 @@ void
pty_setowner(struct passwd *pw, const char *tty_name)
{
struct group *grp;
+ FILE* ttyfd;
gid_t gid;
mode_t mode;
struct stat st;
@@ -375,21 +376,25 @@ pty_setowner(struct passwd *pw, const ch
* Warn but continue if filesystem is read-only and the uids match/
* tty is owned by root.
*/
- if (stat(tty_name, &st)) {
- dropbear_exit("pty_setowner: stat(%.101s) failed: %.100s",
+ if (!(ttyfd = fopen(tty_name, "w+"))) {
+ dropbear_exit("pty_setowner: fopen(%.101s) failed: %.100s",
+ tty_name, strerror(errno));
+ }
+ if (fstat(ttyfd, &st)) {
+ dropbear_exit("pty_setowner: fstat(%.101s) failed: %.100s",
tty_name, strerror(errno));
}
if (st.st_uid != pw->pw_uid || st.st_gid != gid) {
- if (chown(tty_name, pw->pw_uid, gid) < 0) {
+ if (fchown(ttyfd, pw->pw_uid, gid) < 0) {
if (errno == EROFS &&
(st.st_uid == pw->pw_uid || st.st_uid == 0)) {
dropbear_log(LOG_ERR,
- "chown(%.100s, %u, %u) failed: %.100s",
+ "fchown(%.100s, %u, %u) failed: %.100s",
tty_name, (unsigned
int)pw->pw_uid, (unsigned int)gid,
strerror(errno));
} else {
- dropbear_exit("chown(%.100s, %u, %u) failed:
%.100s",
+ dropbear_exit("fchown(%.100s, %u, %u) failed:
%.100s",
tty_name, (unsigned int)pw->pw_uid,
(unsigned int)gid,
strerror(errno));
}
@@ -397,16 +402,18 @@ pty_setowner(struct passwd *pw, const ch
}
if ((st.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != mode) {
- if (chmod(tty_name, mode) < 0) {
+ if (fchmod(ttyfd, mode) < 0) {
if (errno == EROFS &&
(st.st_mode & (S_IRGRP | S_IROTH)) == 0) {
dropbear_log(LOG_ERR,
- "chmod(%.100s, 0%o) failed: %.100s",
+ "fchmod(%.100s, 0%o) failed: %.100s",
tty_name, mode, strerror(errno));
} else {
- dropbear_exit("chmod(%.100s, 0%o) failed:
%.100s",
+ dropbear_exit("fchmod(%.100s, 0%o) failed:
%.100s",
tty_name, mode, strerror(errno));
}
}
}
+ if (ttyfd)
+ fclose(ttyfd);
}
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "sshpty.c"
# from [5f45b06f6c68c3a4b163a7caf5efd681cd2dd7b8]
# to [ead81d13baa32bcdb4394eb04e920b289e52df06]
#
BUG: Since exit is set to NULL at the end of the loop, it is possible to
dereference exit as NULL (and segfault).
FIX: Co-opt the check for the race condition to make sure that exit
points at something valid.
--- svr-chansession.c old
+++ svr-chansession.c new
@@ -99,7 +99,7 @@ static void sesssigchild_handler(int UNU
/* If the pid wasn't matched, then we might have hit the race
mentioned
* above. So we just store the info for the parent to deal with
*/
- if (i == svr_ses.childpidsize) {
+ if (i == svr_ses.childpidsize || !exit) {
exit = &svr_ses.lastexit;
}
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "svr-chansession.c"
# from [030d10c1bd3ddf7d5e1a85165ee24685214a6b48]
# to [609b0035bc1da3b426ac09adf1780a03aaf6c70e]
#
BUG: buf_getmpint() is not checked to see if it worked.
FIX: Add a check and exit if it doesn't.
--- svr-kex.c old
+++ svr-kex.c new
@@ -52,7 +52,8 @@ void recv_msg_kexdh_init() {
}
m_mp_init(&dh_e);
- buf_getmpint(ses.payload, &dh_e);
+ if (buf_getmpint(ses.payload, &dh_e) != DROPBEAR_SUCCESS)
+ dropbear_exit("Unable to retrieve mp_int from buffer");
send_msg_kexdh_reply(&dh_e);
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "svr-kex.c"
# from [307e41123e5a9962093a7285f73a2400916bf526]
# to [0e5589d22c3cc3c0fc1e84a13d4734c463ac3e9f]
#
BUG: If you use the pointer that is obtained from tcpinfo and tcpinfo is NULL
this call will segfault.
FIX: Use the pointer var bindaddr instead since it is the same thing.
--- svr-tcpfwd.c old
+++ svr-tcpfwd.c new
@@ -216,7 +216,7 @@ out:
if (ret == DROPBEAR_FAILURE) {
/* we only free it if a listener wasn't created, since the
listener
* has to remember it if it's to be cancelled */
- m_free(tcpinfo->listenaddr);
+ m_free(bindaddr);
m_free(tcpinfo);
}
TRACE(("leave remotetcpreq"))
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "svr-tcpfwd.c"
# from [aa3d9bd98b61f4a84f5b28b1debc66a2b398045b]
# to [dba94b5f63e6e5cfcc4eb61809f890715c8eacf5]
#
BUG: Since listen_tcpfwd() does not free the buffer that tcpinfo
points to when it notices a check in a previous block of code, it
is inconsistent to free tcpinfo in this check. Especially since the
call is responsible for giving a valid pointer to listen_tcpfwd().
FIX: Remove the m_free(tcpinfo); call so that listen_tcpfwd()
always behaves the same when bailing out.
--- tcp-accept.c old
+++ tcp-accept.c new
@@ -131,7 +131,6 @@ int listen_tcpfwd(struct TCPListener* tc
tcp_acceptor, cleanup_tcp);
if (listener == NULL) {
- m_free(tcpinfo);
TRACE(("leave listen_tcpfwd: listener failed"))
return DROPBEAR_FAILURE;
}
#
# old_revision [4c32ad8064e93b83c6d635d193b34d5f9112d1ae]
#
# patch "tcp-accept.c"
# from [30c320bfdcfb7fd4dfbb35d8af5afdb1778f1d7d]
# to [69b03a9fd536daae6f43bc740522947591247de0]
#
