Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix plugin command line parsing logic #194

Closed
struanb opened this issue Nov 8, 2022 · 0 comments
Closed

Fix plugin command line parsing logic #194

struanb opened this issue Nov 8, 2022 · 0 comments

Comments

@struanb
Copy link

struanb commented Nov 8, 2022

When deploying dropbear with the libepka_file.so plugin from https://github.com/fabriziobertocci/dropbear-epka, I encountered intermittent failures in the plugin operating correctly.

Using strace, ltrace and then adding additional debug logging to dropbear and recompiling, I believe I have traced the issue to lines 450-461 of https://github.com/mkj/dropbear/blob/master/svr-runopts.c.

#if DROPBEAR_PLUGIN
        if (pubkey_plugin) {
            char *args = strchr(pubkey_plugin, ',');
            if (args) {
                *args='\0';
                ++args;
            }
            svr_opts.pubkey_plugin = pubkey_plugin;
            svr_opts.pubkey_plugin_options = args;
        }
#endif
}

On the face of it, this code looks sound, and it does work in the main dropbear process.

But when dropbear accepts an incoming ssh connection it forks a child, which reparses the command line. At this point pubkey_plugin appears to be NULL (I'm not sure why) and the , that the above code searches for has already been replaced with \0. And so, the parsing fails to identify the , and it assigns NULL to svr_opts.pubkey_plugin_options instead of the plugin's command line options.

To demonstrate, here is the output from dropbear with some debug logging:

#if DROPBEAR_PLUGIN
        if(svr_opts.pubkey_plugin != NULL) dropbear_log(LOG_INFO, "1:svr_opts.pubkey_plugin=[%s][%s]", svr_opts.pubkey_plugin, svr_opts.pubkey_plugin_options);

        if (pubkey_plugin) {
            dropbear_log(LOG_INFO, "pubkey_plugin=[%s][%s]", pubkey_plugin, pubkey_plugin + strlen(pubkey_plugin)+1);
            char *args = strchr(pubkey_plugin, ',');
            if (args) {
                *args='\0';
                ++args;
            }
            svr_opts.pubkey_plugin = pubkey_plugin;
            svr_opts.pubkey_plugin_options = args;
        }

        if(svr_opts.pubkey_plugin != NULL) dropbear_log(LOG_INFO, "2:svr_opts.pubkey_plugin=[%s][%s]", svr_opts.pubkey_plugin, svr_opts.pubkey_plugin_options);

#endif
}
bash-5.1# ./dropbear -A /root/dropbear-epka/src/.libs/libepka_file.so,/root/epka_file.json -R -E -F
[21832] Nov 08 10:23:18 pubkey_plugin=[/root/dropbear-epka/src/.libs/libepka_file.so,/root/epka_file.json][-R]
[21832] Nov 08 10:23:18 2:svr_opts.pubkey_plugin=[/root/dropbear-epka/src/.libs/libepka_file.so][/root/epka_file.json]
[21832] Nov 08 10:23:18 Not backgrounding
[21834] Nov 08 10:23:22 Child connection from 127.0.0.1:51574
[21834] Nov 08 10:23:22 pubkey_plugin=[/root/dropbear-epka/src/.libs/libepka_file.so][-R]
[21834] Nov 08 10:23:22 2:svr_opts.pubkey_plugin=[/root/dropbear-epka/src/.libs/libepka_file.so][(null)]
[fileauth] - missing auth file from options
[21834] Nov 08 10:23:22 Early exit from <127.0.0.1:51574>: external plugin initialization failed

At this stage I don't know if the underlying issue is that:

  1. the child process shouldn't be reparsing the command-line (if it should, I am curious to know why it should and why svr_opts.pubkey_plugin and svr_opts.pubkey_plugin_options do not appear to be inherited by the child from the parent); or
  2. the command-line parsing process should be repeatable and non-destructive of the input so that the child can reparse the command-line correctly.

P.S. Purely as a proof of issue, if I patch the code to assign a literal filepath to svr_opts.pubkey_plugin_options the plugin does work consistently.

@mkj mkj closed this as completed in ab6ea4d Nov 9, 2022
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Nov 15, 2022
Features and Changes:
  Note >> for compatibility/configuration changes

- >> Disable DROPBEAR_DSS by default
  It is only 1024 bit and uses sha1, most distros disable it by default already.

- Added DROPBEAR_RSA_SHA1 option to allow disabling sha1 rsa signatures.
  >> RSA with sha1 will be disabled in a future release (rsa keys will continue
  to work OK, with sha256 signatures used instead).

- Add option for requiring both password and pubkey (-t)
  Patch from Jackkal

- Add 'no-touch-required' and 'verify-required' options for sk keys
  Patch from Egor Duda

  - >> DROPBEAR_SK_KEYS config option now replaces separate DROPBEAR_SK_ECDSA
  and DROPBEAR_SK_ED25519 options.

- Add 'permitopen' option for authorized_keys to restrict forwarded ports
  Patch from Tuomas Haikarainen

- >> Added LTM_CFLAGS configure argument to set flags for building
  bundled libtommath. This also restores the previous arguments used
  in 2020.81 (-O3 -funroll-loops). That gives a big speedup for RSA
  key generation, which regressed in 2022.82.
  There is a tradeoff with code size, so -Os can be used if required.
  mkj/dropbear#174
  Reported by David Bernard

- Add '-z' flag to disable setting QoS traffic class. This may be necessary
  to work with broken networks or network drivers, exposed after changes to use
  AF21 in 2022.82
  mkj/dropbear#193
  Reported by yuhongwei380, patch from Petr Štetiar

- Allow overriding user shells with COMPAT_USER_SHELLS
  Based on a patch from Matt Robinson

- Improve permission error message
  Patch from k-kurematsu

- >> Remove HMAC_MD5 entirely

Regression fixes from 2022.82:

- Fix X11 build

- Fix build warning

- Fix compilation when disabling pubkey authentication
  Patch from MaxMougg

- Fix MAX_UNAUTH_CLIENTS regression
  Reported by ptpt52

- Avoid using slower prime testing in bundled libtomcrypt when DSS is disabled
  mkj/dropbear#174
  Suggested by Steffen Jaeckel

- Fix Dropbear plugin support
  mkj/dropbear#194
  Reported by Struan Bartlett

Other fixes:

- Fix long standing incorrect compression size check. Dropbear
  (client or server) would erroneously exit with
  "bad packet, oversized decompressed"
  when receiving a compressed packet of exactly the maximum size.

- Fix missing setsid() removed in 2020.79
  mkj/dropbear#180
  Reported and debugged by m5jt and David Bernard

- Try keyboard-interactive auth before password, in dbclient.
  This was unintentionally changed back in 2013
  mkj/dropbear#190
  Patch from Michele Giacomoli

- Drain the terminal when reading the fingerprint confirmation response
  mkj/dropbear#191
  Patch from Michele Giacomoli

- Fix utx wtmp variable typo. This has been wrong for a long time but
  only recently became a problem when wtmp was detected.
  mkj/dropbear#189
  Patch from Michele Giacomoli

- Improve configure test for hardening options.
  Fixes building on AIX
  mkj/dropbear#158

- Fix debian/dropbear.init newline
  From wulei-student

Infrastructure:

- Test off-by-default compile options
- Set -Wundef to catch typos in #if statements
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Dec 20, 2023
Include terrapin fix and bump PKGREVISION to make clear this
is not 2022.83.

2022.83 - 14 November 2022

Features and Changes:
  Note >> for compatibility/configuration changes

- >> Disable DROPBEAR_DSS by default
  It is only 1024 bit and uses sha1, most distros disable it by default already.

- Added DROPBEAR_RSA_SHA1 option to allow disabling sha1 rsa signatures.
  >> RSA with sha1 will be disabled in a future release (rsa keys will continue
  to work OK, with sha256 signatures used instead).

- Add option for requiring both password and pubkey (-t)
  Patch from Jackkal

- Add 'no-touch-required' and 'verify-required' options for sk keys
  Patch from Egor Duda

  - >> DROPBEAR_SK_KEYS config option now replaces separate DROPBEAR_SK_ECDSA
  and DROPBEAR_SK_ED25519 options.

- Add 'permitopen' option for authorized_keys to restrict forwarded ports
  Patch from Tuomas Haikarainen

- >> Added LTM_CFLAGS configure argument to set flags for building
  bundled libtommath. This also restores the previous arguments used
  in 2020.81 (-O3 -funroll-loops). That gives a big speedup for RSA
  key generation, which regressed in 2022.82.
  There is a tradeoff with code size, so -Os can be used if required.
  mkj/dropbear#174
  Reported by David Bernard

- Add '-z' flag to disable setting QoS traffic class. This may be necessary
  to work with broken networks or network drivers, exposed after changes to use
  AF21 in 2022.82
  mkj/dropbear#193
  Reported by yuhongwei380, patch from Petr Štetiar

- Allow overriding user shells with COMPAT_USER_SHELLS
  Based on a patch from Matt Robinson

- Improve permission error message
  Patch from k-kurematsu

- >> Remove HMAC_MD5 entirely

Regression fixes from 2022.82:

- Fix X11 build

- Fix build warning

- Fix compilation when disabling pubkey authentication
  Patch from MaxMougg

- Fix MAX_UNAUTH_CLIENTS regression
  Reported by ptpt52

- Avoid using slower prime testing in bundled libtomcrypt when DSS is disabled
  mkj/dropbear#174
  Suggested by Steffen Jaeckel

- Fix Dropbear plugin support
  mkj/dropbear#194
  Reported by Struan Bartlett

Other fixes:

- Fix long standing incorrect compression size check. Dropbear
  (client or server) would erroneously exit with
  "bad packet, oversized decompressed"
  when receiving a compressed packet of exactly the maximum size.

- Fix missing setsid() removed in 2020.79
  mkj/dropbear#180
  Reported and debugged by m5jt and David Bernard

- Try keyboard-interactive auth before password, in dbclient.
  This was unintentionally changed back in 2013
  mkj/dropbear#190
  Patch from Michele Giacomoli

- Drain the terminal when reading the fingerprint confirmation response
  mkj/dropbear#191
  Patch from Michele Giacomoli

- Fix utx wtmp variable typo. This has been wrong for a long time but
  only recently became a problem when wtmp was detected.
  mkj/dropbear#189
  Patch from Michele Giacomoli

- Improve configure test for hardening options.
  Fixes building on AIX
  mkj/dropbear#158

- Fix debian/dropbear.init newline
  From wulei-student

Infrastructure:

- Test off-by-default compile options

- Set -Wundef to catch typos in #if statements


2022.82 - 1 April 2022

Features and Changes:
  Note >> for compatibility/configuration changes

- Implemented OpenSSH format private key handling for dropbearconvert.
  Keys can be read in OpenSSH format or the old PEM format.
  >> Keys are now written in OpenSSH format rather than PEM.
  ED25519 support is now correct. DSS keys are still PEM format.

- Use SHA256 for key fingerprints

- >> Reworked -v verbose printing, specifying multiple times will increase
  verbosity. -vvvv is equivalent to the old DEBUG_TRACE -v level, it
  can be configured at compile time in localoptions.h (see default_options.h)
  Lower -v options can be used to check connection progress or algorithm
  negotiation.
  Thanks to Hans Harder for the implementation

  localoptions.h DEBUG_TRACE should be set to 4 for the same result as the
  previous DEBUG_TRACE 1.

- Added server support for U2F/FIDO keys (ecdsa-sk and ed25519-sk) in
  authorized_keys. no-touch-required option isn't allowed yet.
  Thanks to Egor Duda for the implementation

- autoconf output (configure script etc) is now committed to version control.
  >> It isn't necessary to run "autoconf" any more on a checkout.

- sha1 will be omitted from the build if KEX/signing/MAC algorithms don't
  require it. Instead sha256 is used for random number generation.
  See sysoptions.h to see which algorithms require which hashes.

- Set SSH_PUBKEYINFO environment variable based on the authorized_keys
  entry used for auth. The first word of the comment after the key is used
  (must only have characters a-z A-Z 0-9 .,_-+@)
  Patch from Hans Harder, modified by Matt Johnston

- Let dbclient multihop mode be used with '-J'.
  Patch from Hans Harder

- Allow home-directory relative paths ~/path for various settings
  and command line options.
  *_PRIV_FILENAME DROPBEAR_PIDFILE SFTPSERVER_PATH MOTD_FILENAME
  Thanks to Begley Brothers Inc

  >> The default DROPBEAR_DEFAULT_CLI_AUTHKEY has now changed, it now needs
  a tilde prefix.

- LANG environment variable is carried over from the Dropbear server process
  From Maxim Kochetkov

- Add /usr/sbin and /sbin to $PATH when logging in as root.
  Patch from Raphaël Hertzog
  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=903403

- Added client option "-o DisableTrivialAuth". It disallows a server immediately
  giving successful authentication (without presenting any password/pubkey prompt).
  This avoids a UI confusion issue where it may appear that the user is accepting
  a SSH agent prompt from their local machine, but are actually accepting a prompt
  sent immediately by the remote server.
  CVE-2021-36369 though the description there is a bit confused. It only applies
  to Dropbear as a client.
  Thanks to Manfred Kaiser from Austrian MilCERT

- Add -q client option to hide remote banner, from Hans Harder

- Add -e option to pass all server environment variables to child processes.
  This should be used with caution.
  Patch from Roland Vollgraf (github #118)

- >> Use DSCP for QoS traffic classes. Priority (tty) traffic is now set to
  AF21 "interactive". Previously TOS classes were used, they are not used by
  modern traffic classifiers. Non-tty traffic is left at default priority.

- >> Disable dh-group1 key exchange by default. It has been disabled server
  side by default since 2018.

- >> Removed Twofish cipher

Fixes:

- Fix flushing channel data when pty was allocated (github #85)
  Data wasn't completely transmitted at channel close.
  Reported and initial patch thanks to Yousong Zhou

- Dropbear now re-executes itself rather than just forking for each connection
  (only on Linux). This allows ASLR to randomise address space for each
  connection as a security mitigation. It should not have any visible impact
  - if there are any performance impacts in the wild please report it.

- Check authorized_keys permissions as the user, fixes NFS squash root.
  Patch from Chris Dragan (github #107)

- A missing home directory is now non-fatal, starting in / instead

- Fixed IPv6 [address]:port parsing for dbclient -b
  Reported by Fabio Molinari

- Improve error logging so that they are logged on the server rather than being
  sent to the client over the connection

- Max window size is increased to 10MB, more graceful fallback if it's invalid.

- Fix correctness of Dropbear's handling of global requests.
  Patch from Dirkjan Bussink

- Fix some small bugs found by fuzzers, null pointer dereference crash and leaks
  (post authentication)

- $HOME variable is used before /etc/passwd when expanding paths such as
  ~/.ssh/id_dropbear (for the client). Patch from Matt Robinson

- C89 build fixes from Guillaume Picquet

Infrastructure:

- Improvements to fuzzers. Added post-auth fuzzer, and a mutator that can
  handle the structure of SSH packet streams. Added cifuzz to run on commits
  and pull requests.
  Thanks to OSS-Fuzz for the tools/clusters and reward funding.

- Dropbear source tarballs generated by release.sh are now reproducible from a
  Git or Mercurial checkout, they will be identical on any system. Tested
  on ubuntu and macos.

- Added some integration testing using pytest. Currently this has tests
  for various channel handling edge cases, ASLR fork randomisation,
  dropbearconvert, and SSH_PUBKEYINFO

- Set up github actions. This runs the pytest suite and other checks.
  - build matrix includes c89, dropbearmulti, bundled libtom, macos, DEBUG_TRACE
  - test for configure script regeneration
  - build a tarball for external reproducibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants