mkj commented on code in PR #3532:
URL: https://github.com/apache/nuttx-apps/pull/3532#discussion_r3413081667
##########
netutils/dropbear/patch/0002-use-nuttx-ecdsa-hostkey-sign.patch:
##########
@@ -0,0 +1,103 @@
+--- a/src/ecdsa.c
++++ b/src/ecdsa.c
+@@ -4,6 +4,9 @@
+ #include "ecc.h"
+ #include "ecdsa.h"
+ #include "signkey.h"
++#ifdef DROPBEAR_NUTTX
++#include "nuttx_hostkey.h"
++#endif
+
+ #if DROPBEAR_ECDSA
+
+@@ -67,10 +70,14 @@
+ , bit_size);
+ }
+
++#ifdef DROPBEAR_NUTTX
++ dropbear_exit("ECDSA key generation uses NuttX host key storage");
++#else
+ new_key = m_malloc(sizeof(*new_key));
+ if (ecc_make_key_ex(NULL, dropbear_ltc_prng, new_key, dp) != CRYPT_OK) {
+ dropbear_exit("ECC error");
+ }
++#endif
+ return new_key;
+ }
+
+@@ -164,8 +171,13 @@
+ /* Based on libtomcrypt's ecc_sign_hash but without the asn1 */
+ int err = DROPBEAR_FAILURE;
+ struct dropbear_ecc_curve *curve = NULL;
++#ifndef DROPBEAR_NUTTX
+ hash_state hs;
+ unsigned char hash[64];
++#endif
++#ifdef DROPBEAR_NUTTX
++ unsigned char nuttx_r[32], nuttx_s[32];
++#endif
+ void *e = NULL, *p = NULL, *s = NULL, *r;
+ char key_ident[30];
+ buffer *sigbuf = NULL;
+@@ -177,6 +189,31 @@
+ goto out;
+ }
+
++#ifdef DROPBEAR_NUTTX
++#if DROPBEAR_ECC_256
++ if (curve == &ecc_curve_nistp256) {
++ if (dropbear_hostkey_ecdsa_p256_sign(data_buf->data,
data_buf->len,
++ nuttx_r, nuttx_s) != DROPBEAR_SUCCESS) {
++ explicit_bzero(nuttx_r, sizeof(nuttx_r));
++ explicit_bzero(nuttx_s, sizeof(nuttx_s));
++ goto out;
++ }
++
++ if (mp_from_ubin(r, nuttx_r, sizeof(nuttx_r)) != MP_OKAY ||
++ mp_from_ubin(s, nuttx_s, sizeof(nuttx_s)) !=
MP_OKAY) {
++ explicit_bzero(nuttx_r, sizeof(nuttx_r));
++ explicit_bzero(nuttx_s, sizeof(nuttx_s));
++ goto out;
++ }
++
++ explicit_bzero(nuttx_r, sizeof(nuttx_r));
++ explicit_bzero(nuttx_s, sizeof(nuttx_s));
++ goto put_sig;
++ }
++#endif
++#endif
++
++#ifndef DROPBEAR_NUTTX
+ curve->hash_desc->init(&hs);
+ curve->hash_desc->process(&hs, data_buf->data, data_buf->len);
+ curve->hash_desc->done(&hs, hash);
+@@ -227,7 +264,11 @@
+ break;
+ }
+ }
++#else
++ dropbear_exit("Unsupported NuttX ECDSA curve");
++#endif
+
++put_sig:
+ snprintf(key_ident, sizeof(key_ident), "ecdsa-sha2-%s", curve->name);
+ buf_putstring(buf, key_ident, strlen(key_ident));
+ /* enough for nistp521 */
+--- a/src/sysoptions.h
++++ b/src/sysoptions.h
+@@ -161,9 +161,15 @@
+ #define LTM_DESC
+ #endif
+
++#ifndef DROPBEAR_ECC_256
+ #define DROPBEAR_ECC_256 (DROPBEAR_ECC)
++#endif
++#ifndef DROPBEAR_ECC_384
+ #define DROPBEAR_ECC_384 (DROPBEAR_ECC)
++#endif
++#ifndef DROPBEAR_ECC_521
+ #define DROPBEAR_ECC_521 (DROPBEAR_ECC)
++#endif
Review Comment:
This can go upstream
##########
netutils/dropbear/patch/0004-fix-nuttx-compile-warnings.patch:
##########
@@ -0,0 +1,107 @@
+--- a/src/common-kex.c
++++ b/src/common-kex.c
+@@ -117,7 +117,7 @@ void send_msg_kexinit() {
+
+ }
+
+-static void switch_keys() {
++static void switch_keys(void) {
+ TRACE2(("enter switch_keys"))
+ if (!(ses.kexstate.sentkexinit && ses.kexstate.recvkexinit)) {
+ dropbear_exit("Unexpected newkeys message");
+--- a/src/common-session.c
++++ b/src/common-session.c
+@@ -503,7 +503,7 @@ void ignore_recv_response() {
+ TRACE(("Ignored msg_request_response"))
+ }
+
+-static void send_msg_keepalive() {
++static void send_msg_keepalive(void) {
+ time_t old_time_idle = ses.last_packet_time_idle;
+ struct Channel *chan = get_any_ready_channel();
+
+--- a/src/compat.c
++++ b/src/compat.c
+@@ -81,6 +81,10 @@
+
+ #include "includes.h"
+
++#ifdef DROPBEAR_NUTTX
++int setsid(void);
++#endif
++
+ #ifndef HAVE_GETUSERSHELL
+ static char **curshell, **shells, *strings;
+ static char **initshells();
+--- a/src/dbrandom.c
++++ b/src/dbrandom.c
+@@ -129,7 +129,7 @@ static void addrandom(const unsigned char * buf, unsigned
int len) {
+ sha256_done(&hs, hashpool);
+ }
+
+-static void write_urandom()
++static void write_urandom(void)
+ {
+ #if DROPBEAR_FUZZ
+ if (fuzz.fuzzing) {
+--- a/src/dbutil.c
++++ b/src/dbutil.c
+@@ -66,6 +66,10 @@
+ #include "session.h"
+ #include "atomicio.h"
+
++#ifdef DROPBEAR_NUTTX
++int execv(const char *path, char * const argv[]);
++#endif
++
+ #define MAX_FMT 100
+
+ static void generic_dropbear_exit(int exitcode, const char* format,
+--- a/src/packet.c
++++ b/src/packet.c
+@@ -475,7 +475,7 @@ int packet_is_okay_for_queues(unsigned char type) {
+ return 1;
+ }
+
+-static void enqueue_reply_packet() {
++static void enqueue_reply_packet(void) {
+ struct packetlist * new_item = NULL;
+ new_item = m_malloc(sizeof(struct packetlist));
+ new_item->next = NULL;
+--- a/src/svr-runopts.c
++++ b/src/svr-runopts.c
+@@ -38,7 +38,7 @@ svr_runopts svr_opts; /* GLOBAL */
+ static void printhelp(const char * progname);
+ static void addportandaddress(const char* spec);
+ static void loadhostkey(const char *keyfile, int fatal_duplicate);
+ static void addhostkey(const char *keyfile);
+-static void load_banner();
++static void load_banner(void);
+
+ static void printhelp(const char * progname) {
+
+@@ -729,7 +729,7 @@ void load_all_hostkeys() {
+ }
+ }
+
+-static void load_banner() {
++static void load_banner(void) {
+ struct stat buf;
+ if (stat(svr_opts.bannerfile, &buf) != 0) {
+ dropbear_log(LOG_WARNING, "Error opening banner file '%s'",
+--- a/src/svr-tcpfwd.c
++++ b/src/svr-tcpfwd.c
+@@ -53,9 +53,13 @@ void recv_msg_global_request_remotetcp() {
+ /* */
+ #endif /* !DROPBEAR_SVR_REMOTETCPFWD */
+
++#if DROPBEAR_SVR_REMOTETCPFWD
+ static int svr_cancelremotetcp(void);
+ static int svr_remotetcpreq(int *allocated_listen_port);
++#endif
++#if DROPBEAR_SVR_LOCALTCPFWD
+ static int newtcpdirect(struct Channel * channel);
++#endif
Review Comment:
This has since changed upstream so might now be OK.
https://github.com/mkj/dropbear/pull/414
##########
netutils/dropbear/dropbear_main.c:
##########
@@ -0,0 +1,305 @@
+/****************************************************************************
+ * apps/netutils/dropbear/dropbear_main.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/socket.h>
+#include <setjmp.h>
+#include <stdarg.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include "includes.h"
+#include "algo.h"
+#include "crypto_desc.h"
+#define dropbear_main dropbear_multi_entry
+#include "dbutil.h"
+#undef dropbear_main
+#include "dbrandom.h"
+#include "netio.h"
+#include "runopts.h"
+#include "session.h"
+#include "ssh.h"
+#include "nuttx_hostkey.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define DROPBEAR_PORT_STRING_HELPER(n) #n
+#define DROPBEAR_PORT_STRING(n) DROPBEAR_PORT_STRING_HELPER(n)
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+typedef void (*dropbear_exit_handler_t)(int exitcode, FAR const char *format,
+ va_list param) ATTRIB_NORETURN;
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static jmp_buf g_session_exit_jmp;
+static int g_session_exitcode;
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+static void dropbear_session_exit(int exitcode, FAR const char *format,
+ va_list param) noreturn_function;
+static void dropbear_session_exit(int exitcode, FAR const char *format,
+ va_list param)
+{
+ char exitmsg[150];
+ char fullmsg[300];
+ char fromaddr[60];
+ int signal_pipe[2];
+
+ vsnprintf(exitmsg, sizeof(exitmsg), format, param);
+
+ fromaddr[0] = '\0';
+
+ if (svr_ses.addrstring != NULL)
+ {
+ snprintf(fromaddr, sizeof(fromaddr), " from <%s>", svr_ses.addrstring);
+ }
+
+ if (!ses.init_done)
+ {
+ snprintf(fullmsg, sizeof(fullmsg), "Early exit%s: %s", fromaddr,
+ exitmsg);
+ }
+ else if (ses.authstate.authdone)
+ {
+ snprintf(fullmsg, sizeof(fullmsg), "Exit (%s)%s: %s",
+ ses.authstate.pw_name, fromaddr, exitmsg);
+ }
+ else if (ses.authstate.pw_name != NULL)
+ {
+ snprintf(fullmsg, sizeof(fullmsg),
+ "Exit before auth%s: (user '%s', %u fails): %s",
+ fromaddr, ses.authstate.pw_name, ses.authstate.failcount,
+ exitmsg);
+ }
+ else
+ {
+ snprintf(fullmsg, sizeof(fullmsg), "Exit before auth%s: %s", fromaddr,
+ exitmsg);
+ }
+
+ dropbear_log(LOG_INFO, "%s", fullmsg);
+
+ signal_pipe[0] = ses.signal_pipe[0];
+ signal_pipe[1] = ses.signal_pipe[1];
+ session_cleanup();
Review Comment:
`session_cleanup()` _should_ be OK to clean up the session in the clean exit
case (the fuzzer tests for leaks), but memory won't be freed when
dropbear_exit() occurs for an error case. Those error cases can readily be
triggered by network traffic.
Dropbear's fuzzing harness had the same problem. As a usable hack it sets
`DROPBEAR_TRACKING_MALLOC` and then the harness calls `m_malloc_free_epoch()`.
https://github.com/mkj/dropbear/blob/master/FUZZER-NOTES.md#malloc-wrapper
I don't want to add any more complexity/API like that to upstream Dropbear
(it was never intended as a library), but you might be to patch something
similar in the NuttX wrapper? If NuttX has arena allocators (or similar) that
might be another option.
##########
netutils/dropbear/patch/0004-fix-nuttx-compile-warnings.patch:
##########
Review Comment:
The func(void) changes make sense to go upstream
##########
netutils/dropbear/dropbear_main.c:
##########
@@ -0,0 +1,305 @@
+/****************************************************************************
+ * apps/netutils/dropbear/dropbear_main.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/socket.h>
+#include <setjmp.h>
+#include <stdarg.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include "includes.h"
+#include "algo.h"
+#include "crypto_desc.h"
+#define dropbear_main dropbear_multi_entry
+#include "dbutil.h"
+#undef dropbear_main
Review Comment:
```suggestion
#undef dropbear_main
#include "dbutil.h"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
