[PATCH] Fix segfault with restricted authorized_key files without forced command.

[Guilhem Moulin]

S $ sed -n '/ ssh-.*/{s///p; q}' ~/.ssh/authorized_keys
no-port-forwarding
S $ /usr/sbin/dropbear -r /tmp/dropbear.key -svEF -p 127.0.0.1:
[…]
[6773] Jun 22 01:56:38 Port forwarding disabled.
[…]
[6773] Jun 22 01:56:38 Port forwarding disabled.
[…]
[6773] Jun 22 01:56:38 Pubkey auth succeeded for 'guilhem' with key …
TRACE  (6773) 1434930998.973669: enter chansessionrequest
TRACE  (6773) 1434930998.973688: type is shell
TRACE  (6773) 1434930998.973712: enter sessioncommand
Aiee, segfault! You should probably report this as a bug to the developer

C $ ssh -oStrictHostKeyChecking=no -oUserKnownHostsFile=/dev/null -p  
localhost
Warning: Permanently added '[localhost]:' (RSA) to the list of known 
hosts.
Connection to localhost closed by remote host.
Connection to localhost closed.
---
 svr-authpubkeyoptions.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/svr-authpubkeyoptions.c b/svr-authpubkeyoptions.c
index c296141..9bdf99d 100644
--- a/svr-authpubkeyoptions.c
+++ b/svr-authpubkeyoptions.c
@@ -91,7 +91,7 @@ int svr_pubkey_allows_pty() {
 /* Set chansession command to the one forced 
  * by any 'command' public key option. */
 void svr_pubkey_set_forced_command(struct ChanSess *chansess) {
-   if (ses.authstate.pubkey_options) {
+   if (ses.authstate.pubkey_options && 
ses.authstate.pubkey_options->forced_command) {
if (chansess->cmd) {
/* original_command takes ownership */
chansess->original_command = chansess->cmd;
-- 
2.1.4

[PATCH] Fix typo in dropbear(8)'s manpage.

[Guilhem Moulin]

---
 dropbear.8 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/dropbear.8 b/dropbear.8
index 42f8ddb..3e05b08 100644
--- a/dropbear.8
+++ b/dropbear.8
@@ -100,8 +100,8 @@ Print the version
 .TP
 Authorized Keys
 
-~/.ssh/authorized_keys can be set up to allow remote login with a RSA or DSS
-key. Each line is of the form
+~/.ssh/authorized_keys can be set up to allow remote login with a DSS,
+RSA or ECDSA key. Each line is of the form
 .TP
 [restrictions] ssh-rsa B3NzaC1yc2EBIwAAAIgAsp... [comment]
 
@@ -139,7 +139,7 @@ Host Key Files
 
 Host key files are read at startup from a standard location, by default
 /etc/dropbear/dropbear_dss_host_key, /etc/dropbear/dropbear_rsa_host_key, and 
-/etc/dropbear/dropbear-ecdsa_host_key
+/etc/dropbear/dropbear_ecdsa_host_key
 or specified on the commandline with -r. These are of the form generated
 by dropbearkey. The -R option can be used to automatically generate keys
 in the default location - keys will be generated after startup when the first
-- 
2.1.4

Detached tarball signatures vs. clearsigned checksum files

[Guilhem Moulin]

Hi Matt,

I'm currently helping out packaging dropbear for Debian [0].  As
mentioned on your webpage the drobpear package is currently rather
outdated (even sid is lagging behind with 2014.65-1), and in order to
reduce the delays between upstream and package releases I'd like to make
the import of upstream tarballs easier.

Along with the most recent tarballs, one finds a clearsigned
SHA256SUM.asc file in https://matt.ucc.asn.au/dropbear/ .  Since
sha256sum(1) chokes on the OpenPGP header, in order to verify the
integrity of the package one needs to 1/ run `gpg --verify`, 2/ remove
the OpenPGP header & footer, and 3/ run `sha256sum -c`.

I wonder if you could provide a detached signature of the tarball
instead of clearsigning the checksum file.  While Debian's uscan(1) is
currently not able to deal with checksum files, it can import detached
signatures along with tarballs and check the signature validity.
(Furthermore it doesn't rely on the WoT since the signer's key is
available in the repository under ‘debian/upstream/signing-key.asc’.)

This would make importing further releases much easier :-)  In a
nutshell this is what I have in mind:

./dropbear-2015.67.tar.bz2
./dropbear-2015.67.tar.bz2.sig  (or .asc for armored files)
./SHA256SUM  (optional)

Also risking nitpicking, you could also modify your gpg(1) digest
preferences to something stronger than SHA1 [1] :-P  For instance:

echo 'personal-digest-preferences SHA512' >> ~/.gnupg/gpg.conf

Thanks!
Cheers,
-- 
Guilhem.

[0] https://lists.debian.org/debian-devel/2015/06/msg00285.html
[1] https://www.debian-administration.org/users/dkg/weblog/48


signature.asc
Description: Digital signature

Re: Detached tarball signatures vs. clearsigned checksum files

[Guilhem Moulin]

Hi,

On Mon, 29 Jun 2015 at 21:27:23 +0800, Matt Johnston wrote:
> New Debian packages would be great. I've signed
> releases/dropbear-2015.67.tar.bz2.sig for the latest
> one so far, I'll keep more for future releases.
> […]
> Making a new pgp key has been on my todo list so there is now
> a Dropbear Release Key. (The old key is DSA so seemed to
> only make SHA1 signatures)

That's great, thanks!  While I'm at it, please also consider excluding
mercurial dotfiles from the tarballs:



diff --git a/release.sh b/release.sh
index f377d0e..f2c6cad 100755
--- a/release.sh
+++ b/release.sh
@@ -27,7 +27,7 @@ if test -e $ARCHIVE; then
exit 1
 fi
 
-hg archive "$RELDIR"  || exit 2
+hg archive "$RELDIR" -X ".hg*" || exit 2
 
 (cd "$RELDIR" && autoconf && autoheader) || exit 2



(Not sure if you left the ‘./debian’ directory on purpose, but if not
you might want to exclude it as well.)

Cheers,
-- 
Guilhem.


signature.asc
Description: Digital signature

Re: Mercurial dotfiles (Was: Detached tarball signatures vs. clearsigned checksum files)

[Guilhem Moulin]

On Mon, 29 Jun 2015 at 22:06:20 +0800, Matt Johnston wrote:
> On Mon, Jun 29, 2015 at 03:51:54PM +0200, Guilhem Moulin wrote:
>> That's great, thanks!  While I'm at it, please also consider excluding
>> mercurial dotfiles from the tarballs:
> 
> Do they cause a problem? At least hg_archival.txt is kind of
> useful to see which hg revision made the tarball.

Nah I guess they are harmless; now that you say it hg_archival.txt can
be useful indeed, and in fact lintian(1) only complains about .hgtags:

https://lintian.debian.org/tags/source-contains-hg-tags-file.html

-- 
Guilhem.


signature.asc
Description: Digital signature

Re: Detached tarball signatures vs. clearsigned checksum files

[Guilhem Moulin]

On Mon, 29 Jun 2015 at 15:13:44 +0100, Andrea Bolandrina wrote:
> how do I remove myself from this mailing list?
> 
> There is no link at the bottom (or anywhere else)...

Yes, not in the body but in the headers:

List-Unsubscribe: 
,


-- 
Guilhem.


signature.asc
Description: Digital signature

Unexpected behavior of dbutil.c's expand_tilde functionality

[Guilhem Moulin]

Hi list,

Helmut Grohne reported the following [0]:

> I note that the new tilde expansion functionality in the upstream
> release is a bit strange. If my own home is "/home/helmut", it will
> expand "~/foo" to "/home/helmut//foo" (note the double slash).  Worse,
> it will expand expand "~otheruser/foo" to
> "/home/helmut/otheruser/foo", which does not match the general
> expectation of tilde expansion.

Not sure if that was intended or if that's a bug.  In the former case,
it might be helpful to explicitly write that ‘~foo’'s expansion isn't
that of a POSIX compliant shell [1], i.e., doesn't expand to user foo's
homedir as one could expect [2].

Cheers,
-- 
Guilhem.

[0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=790125#27
[1] 
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_01
[2] https://www.gnu.org/software/libc/manual/html_mono/libc.html#Tilde-Expansion


signature.asc
Description: Digital signature

/etc/motd is also printed on non-login shells if a TTY has been requested

[Guilhem Moulin]

Hi,

As of 2015.68, dropbear(8) says

   “By default the file /etc/motd will be printed for any login shell
(unless disabled at compile-time). This can also be disabled
per-user by creating  a file ~/.hushlogin .”

But in fact /etc/motd is printed whenever a TTY has been requested, even
when a command is executed on a non-login shell (explicitly or via an
authorized_key(5) restriction).  For instance, using the OpenSSH client,

ssh -t localhost -p  true

prints the Message Of The Day.  This is probably not the intended
behavior, as it messes up with the command's standard output hence make
it hard to use redirections.

Cheers,
-- 
Guilhem.


signature.asc
Description: PGP signature

svr_getopts should either support bundling or fail if bundling is used

[Guilhem Moulin]

Hi,

It's fine not to implement bundling in dropbear's option parsing
function (svr-runopts.c's svr_getopts), but it should at least croak if
argv[i][2] != '\0'.  For instance

dropbear -rdropbear.key -p127.0.0.1: -sjk

should either fail, or be parsed as

dropbear -r dropbear.key -p 127.0.0.1: -s -j -k

if bundling is allowed.


This might have security implications, as the current parsing mechanism
might make a user think that passing ‘-sjk’ disables port forwarding,
which is not the case (the trailing ‘jk’ is ignored).

Cheers,
-- 
Guilhem.


signature.asc
Description: PGP signature

[PATCH] Don't display the MOTD when an explicit command is run.

[Guilhem Moulin]

(possibly via authorized_keys(5) restrictions), even when a
pseudo-terminal has been allocated for the session.  In other words,
only display the MOTD when the server starts the user's default shell.
---
 svr-chansession.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/svr-chansession.c b/svr-chansession.c
index e44299e..bfaf7f6 100644
--- a/svr-chansession.c
+++ b/svr-chansession.c
@@ -814,7 +814,7 @@ static int ptycommand(struct Channel *channel, struct 
ChanSess *chansess) {
login_free_entry(li);
 
 #ifdef DO_MOTD
-   if (svr_opts.domotd) {
+   if (svr_opts.domotd && !chansess->cmd) {
/* don't show the motd if ~/.hushlogin exists */
 
/* 12 == strlen("/.hushlogin\0") */
-- 
2.6.1

Re: svr_getopts should either support bundling or fail if bundling is used

[Guilhem Moulin]

Hi Matt,

On Wed, 21 Oct 2015 at 22:11:43 +0800, Matt Johnston wrote:
> Thanks for pointing that out, I’ve made -sjk fail rather than be
> dropped silently.

Thanks.  However on second thought, the downside of this solution is
that it might render remote systems unreachable after upgrade (at least
for the users not reading changelogs or distrib NEWS files).  Worse, it
might not be noticed before a reboot, since upgrading typically doesn't
kill existing SSH connections.  (I've set “DROPBEAR_OPTIONS=-sjk”
myself, until last week where I noticed that only the first flag was
considered.)

Would you consider a patch to enable bundling instead?  If yes I'll try
to hack something up in the next couple of days.  By the way, out of
curiosity, is there a reason why you're not using getopt()?  It's POSIX
after all, and you're already using it for scp.

Cheers,
-- 
Guilhem.


signature.asc
Description: PGP signature

Re: svr_getopts should either support bundling or fail if bundling is used

[Guilhem Moulin]

On Thu, 22 Oct 2015 at 08:02:01 +0800, Matt Johnston wrote:
> On Thu 22/10/2015, at 1:21 am, Guilhem Moulin  wrote:
>> Thanks.  However on second thought, the downside of this solution is
>> that it might render remote systems unreachable after upgrade (at least
>> for the users not reading changelogs or distrib NEWS files).  Worse, it
>> might not be noticed before a reboot, since upgrading typically doesn't
>> kill existing SSH connections. 
> 
> Even enabling bundling could result in dropbear failing to start if
> there were trailing options that weren't valid.

Fair enough, but — assuming that distributions didn't change compiling
options, and that users didn't rely on dropbear ignoring trailing flags
— the failure would have to come from a typo, right?  Then I think it's
fine to croak, from a distro perspective at least.

> Perhaps I should just make the failure a warning instead, it'll be
> visible on "service dropbear restart"? 'Ignored extra trailing "jk" of
> "-sjk"'

Combined with a changelog entry (and a NEWS entry on the distro side),
that would indeed allow smooth upgrade.  However note that you can't
rely on the warning being visible on the error output since it depends
on the init system.  But AFAIK they all log in the system log when
available (hence not in the initrd).

>> By the way, out of curiosity, is there a reason why you're not using
>> getopt()?  It's POSIX after all, and you're already using it for scp.
> 
> I think I looked into it a long time ago and it resulted in a larger
> static binary size. It might be worth revisiting though. Backward
> compatibility would still be an issue.

Alright, I'll give it a try when time allows.  I don't understand your
last sentence though: backward compatibility for what?

-- 
Guilhem.


signature.asc
Description: PGP signature

[PATCH] Fix minor manpage formatting issues.

[Guilhem Moulin]

---
 dbclient.1| 11 +--
 dropbear.8|  9 -
 dropbearconvert.1 | 10 +-
 dropbearkey.1 |  7 ++-
 4 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/dbclient.1 b/dbclient.1
index c33f955..259c786 100644
--- a/dbclient.1
+++ b/dbclient.1
@@ -3,7 +3,7 @@
 dbclient \- lightweight SSH client
 .SH SYNOPSIS
 .B dbclient
-[flag arguments] [\-p
+[\fIflag arguments\fR] [\-p
 .I port\fR] [\-i
 .I id\fR] [\-L
 .I l\fR:\fIh\fR:\fIr\fR] [\-R
@@ -13,9 +13,8 @@ dbclient \- lightweight SSH client
 .RI [ command ]
 
 .B dbclient
-[
-.I args ]
-.I [user1]@host1[^port1],[user2]@host2[^port2],...
+[\fIargs\fR]
+[\fIuser1\fR]@\fIhost1\fR[^\fIport1\fR],[\fIuser2\fR]@\fIhost2\fR[^\fIport2\fR],...
 
 .SH DESCRIPTION
 .B dbclient
@@ -35,7 +34,7 @@ Read the identity key from file
 (multiple allowed). This file is created with dropbearkey(1) or converted
 from OpenSSH with dropbearconvert(1). The default path ~/.ssh/id_dropbear is 
used
 .TP
-.B \-L [\fIlistenaddress\fR]:\fIlistenport\fR:\fIhost\fR:\fIport\fR
+.B \-L\fR [\fIlistenaddress\fR]:\fIlistenport\fR:\fIhost\fR:\fIport\fR
 Local port forwarding.
 Forward the port
 .I listenport
@@ -44,7 +43,7 @@ on the local host through the SSH connection to port
 on the host
 .IR host .
 .TP
-.B \-R [\fIlistenaddress\fR]:\fIlistenport\fR:\fIhost\fR:\fIport\fR
+.B \-R\fR [\fIlistenaddress\fR]:\fIlistenport\fR:\fIhost\fR:\fIport\fR
 Remote port forwarding.
 Forward the port
 .I listenport
diff --git a/dropbear.8 b/dropbear.8
index 501cecf..71200d9 100644
--- a/dropbear.8
+++ b/dropbear.8
@@ -3,11 +3,10 @@
 dropbear \- lightweight SSH server
 .SH SYNOPSIS
 .B dropbear
-[flag arguments] [\-b
+[\fIflag arguments\fR] [\-b
 .I banner\fR] 
 [\-r
-.I hostkeyfile\fR] [\-p
-.IR [address:]port ]
+.I hostkeyfile\fR] [\-p [\fIaddress\fR:]\fIport\fR]
 .SH DESCRIPTION
 .B dropbear
 is a small SSH server 
@@ -54,7 +53,7 @@ Disable local port forwarding.
 .B \-k
 Disable remote port forwarding.
 .TP
-.B \-p \fI[address:]port
+.B \-p\fR [\fIaddress\fR:]\fIport
 Listen on specified 
 .I address
 and TCP
@@ -128,7 +127,7 @@ Disable PTY allocation. Note that a user can still obtain 
most of the
 same functionality with other means even if no-pty is set.
 
 .TP
-.B command="\fIforced_command\fR"
+.B command=\fR"\fIforced_command\fR"
 Disregard the command provided by the user and always run \fIforced_command\fR.
 
 The authorized_keys file and its containing ~/.ssh directory must only be
diff --git a/dropbearconvert.1 b/dropbearconvert.1
index b2f34ef..dd97ad4 100644
--- a/dropbearconvert.1
+++ b/dropbearconvert.1
@@ -21,24 +21,24 @@ from a private key by using
 .P
 Encrypted private keys are not supported, use ssh-keygen(1) to decrypt them
 first.
-.SH OPTIONS
+.SH ARGUMENTS
 .TP
-.B input type
+.I input_type
 Either 
 .I dropbear
 or 
 .I openssh
 .TP
-.B output type
+.I output_type
 Either 
 .I dropbear
 or 
 .I openssh
 .TP
-.B input file
+.I input_file
 An existing Dropbear or OpenSSH private key file
 .TP
-.B output file
+.I output_file
 The path to write the converted private key file. For client authentication 
~/.ssh/id_dropbear is loaded by default
 .SH EXAMPLE
  # dropbearconvert openssh dropbear ~/.ssh/id_rsa ~/.ssh/id_dropbear
diff --git a/dropbearkey.1 b/dropbearkey.1
index b4d202e..65dc9d2 100644
--- a/dropbearkey.1
+++ b/dropbearkey.1
@@ -12,10 +12,7 @@ dropbearkey \- create private keys for the use with 
dropbear(8) or dbclient(1)
 .SH DESCRIPTION
 .B dropbearkey
 generates a
-.I RSA 
-.I DSS,
-or
-.I ECDSA
+\fIRSA\fR, \fIDSS\fR, or \fIECDSA\fR
 format SSH private key, and saves it to a file for the use with the
 Dropbear client or server.
 Note that 
@@ -33,7 +30,7 @@ or
 .TP
 .B \-f \fIfile
 Write the secret key to the file
-.IR file . For client authentication ~/.ssh/id_dropbear is loaded by default
+\fIfile\fR. For client authentication ~/.ssh/id_dropbear is loaded by default
 .TP
 .B \-s \fIbits
 Set the key size to
-- 
2.6.2

Re: [PATCH] Fix minor manpage formatting issues.

[Guilhem Moulin]

Hi,

We've also got the two attached patches in the Debian package.  Please
consider applying them upstream.  (Actually both dropbear(8) and
dropbearconvert(1) mention the ‘-y’ flag for dropbearkey, but it's
currently undocumented in the upstream manpage.)

Cheers,
-- 
Guilhem.
From 5684f6a7c10660d5350b0f94a33dda93d615ffd8 Mon Sep 17 00:00:00 2001
From: Gerrit Pape 
Date: Wed, 19 Nov 2008 20:00:53 +
Subject: [PATCH] dbclient.1: dbclient uses compression if compiled with zlib
 support

On Fri, Sep 26, 2008 at 03:40:57PM +0200, Luca Capello wrote:
> On Tue, 23 Sep 2008 16:24:25 +0200, Matt Johnston wrote:
> > On Wed, Aug 20, 2008 at 07:43:58PM +0200, Luca Capello wrote:
> >> Could you please clarify if dbclient really support compression or not?
> >> If yes, could you please explain in the manpage how to enable it?
> >
> > As long as Dropbear was compiled with zlib support (which I
> > assume is the case for Debian)
>
> It seems so, at least the source build-depends on libz-dev and the
> binary depends on zlib1g.
>
> > dbclient will always use compression if the server supports it. There
> > isn't any option to disable it at the moment.
>
> Is there a way to test that compression is used?  I'm sorry, but I'm
> really not an expert about these stuff.
>
> Could the following patch against version 0.51 be applied to the
> dbclient manpage?
---
 dbclient.1 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/dbclient.1 b/dbclient.1
index 4502b23..7541517 100644
--- a/dbclient.1
+++ b/dbclient.1
@@ -20,6 +20,9 @@ dbclient \- lightweight SSH client
 .SH DESCRIPTION
 .B dbclient
 is a small SSH client 
+.P
+If compiled with zlib support and if the server supports it, dbclient will
+always use compression.
 .SH OPTIONS
 .TP
 .B \-p \fIport
-- 
2.0.1

From ee1b73673648c54e35828ee2bbae991f9b07cd57 Mon Sep 17 00:00:00 2001
From: Gerrit Pape 
Date: Wed, 19 Nov 2008 20:57:36 +
Subject: [PATCH] dropbearkey.8: mention -y option, add example

Patch from deb...@x.ray.net through
 http://bugs.debian.org/465903
---
 dropbearkey.1 | 9 +
 1 file changed, 9 insertions(+)

diff --git a/dropbearkey.1 b/dropbearkey.1
index 207a6fe..2419f26 100644
--- a/dropbearkey.1
+++ b/dropbearkey.1
@@ -9,6 +9,7 @@ dropbearkey \- create private keys for the use with dropbear(8) or dbclient(1)
 .I file
 [\-s
 .IR bits ]
+[\-y]
 .SH DESCRIPTION
 .B dropbearkey
 generates a
@@ -39,12 +40,19 @@ Write the secret key to the file
 Set the key size to
 .I bits
 bits, should be multiple of 8 (optional). 
+.TP
+.B \-y
+Just print the publickey and fingerprint for the private key in \fIfile\fR.
 .SH NOTES
 The program dropbearconvert(1) can be used to convert between Dropbear and OpenSSH key formats.
 .P
 Dropbear does not support encrypted keys. 
 .SH EXAMPLE
+generate a host-key:
  # dropbearkey -t rsa -f /etc/dropbear/dropbear_rsa_host_key
+
+extract a public key suitable for authorized_keys from private key:
+ # dropbearkey -y -f id_rsa | grep "^ssh-rsa " >> authorized_keys
 .SH AUTHOR
 Matt Johnston (m...@ucc.asn.au).
 .br
-- 
2.0.1



signature.asc
Description: PGP signature

[PATCH] Enable bundling in svr-runopts's svr_getopts.

[Guilhem Moulin]

On Wed, 28 Oct 2015 at 21:47:24 +0800, Matt Johnston wrote:
> I've changed the code to just print a warning for the time being. I'm
> intending for the next release to be soon with small bugfixes. Using getopt
> would probably be good though would require checking availability for the
> platforms where Dropbear is used.

In fact the current code can easily be tweaked to enable bundling.
(I've only touched svr-runopts for now; will proceed with cli-runopts if
you're fine with that patch.)  Refactoring the code to use getopt is
actually cumpersome due to the #ifdef changing the option string.

> By backwards compatibility I just meant the issue where the behaviour would
> change slightly.

---
 svr-runopts.c | 59 +++
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/svr-runopts.c b/svr-runopts.c
index 26c199b..ec29883 100644
--- a/svr-runopts.c
+++ b/svr-runopts.c
@@ -112,13 +112,14 @@ static void printhelp(const char * progname) {
 
 void svr_getopts(int argc, char ** argv) {
 
-   unsigned int i;
+   unsigned int i, j;
char ** next = 0;
int nextisport = 0;
char* recv_window_arg = NULL;
char* keepalive_arg = NULL;
char* idle_timeout_arg = NULL;
char* keyfile = NULL;
+   char c;
 
 
/* see printhelp() for options */
@@ -168,33 +169,10 @@ void svr_getopts(int argc, char ** argv) {
 #endif
 
for (i = 1; i < (unsigned int)argc; i++) {
-   if (nextisport) {
-   addportandaddress(argv[i]);
-   nextisport = 0;
-   continue;
-   }
- 
-   if (next) {
-   *next = argv[i];
-   if (*next == NULL) {
-   dropbear_exit("Invalid null argument");
-   }
-   next = 0x00;
+   if (argv[i][0] != '-' || argv[i][1] == '\0')
+   dropbear_exit("Invalid argument: %s", argv[i]);
 
-   if (keyfile) {
-   addhostkey(keyfile);
-   keyfile = NULL;
-   }
-   continue;
-   }
-
-   if (argv[i][0] == '-') {
-   char c = argv[i][1];
-   if (strlen(argv[i]) != 2) {
-   /* We only handle one flag per hyphen */
-   fprintf(stderr, "Warning, trailing '%s' of '%s' 
is ignored.\n",
-   &argv[i][2], argv[i]);
-   }
+   for (j = 1; (c = argv[i][j]) != '\0' && !next && !nextisport; 
j++) {
switch (c) {
case 'b':
next = &svr_opts.bannerfile;
@@ -284,12 +262,37 @@ void svr_getopts(int argc, char ** argv) {
exit(EXIT_SUCCESS);
break;
default:
-   fprintf(stderr, "Unknown argument 
%s\n", argv[i]);
+   fprintf(stderr, "Invalid option -%c\n", 
c);
printhelp(argv[0]);
exit(EXIT_FAILURE);
break;
}
}
+
+   if (!next && !nextisport)
+   continue;
+
+   if (c == '\0') {
+   i++;
+   j = 0;
+   }
+
+   if (nextisport) {
+   addportandaddress(&argv[i][j]);
+   nextisport = 0;
+   }
+   else if (next) {
+   *next = &argv[i][j];
+   if (*next == NULL) {
+   dropbear_exit("Invalid null argument");
+   }
+   next = 0x00;
+
+   if (keyfile) {
+   addhostkey(keyfile);
+   keyfile = NULL;
+   }
+   }
}
 
/* Set up listening ports */
-- 
2.6.2

Re: [PATCH] Enable bundling in svr-runopts's svr_getopts.

[Guilhem Moulin]

On Sat, 07 Nov 2015 at 00:03:14 +0800, Matt Johnston wrote:
> Thanks for the patch, it looks good. I've committed it with one small
> change (otherwise "dropbear -p" would segfault). I'll do the same for
> client unless you want to.

Awesome!  I don't have time this week-end, but I can have a look during
the next one.  Unless you've already done it, that is ;-)

-- 
Guilhem.


signature.asc
Description: PGP signature

[PATCH] Enable bundling for dbclient.

[Guilhem Moulin]

---
 cli-runopts.c | 202 ++
 1 file changed, 92 insertions(+), 110 deletions(-)

diff --git a/cli-runopts.c b/cli-runopts.c
index 2b0cb7d..59ebb5a 100644
--- a/cli-runopts.c
+++ b/cli-runopts.c
@@ -105,25 +105,30 @@ static void printhelp() {
 void cli_getopts(int argc, char ** argv) {
unsigned int i, j;
char ** next = 0;
-   unsigned int cmdlen;
+   enum {
 #ifdef ENABLE_CLI_PUBKEY_AUTH
-   int nextiskey = 0; /* A flag if the next argument is a keyfile */
+   OPT_AUTHKEY,
 #endif
 #ifdef ENABLE_CLI_LOCALTCPFWD
-   int nextislocal = 0;
+   OPT_LOCALTCPFWD,
 #endif
 #ifdef ENABLE_CLI_REMOTETCPFWD
-   int nextisremote = 0;
+   OPT_REMOTETCPFWD,
 #endif
 #ifdef ENABLE_CLI_NETCAT
-   int nextisnetcat = 0;
+   OPT_NETCAT,
 #endif
+   /* a flag (no arg) if 'next' is NULL, a string-valued option 
otherwise */
+   OPT_OTHER
+   } opt;
+   unsigned int cmdlen;
char* dummy = NULL; /* Not used for anything real */
 
char* recv_window_arg = NULL;
char* keepalive_arg = NULL;
char* idle_timeout_arg = NULL;
char *host_arg = NULL;
+   char c;
 
/* see printhelp() for options */
cli_opts.progname = argv[0];
@@ -172,58 +177,8 @@ void cli_getopts(int argc, char ** argv) {
 
fill_own_user();
 
-   /* Iterate all the arguments */
-   for (i = 1; i < (unsigned int)argc; i++) {
-#ifdef ENABLE_CLI_PUBKEY_AUTH
-   if (nextiskey) {
-   /* Load a hostkey since the previous argument was "-i" 
*/
-   loadidentityfile(argv[i], 1);
-   nextiskey = 0;
-   continue;
-   }
-#endif
-#ifdef ENABLE_CLI_REMOTETCPFWD
-   if (nextisremote) {
-   TRACE(("nextisremote true"))
-   addforward(argv[i], cli_opts.remotefwds);
-   nextisremote = 0;
-   continue;
-   }
-#endif
-#ifdef ENABLE_CLI_LOCALTCPFWD
-   if (nextislocal) {
-   TRACE(("nextislocal true"))
-   addforward(argv[i], cli_opts.localfwds);
-   nextislocal = 0;
-   continue;
-   }
-#endif
-#ifdef ENABLE_CLI_NETCAT
-   if (nextisnetcat) {
-   TRACE(("nextisnetcat true"))
-   add_netcat(argv[i]);
-   nextisnetcat = 0;
-   continue;
-   }
-#endif
-   if (next) {
-   /* The previous flag set a value to assign */
-   *next = argv[i];
-   if (*next == NULL) {
-   dropbear_exit("Invalid null argument");
-   }
-   next = NULL;
-   continue;
-   }
-
-   if (argv[i][0] == '-') {
-   /* A flag *waves* */
-   char c = argv[i][1];
-   if (strlen(argv[i]) != 2) {
-   /* We only handle one flag per hyphen */
-   fprintf(stderr, "Warning, trailing '%s' of '%s' 
is ignored.\n",
-   &argv[i][2], argv[i]);
-   }
+   for (i = 1; i < (unsigned int)argc && argv[i][0] == '-'; i++) {
+   for (j = 1; (c = argv[i][j]) != '\0' && !next && opt == 
OPT_OTHER; j++) {
switch (c) {
case 'y': /* always accept the remote hostkey */
if (cli_opts.always_accept_key) {
@@ -237,12 +192,7 @@ void cli_getopts(int argc, char ** argv) {
break;
 #ifdef ENABLE_CLI_PUBKEY_AUTH
case 'i': /* an identityfile */
-   /* Keep scp happy when it changes "-i 
file" to "-ifile" */
-   if (strlen(argv[i]) > 2) {
-   loadidentityfile(&argv[i][2], 
1);
-   } else  {
-   nextiskey = 1;
-   }
+   opt = OPT_AUTHKEY;
break;
 #endif
case 't': /* we want a pty */
@@ -262,7 +212,7 @@ void cli_getopts(int argc, char ** argv) {
break;
 #ifdef ENABLE_CLI_LOCALTCPFWD
case 'L':
-   nextislocal = 1;
+   opt = OPT_LOCALTCPFWD;
break;
  

Re: [PATCH] Fix minor manpage formatting issues.

[Guilhem Moulin]

On Wed, 28 Oct 2015 at 21:44:34 +0800, Matt Johnston wrote:
> Thanks, I've applied these.

In case you just missed it: FYI the patch at the root of this thread has
not been applied.

-- 
Guilhem.


signature.asc
Description: PGP signature

Syscall based entropy

[Guilhem Moulin]

Hi there,

N. Heninger, Z. Durumeric, E. Wustrow and A. Halderman have shown [0]
that a low entropy pool (for instance early in the boot process) during
key generation can lead to predictable keys.  (Worse, for DSA this can
lead to the exposure of the private host key material, but dropbear now
mitigates against this in dss.c:buf_put_dss_sign.)

Even when strong host keys have been generated using a properly seeded
entropy pool, I guess the early boot issue can bite again and yield
predictable ephemeral keys used in the Diffie-Hellman key exchange. 
(This looks especially bad when the server is installed in the initramfs
image to unlock the encrypted root partition for instance, as the server
is started very early in the initramfs stage.)

The problem arise because reading from /dev/urandom doesn't block when
the entropy pool hasn't been properly initialized.  As a workaround,
dbrandom.c:seedrandom implements its own logic to try to properly seed
the pool.  I wonder if you have considered to use getrandom(2) [1] on
Linux ≥3.17?  AFAICT it has the semantics one would naively expect from
reading from /dev/urandom.  I could provide with a patch if there is
interest.  (OpenBSD has its own getentropy() syscall, but I don't have
access to an OpenBSD box though.)

Cheers,
-- 
Guilhem.

[0] https://factorable.net/paper.html
[1] http://man7.org/linux/man-pages/man2/getrandom.2.html

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c6e9d6f38894798696f23c8084ca7edbf16ee895


signature.asc
Description: PGP signature

Re: Syscall based entropy

[Guilhem Moulin]

On Wed, 30 Dec 2015 at 22:08:14 +0800, Matt Johnston wrote:
> Using getrandom() is on my todo list - I'd be glad to take a
> patch.

Awesome!  I most likely won't have time to work on this during the next
couple of weeks, but I'll have a look at some point if you have not done
so already ;-)

> I think the best behaviour would be to call
> getrandom() on urandom with GRND_NONBLOCK in a loop
> printing a warning to dropbear_log() if it is blocking (not
> yet initialised) and keep waiting.

This is exactly what I've seen done elsewhere :-)  I'm curious of the
possibility of an infinite loop though, but there is only one way to
find out how long one has to wait in practice ;-)  I'm not familiar with
how the kernel fills its entropy pool, but I would hope it can use TCP
packets once network has been configured and a client tries to speak
with the SSH port, even when there is nothing listening on that port
yet.

> The extra sources in seedrandom() are purely opportunistic -
> better than nothing, though really it would be best if
> /dev/urandom blocked at boot until it's seeded (like getrandom()).

Yup

-- 
Guilhem.


signature.asc
Description: PGP signature

Re: Dropbear 2017.75

[Guilhem Moulin]

Hi Matt,

On Thu, 18 May 2017 at 23:02:09 +0800, Matt Johnston wrote:
> Dropbear 2017.75 is released. This has a couple of security
> fixes and a couple of bug fixes since 2016.74.

FYI https://matt.ucc.asn.au/dropbear/CHANGES yields 403 forbidden.

> - Security: Fix double-free in server TCP listener cleanup
>  A double-free in the server could be triggered by an authenticated user if
>  dropbear is running with -a (Allow connections to forwarded ports from any 
> host)
>  This could potentially allow arbitrary code execution as root by an 
> authenticated user.
>  Affects versions 2013.56 to 2016.74. Thanks to Mark Shepard for reporting 
> the crash.
> 
> - Security: Fix information disclosure with ~/.ssh/authorized_keys symlink.
>  Dropbear parsed authorized_keys as root, even if it were a symlink. The fix
>  is to switch to user permissions when opening authorized_keys
> 
>  A user could symlink their ~/.ssh/authorized_keys to a root-owned file they
>  couldn't normally read. If they managed to get that file to contain valid
>  authorized_keys with command= options it might be possible to read other
>  contents of that file.
>  This information disclosure is to an already authenticated user.
>  Thanks to Jann Horn of Google Project Zero for reporting this.

We're backporting these two to Debian Jessie (stable, soon to be
oldstable).  Did you already request CVE IDs?

Cheers,
-- 
Guilhem.


signature.asc
Description: PGP signature

Re: Cryptroot-unlock Timeout on askpass

[Guilhem Moulin]

Hi,

On Tue, 09 Jul 2019 at 09:02:53 +, Luke Flinders wrote:
> We have had the remote decrypting functioning for a while, however
> following recent updates to Debian it has now stopped working.
> […]
> I have had a look through your mailing archive and can not see this
> issue mentioned anywhere.

That feature isn't developed nor maintained by dropbear's upstream
maintainer, but by the Debian package maintainer (who happens to be me
right now).  Could please you file a bug against the ‘dropbear-initramfs’
package (or the ‘cryptsetup-initramfs’ package if you believe the problem
is in `cryptroot-look`) at the Debian BTS? https://bugs.debian.org/

We'll likely need some details about your setup.  reportbug(1) will
automatically fill a template with the relevant details.

Thanks!
Cheers,
-- 
Guilhem.


signature.asc
Description: PGP signature

Re: "Bad public key options" (Was: Dropbear 2020.79)

[Guilhem Moulin]

Hi Matt,

On Mon, 15 Jun 2020 at 23:52:51 +0800, Matt Johnston wrote:
> Dropbear 2020.79 is now released.

\o/ congrats!

> - […] x11 forwarding are now disabled by default.

I have no opinion about disabling this at compile-time, however the
current implementation locks out (“Bad public key options”) users with
‘no-X11-forwarding’ in their authorized_keys(5) files.

Wouldn't it make sense to move the #ifdefs to make the option a no-op
instead?  (Same thing for ‘no-agent-forwarding’ actually.)  Attached is
the patch I applied to “fix” the regression in the Debian package.

Cheers
-- 
Guilhem.
From: Guilhem Moulin 
Date: Tue, 16 Jun 2020 00:32:28 +0200
Subject: Don't choke on disabled authorized_keys(5) options

As of 2020.79 X11 forwarding is disabled at build time, which could lock
out users with authorized_keys(5) files containing ‘no-X11-forwarding’
options.

---
 svr-authpubkeyoptions.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/svr-authpubkeyoptions.c
+++ b/svr-authpubkeyoptions.c
@@ -147,20 +147,20 @@ int svr_add_pubkey_options(buffer *optio
 			ses.authstate.pubkey_options->no_port_forwarding_flag = 1;
 			goto next_option;
 		}
-#if DROPBEAR_SVR_AGENTFWD
 		if (match_option(options_buf, "no-agent-forwarding") == DROPBEAR_SUCCESS) {
+#if DROPBEAR_SVR_AGENTFWD
 			dropbear_log(LOG_WARNING, "Agent forwarding disabled.");
 			ses.authstate.pubkey_options->no_agent_forwarding_flag = 1;
+#endif
 			goto next_option;
 		}
-#endif
-#if DROPBEAR_X11FWD
 		if (match_option(options_buf, "no-X11-forwarding") == DROPBEAR_SUCCESS) {
+#if DROPBEAR_X11FWD
 			dropbear_log(LOG_WARNING, "X11 forwarding disabled.");
 			ses.authstate.pubkey_options->no_x11_forwarding_flag = 1;
+#endif
 			goto next_option;
 		}
-#endif
 		if (match_option(options_buf, "no-pty") == DROPBEAR_SUCCESS) {
 			dropbear_log(LOG_WARNING, "Pty allocation disabled.");
 			ses.authstate.pubkey_options->no_pty_flag = 1;


signature.asc
Description: PGP signature

Re: "Bad public key options"

[Guilhem Moulin]

On Wed, 17 Jun 2020 at 20:18:58 +0800, Matt Johnston wrote:
>> On Tue 16/6/2020, at 9:58 am, Guilhem Moulin  wrote:
>>> - […] x11 forwarding are now disabled by default.
>>
>> I have no opinion about disabling this at compile-time, however the
>> current implementation locks out (“Bad public key options”) users with
>> ‘no-X11-forwarding’ in their authorized_keys(5) files.
>
> Thanks, I'll apply that and organise a bug fix release (waiting to see
> if there are an other immediate regressions).

Awesome thanks :-)
 
> For Debian I think it might be worth keeping x11 forwarding enabled.
> I disabled x11 forwarding because most embedded platforms (Dropbear's
> most common usecase (?)) wouldn't have any use for it. On a general
> distro it can be useful.

I considered that before the upload: my gut feeling based on popcon and
bug reports to the Debian BTS is that most users of the Debian package
don't have X11 alongside the SSHd.  I mentioned the change in the NEWS
file; might reconsider if someone complains.

Would rather stick to the upstream compiled-in code as the rest is less
likely to be battle-tested :-P

-- 
Guilhem.


signature.asc
Description: PGP signature

Patch: use a different $PATH for the root user

[Guilhem Moulin]

Hi Matt,

Received the attached patch from Raphael Hertzog  at
https://bugs.debian.org/903403 .  You wrote in the bug report that you'd
apply the patch upstream but maybe that fell off-screen?  Forwarding to
the list for more visibility anyway. :-)

Cheers,
-- 
Guilhem.
--- a/default_options.h
+++ b/default_options.h
@@ -291,5 +291,6 @@ be overridden at runtime with -I. 0 disa
 
 /* The default path. This will often get replaced by the shell */
 #define DEFAULT_PATH "/usr/bin:/bin"
+#define DEFAULT_ROOT_PATH "/usr/sbin:/usr/bin:/sbin:/bin"
 
 #endif /* DROPBEAR_DEFAULT_OPTIONS_H_ */
--- a/svr-chansession.c
+++ b/svr-chansession.c
@@ -961,7 +961,11 @@ static void execchild(const void *user_d
 	addnewvar("LOGNAME", ses.authstate.pw_name);
 	addnewvar("HOME", ses.authstate.pw_dir);
 	addnewvar("SHELL", get_user_shell());
-	addnewvar("PATH", DEFAULT_PATH);
+	if (getuid() == 0) {
+	addnewvar("PATH", DEFAULT_ROOT_PATH); 
+	} else {
+	addnewvar("PATH", DEFAULT_PATH); 
+	}
 	if (chansess->term != NULL) {
 		addnewvar("TERM", chansess->term);
 	}


signature.asc
Description: PGP signature

[PATCH] Support running test/test_aslr.py without venv

[Guilhem Moulin]

Hi Matt,

Congrats for the new release! :-)  The compile-time tests are especially
welcome, even though unfortunately we can't enable them for Debian
packages since writing into ~/.ssh within build environments is
forbidden by Policy.  But instead we can run the pytest command as an
autopkgtest with the adequate Restriction field.

Since these tests are self contained we can't use virtualenv, and
without it pytest fails due a shell parsing error similar to

$ ; echo nay
bash: syntax error near unexpected token `;'

The fix is trivial.  Please consider applying
https://salsa.debian.org/debian/dropbear/-/blob/debian/latest/debian/patches/support-running-test_aslr-without-venv.patch
which I'm also attaching to this mail for convenience.

Thanks,
cheers
-- 
Guilhem.
From: Guilhem Moulin 
Date: Fri, 1 Apr 2022 23:27:50 +0200
Subject: Support running test_aslr without venv.

Without this patch the test fails because the remote shell can't parse
the command:

$ ; echo nay
bash: syntax error near unexpected token `;'
---
 test/test_aslr.py | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/test/test_aslr.py b/test/test_aslr.py
index ec38844..b2fa272 100644
--- a/test/test_aslr.py
+++ b/test/test_aslr.py
@@ -9,9 +9,12 @@ def test_reexec(request, dropbear):
 	This indicates that re-exec makes ASLR work
 	"""
 	map_script = (Path(request.node.fspath).parent / "parent_dropbear_map.py").resolve()
-	# run within the same venv, for python deps
 	activate = own_venv_command()
-	cmd = f"{activate}; {map_script}"
+	if activate == "":
+		cmd = map_script
+	else:
+		# run within the same venv, for python deps
+		cmd = f"{activate}; {map_script}"
 	print(cmd)
 	r = dbclient(request, cmd, capture_output=True, text=True)
 	map1 = r.stdout.rstrip()


signature.asc
Description: PGP signature

[PATCH] Fix build failure on hurd-i386.

[Guilhem Moulin]

GNU Hurd defines neither IOV_MAX nor UIO_MAXIOV.

---
 netio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/netio.c b/netio.c
index 2ed9bb1..294c239 100644
--- a/netio.c
+++ b/netio.c
@@ -304,7 +304,7 @@ void packet_queue_to_iovec(const struct Queue *queue, 
struct iovec *iov, unsigne
buffer *writebuf;
 
 #ifndef IOV_MAX
-   #if defined(__CYGWIN__) && !defined(UIO_MAXIOV)
+   #if (defined(__CYGWIN__) || defined(__GNU__)) && !defined(UIO_MAXIOV)
#define IOV_MAX 1024
#elif defined(__sgi)
#define IOV_MAX 512 
-- 
2.35.1