svr_getopts should either support bundling or fail if bundling is used
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
Re: svr_getopts should either support bundling or fail if bundling is used
Hi Guilhem, Thanks for pointing that out, I’ve made -sjk fail rather than be dropped silently. I’ve applied the other patch to avoid MOTD when there’s a command. Thanks, Matt > On Wed 14/10/2015, at 3:13 am, Guilhem Moulin wrote: > > 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.
Re: svr_getopts should either support bundling or fail if bundling is used
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
On Thu 22/10/2015, at 1:21 am, Guilhem Moulin wrote: > 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. Even enabling bundling could result in dropbear failing to start if there were trailing options that weren't valid. Perhaps I should just make the failure a warning instead, it'll be visible on "service dropbear restart"? 'Ignored extra trailing "jk" of "-sjk"' > 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. Cheers, Matt
Re: svr_getopts should either support bundling or fail if bundling is used
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
Re: svr_getopts should either support bundling or fail if bundling is used
> On Thu 22/10/2015, at 8:24 am, Guilhem Moulin wrote: > >>> 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? 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. By backwards compatibility I just meant the issue where the behaviour would change slightly. Cheers, Matt
