netutils/dropbear: initial Dropbear SSH server port for NuttX#3532
netutils/dropbear: initial Dropbear SSH server port for NuttX#3532FelipeMdeO wants to merge 1 commit into
Conversation
eae68b0 to
8778f48
Compare
|
@FelipeMdeO nice work! Kudos!!! Some suggestions to improve this PR:
|
| #define HAVE_STRUCT_ADDRINFO 1 | ||
| #define HAVE_STRUCT_IN6_ADDR 1 | ||
| #define HAVE_STRUCT_SOCKADDR_IN6 1 | ||
| #define HAVE_STRUCT_SOCKADDR_STORAGE 1 | ||
| #define HAVE_STRUCT_SOCKADDR_STORAGE_SS_FAMILY 1 |
There was a problem hiding this comment.
Some of these definitions cannot be hard-code, i.e.: HAVE_STRUCT_SOCKADDR_IN6, HAVE_STRUCT_SOCKADDR_IN6, could be disabled if IPv6 support on NuttX is not enabled.
There was a problem hiding this comment.
About HAVE_STRUCT_SOCKADDR_IN6:
The fake-rfc2553.h dropbear file has the source code below:
#ifndef HAVE_STRUCT_IN6_ADDR
struct in6_addr {
u_int8_t s6_addr[16];
};
#endif /* !HAVE_STRUCT_IN6_ADDR */
The issue is that the nuttx has the same struct in6_addr, so I need "disable" this code sector to be able compile.
Look NuttX file netinet/in.h, it always define s6_addr:
#define s6_addr in6_u.u6_addr8
#define s6_addr16 in6_u.u6_addr16
#define s6_addr32 in6_u.u6_addr32
I am using this macro equal 1 because s6_addr always exist in nuttx.
There was a problem hiding this comment.
@FelipeMdeO although the macro exist, I think it could cause some (silent?) error on dropbear if it assumes NuttX has the IPv6 support enabled, but it is disabled. What do you think @xiaoxiang781216 @mkj ?
There was a problem hiding this comment.
Hello @acassis , ff IPv6 is disabled, NuttX's getaddrinfo() only returns IPv4 addresses, so dropbear never attempts to open IPv6 sockets. If IPv6 is enabled, getaddrinfo() returns both families and dropbear handles both correctly since NuttX's struct in6_addr is fully compatible.
0cc194f to
49a5822
Compare
49a5822 to
9c4ff8e
Compare
|
I am reviewing your comments and doing improvement in the source code, I will ping you all to review again in the next days. tks. |
Hello @acassis, @xiaoxiang781216. Could you please confirm whether you would like me to contact the original author? I am concerned about his position regarding Xiaoxiang’s PR: mkj/dropbear#437 |
|
Guys, I opened the following discussion in the dropbear's repo: mkj/dropbear#440 |
9c4ff8e to
af8bba4
Compare
mkj
left a comment
There was a problem hiding this comment.
Good work getting it going! I suspect there are probably a few more cleanup edge cases that need dealing with, upstream Dropbear relies on exit() cleanup a fair bit. At least most of the postauth code should be in your own dropbear_nshsession.c to handle specially there.
I've added a few notes about upstream parts.
I don't think UNUSED() changes make sense to go upstream. It seems like it would be simpler to #undef UNUSED somewhere in an appropriate nuttx-dropbear header file? (Not sure where the NuttX definition comes from)
|
|
||
| signal_pipe[0] = ses.signal_pipe[0]; | ||
| signal_pipe[1] = ses.signal_pipe[1]; | ||
| session_cleanup(); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Hello mkj, nice catch.
This type of problem is critical.
NuttX has "tools" to handle memory properly but I will follow your suggestion to use DROPBEAR_TRACKING_MALLOC because it is more simple to apply.
You can check fix in commit sha: 7f37a59d8fef9648fadf8418ea36e76ae047454f
| #define dropbear_main dropbear_multi_entry | ||
| #include "dbutil.h" | ||
| #undef dropbear_main |
There was a problem hiding this comment.
| #define dropbear_main dropbear_multi_entry | |
| #include "dbutil.h" | |
| #undef dropbear_main | |
| #undef dropbear_main | |
| #include "dbutil.h" |
There was a problem hiding this comment.
Sorry, I can't apply this change.
I tried this change and unfortunately it breaks the build. See Below:
CC: inode/fs_inoderemove.c <command-line>: error: conflicting types for 'dropbear_main'; have 'int(int, char **)'
dropbear_main.c:258:5: note: in expansion of macro 'main'
258 | int main(int argc, FAR char *argv[])
| ^~~~
In file included from dropbear_main.c:24:
/Users/felipemouradeoliveira/nuttxspace/apps/netutils/dropbear/dropbear/src/dbutil.h:110:5: note: previous declaration of 'dropbear_main' with type 'int(int, char **, const char *)'
110 | int dropbear_main(int argc, char ** argv, const char * multipath);
| ^~~~~~~~~~~~~
The NuttX build system injects -Dmain=dropbear_main at compile time, but dbutil.h already declares dropbear_main with a different signature (3 args: argc, argv, multipath). That causes a conflicting types error when int main(int argc, ...) gets expanded.
The #define dropbear_main dropbear_multi_entry before the include is a trick to silently rename that declaration to dropbear_multi_entry during preprocessing, so the 3-arg version stays out of the way.
There was a problem hiding this comment.
@mkj mkj/dropbear#437 could fix this issue, please reconsider it.
There was a problem hiding this comment.
I think it's better to keep a patch in nuttx. -Dmain is pretty unusual.
Hi @mkj thank you very much for your review and suggestions. I think undefining UNUSED macro and defining it again could work, but could we agree at least in moving the UNUSED from been used in the function prototype and move it to end of the function, like here: https://github.com/apache/nuttx/blob/master/drivers/syslog/syslog_channel.c#L304 This way works better for old compilers (that don't know about attribute((unused)) ), since NuttX is used for retro-computing as well (not only microcontrollers). |
|
I'm not keen on changing Dropbear's style. On other compilers Dropbear makes it a no-op. #ifdef UNUSED
#elif defined(__GNUC__)
# define UNUSED(x) UNUSED_ ## x __attribute__((unused))
#elif defined(__LCLINT__)
# define UNUSED(x) /*@unused@*/ x
#else
# define UNUSED(x) x
#endif |
@FelipeMdeO I try a porting in the last week but doesn't require so many patch/hack to dropbear code base. So could you split the real change require to port dropbear from the pure improvement? |
Yes, I think we don't need to change the UNUSED() macro neither its position, since it won't be included into NuttX code base, but just compiled as external app. |
I tried the #undef UNUSED approach but it doesn't work in our setup. The problem is that dropbear_nshsession.c includes both Dropbear headers and nsh_console.h, which uses UNUSED() in a function body. I cannot find other solution. |
Hello @xiaoxiang781216, A significant portion of the changes (patches 0003–0005) are dedicated to replacing Dropbear's built-in crypto with NuttX's crypto libraries. Specifically: SHA-256/HMAC uses NuttX's mbedtls digest API, ChaCha20-Poly1305 uses NuttX's libtomcrypt state types directly, and password authentication delegates to NuttX's PAM/shadow stack. In other words the port will be more simple if we use crypto libs from Dropbear instead NuttX libs, but we decided use NuttX libs. |
8882d2b to
423dc59
Compare
9d674a3 to
b437112
Compare
b437112 to
bc982ed
Compare
acassis
left a comment
There was a problem hiding this comment.
@FelipeMdeO please move the Documentation to nuttx/Documentation/ repository. All Documentation there will be included in the official Documentation, i.e.: https://nuttx.apache.org/docs/latest/applications/graphics/jpegresizetool/index.html
|
@xiaoxiang781216 PTAL |
Should I open a dedicated PR for this documentation, or can I add documento in the following PR: apache/nuttx#19062? |
Hi Felipe, it is up to you, both ways are fine. Maybe adding it in a separated PR is better because you can have better granularity. For example, we could revert the esp32 PR without removing the Documentation about Dropbear |
|
Hello @acassis , PR documentation is available here: apache/nuttx#19190 |
| g_shell_returned = 0; | ||
| } | ||
|
|
||
| int dropbear_getgroups(int size, gid_t list[]) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Can we solve this in next PR round?
There was a problem hiding this comment.
but why need wait the next pr? it is a simple change.
There was a problem hiding this comment.
Ok, I believe this PR will be merged in the next hours, after this I will change my code.
There was a problem hiding this comment.
Sorry @xiaoxiang781216, I tried but couldn't apply this request.
That getgroups() is only used by Dropbear's non-multiuser sanity check, which expects it to fail with ENOSYS to confirm a non-multiuser kernel. getgrouplist() can't replace it. It's a group-database lookup that always returns at least the primary group passed in by the caller.
In short, I want to check if the kernel is single user, and getgrouplist returns other thing.
There was a problem hiding this comment.
Hello. Thinking better, I solve this issue using other approach and as result I can remove this stub.
I will use CONFIG_SCHED_USER_IDENTITY to know if we are running in single or multi user mode, this is all that I need.
| @@ -0,0 +1,305 @@ | |||
| /**************************************************************************** | |||
| * apps/netutils/dropbear/dropbear_main.c | |||
There was a problem hiding this comment.
why not use the implementation from dropbear directly?
There was a problem hiding this comment.
Because dropbear_main.c replaces main_noinetd() with a single persistent task that uses setjmp/longjmp to intercept dropbear_exit() and return to the accept loop after each session.
fork() is the central difference. svr-main.c:308 calls fork() to create a child process per connection. NuttX (flat build) does not support fork(), and the entire main_noinetd() is structured around it — without fork, the whole model breaks down.
There was a problem hiding this comment.
Yes, the block porting issue for MMU less system is fork, @mkj do you think it's a good idea to add an option which launch the new instance through posix_spawn? I can provide a general patch(work on major POSIX OS include NuttX), if you think it's a right direction.
There was a problem hiding this comment.
I don't think posix_spawn could do the equivalent of fexecve that Dropbear uses at the moment? (That's used so the spawned process is the same as what's running, to avoid having to worry about incompatibility if the binary gets upgraded). Could keep a posix_spawn version in nuttx though.
| return -1; | ||
| } | ||
|
|
||
| int link(FAR const char *path1, FAR const char *path2) |
There was a problem hiding this comment.
why add the dummy implementation which already provide by nuttx kernel?
There was a problem hiding this comment.
Dropbear uses link() in gensignkey.c:160 to atomically write the host key: it writes to a temp file, then calls link(tmp, final) expecting a hard link so both names reference the same inode. When tmp is later removed, the data survives through final. With a symlink instead, final would become a dangling pointer after tmp is deleted — silently losing the host key.
Therefore, the safe fallback (non-atomic write via buf_writefile) is only triggered when link() returns an error such as ENOSYS. If the symlink-based link() returns success, Dropbear proceeds assuming hard link semantics and the key gets lost.
For more details, please look for the code below netutils/dropbear/dropbear/src/gensignkey.c:154
(1) Key content is written to a temporary file only
ret = buf_writefile(buf, fn_temp, 0);
(2) link() is expected to create a hard link:
both fn_temp and filename reference the same inode.
If link() fails with ENOSYS/EPERM/EACCES, Dropbear falls back
to writing filename directly (buf_writefile).
If link() succeeds, Dropbear assumes filename already holds the
key via the shared inode — no additional write is done.
if (link(fn_temp, filename) < 0) {
if (errno == EPERM || errno == EACCES || errno == ENOSYS) {
/* safe fallback: write content directly to filename */
ret = buf_writefile(buf, filename, skip_exist);
}
goto out;
}
success path: key is accessible via filename through the hard link
fsync_parent_dir(filename);
out:
(3) fn_temp is always deleted here.
With a hard link: filename still holds the inode — key survives.
With a symlink: filename points to fn_temp, which is now
deleted — dangling symlink, key is lost.
unlink(fn_temp);
There was a problem hiding this comment.
Ok, since NuttX VFS doesn't support the hard link yet, so it's better to set errno to ENOSYS at https://github.com/apache/nuttx/blob/master/fs/vfs/fs_link.c#L63.
There was a problem hiding this comment.
Ok, I will take a look
There was a problem hiding this comment.
Hello @xiaoxiang781216, can you check this PR please: apache/nuttx#19212
9cd7233 to
d22da40
Compare
|
Hello @xiaoxiang781216 , I did most part of you requested, I tested and everything is ok. |
Thanks. |
|
@xiaoxiang781216 PTAL |
d22da40 to
beea0ce
Compare
|
Hello @xiaoxiang781216 , I finished the changes, however we need merge first the link change: |
Integrated SSH daemon authenticating against FSUTILS_PASSWD, with an ECDSA P-256 host key and an NSH session over a PTY per connection. Built from the upstream Dropbear tarball (pinned commit) and patched for NuttX, using Dropbear's bundled libtomcrypt for all crypto. setsid() (apache/nuttx#19184) and link() now come from NuttX, not local stubs. Signed-off-by: Felipe Moura <moura.fmo@gmail.com>
beea0ce to
f246afd
Compare
Summary
This PR adds two related changes that together bring up an SSH server
on the ESP32-C3 DevKit board using the Dropbear application:
boards/risc-v/esp32c3/esp32c3-devkit/configs/dropbear
A new
dropbeardefconfig is introduced for the ESP32-C3 DevKit board.It wires up the Dropbear SSH server application together with:
bring-up at boot).
/datamountpoint) to persist the host key andthe password database.
FSUTILS_PASSWDpointing to/data/passwdas the credential store,replacing a previous Dropbear-specific password-file path.
/data/dropbear_ecdsa_host_key.dropbeartask on every boot.sessions.
CONFIG_NETUTILS_DROPBEAR_STACKSIZEpinned to 65536 bytes; thedefault 32 KiB overflows during key exchange on this RISC-V target.
CONFIG_NETUTILS_DROPBEAR_LISTEN_RETRY_MAX=120so the daemon keepsretrying until the Wi-Fi link is fully up.
Wi-Fi credentials (
myssid/mypasswd) are placeholders and must beset via
menuconfigbefore flashing.crypto: expose ChaCha20 stream helpers
Dropbear uses the
chacha20-poly1305@openssh.comcipher, which requiresa stateful, multi-call ChaCha20 stream interface rather than the single-
block interface currently exposed by
crypto/chachapoly.c. Three helpersand a context struct are added:
struct chacha20_stream_ctx— opaque wrapper aroundchacha_ctx.chacha20_stream_setkey()— initialise the key.chacha20_stream_ivctr64()— set IV and 64-bit counter.chacha20_stream_crypt()— encrypt/decrypt an arbitrary-length buffer.All three functions are thin wrappers over the existing
chacha_*primitives; no new algorithm code is introduced.
Impact
dropbeardefconfig is additive anddoes not affect any existing configuration.
include/crypto/chachapoly.h. The change is purely additive; existingusers of
chacha20_setkey/chacha20_cryptare unaffected.CONFIG_NETUTILS_DROPBEAR.under
/data; they are generated at first run and persist acrossreboots. Wi-Fi credentials must be provisioned by the user before
flashing.
Testing
Host: Linux x86_64, GCC RISC-V toolchain
Board: ESP32-C3 DevKit (rev 0.4)
Build:
First-time user provisioning (serial console):
The NuttX passwd file lives on SPIFFS (
/data/passwd) and is empty on afresh flash. Before the first SSH login, create a user from the NSH
serial console:
The ECDSA host key is generated automatically on first boot.
Boot log shows Dropbear listening after Wi-Fi association:
SSH connection from the host: