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

nixos/sshd: add options for kexAlgorithms, ciphers and MACs #39232

Merged
merged 1 commit into from
Apr 21, 2018

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Apr 20, 2018

Motivation for this change

These settings were previously hardcoded in sshd's config. I noticed this because I needed to change them because of mssun/passforios#153 (comment).

Edit: The other way of adding defaults (config.services.sshd.kexAlgorithms = mkDefault [ ... ]) doesn't seem appropriate here, because the default wouldn't be visible in the man page and doing kexAlgorithms = [ ... ] in your config would add to the default ones instead of overriding the default, which seems slightly dangerous for security. To add to the default ones with the current config you need to use kexAlgorithms = options.services.sshd.kexAlgorithms.default ++ [ ... ]

Things done

Testing in progress..

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 20, 2018
@infinisil
Copy link
Member Author

Ping @jeaye @NeQuissimus @peterhoeg

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 20, 2018
@jeaye
Copy link
Contributor

jeaye commented Apr 20, 2018

Makes sense to me. Thanks for making this much more easily configurable.

@peterhoeg
Copy link
Member

Nice!

@peterhoeg peterhoeg merged commit e10718f into NixOS:master Apr 21, 2018
Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see comments)

default = [
"[email protected]"
"diffie-hellman-group-exchange-sha256"
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably revert this to not specify defaults for this option,

"aes256-ctr"
"aes192-ctr"
"aes128-ctr"
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this option,

MACs [email protected],[email protected],[email protected],hmac-sha2-512,hmac-sha2-256,[email protected]
KexAlgorithms ${concatStringsSep "," cfg.kexAlgorithms}
Ciphers ${concatStringsSep "," cfg.ciphers}
MACs ${concatStringsSep "," cfg.macs}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then only write these out if the options are specified.

This way, we don't have to maintain our own default list which will inevitably become out of sync with sshd's recommended options.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need a default, we can't just have everybody specify what algs to use in their config all the time. You couldn't just services.openssh.enable = true and have a working openssh anymore.

Copy link
Member

@grahamc grahamc Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They won't need to, from man ssh_config:

       KexAlgorithms
              Specifies the available  KEX  (Key  Exchange)  algo‐
              rithms.   Multiple  algorithms  must  be comma-sepa‐
              rated.  Alternately if the  specified  value  begins
              with  a  `+'  character,  then the specified methods
              will be appended  to  the  default  set  instead  of
              replacing  them.  If the specified value begins with
              a `-' character, then the specified methods (includ‐
              ing  wildcards) will be removed from the default set
              instead of replacing them.  The default is:

              curve25519-sha256,[email protected],
              ecdh-sha2-nistp256,ecdh-sha2-nistp384,ecdh-
              sha2-nistp521,
              diffie-hellman-group-exchange-sha256,
              diffie-hellman-group-exchange-sha1,
              diffie-hellman-group14-sha1

              The  list  of  available key exchange algorithms may
              also be obtained using Qq ssh -Q kex .

       Ciphers
              Specifies the ciphers allowed  and  their  order  of
              preference.   Multiple  ciphers  must be comma-sepa‐
              rated.  If the specified value  begins  with  a  `+'
              character,   then  the  specified  ciphers  will  be
              appended to the default  set  instead  of  replacing
              them.   If  the  specified  value  begins with a `-'
              character, then  the  specified  ciphers  (including
              wildcards)  will  be  removed  from  the default set
              instead of replacing them.

              The supported ciphers are:

              3des-cbc
              aes128-cbc
              aes192-cbc
              aes256-cbc
              aes128-ctr
              aes192-ctr
              aes256-ctr
              [email protected]
              [email protected]
              [email protected]

              The default is:

              [email protected],
              aes128-ctr,aes192-ctr,aes256-ctr,
              [email protected],[email protected],
              aes128-cbc,aes192-cbc,aes256-cbc

       MACs   Specifies  the  MAC  (message  authentication  code)
              algorithms in order of preference.   The  MAC  algo‐
              rithm is used for data integrity protection.  Multi‐
              ple algorithms  must  be  comma-separated.   If  the
              specified  value  begins  with a `+' character, then
              the specified algorithms will  be  appended  to  the
              default set instead of replacing them.  If the spec‐
              ified value begins with a `-'  character,  then  the
              specified  algorithms  (including wildcards) will be
              removed from the default set  instead  of  replacing
              them.

              The  algorithms  that  contain Qq -etm calculate the
              MAC after encryption (encrypt-then-mac).  These  are
              considered safer and their use recommended.

              The default is:

              [email protected],[email protected],
              [email protected],hmac-
              [email protected],
              [email protected],
              [email protected],[email protected],
              hmac-sha2-256,hmac-sha2-512,hmac-sha1

              The list of available MAC  algorithms  may  also  be
              obtained using Qq ssh -Q mac .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are insecure defaults, see https://stribika.github.io/2015/01/04/secure-secure-shell.html (linked from the options descriptions), and the PR that added such defaults originally: #31763

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @andir and @fpletz for an opinion here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I see it, these should either be provided by default, as they care now, or as a secure profile, which was the original discussion point which sparked me to originally do the work. However,

  1. We should maintain these settings in some profile, default or otherwise.
  2. Should the secure settings not be the default settings?

Yes, this is additional work for NixOS security maintenance, but it's better for the security of every NixOS user, by default, and it doesn't make anything harder for them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion is if they are in any profile it should be The Default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the point of the ageing cipher selection and having to maintain them. As @grahamc said we will "inevitably become out of sync" with whatever options one might deem as the holy grail of secure defaults.

BUT I also would argue that we need sane defaults. The defaults supplied by OpenSSH are probably not was many people would prefer.

I agree with Graham that if we have a "secure" profile that should be the default. If we had a "paranoid" profile that could be opt-in. If you have to downgrade your settings then you would disable the profile (services.openssh.server.profiles.default.enable = false, or some better option) and/or just define your custom list of algorithms.

How about we attempt to remove old (and "insecure") algorithms instead with the - qualifier? That would potentially allow new ciphers (which should be more secure?!?) to be picked up while we can easily disable ciphers as required.

I am not really happy with my proposal either. I would really like that the people dealing with the OpenSSH packaging would also maintain this list. They should come across changes in ciphersuites while reading the change logs anyway. The problem there is that it is yet another burden for a small number of people...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Maybe stupid) idea: have some tag we can grep for in the source code saying “This will need to be reviewed before XXXX-XX-XX and every Y months/years”, and have someone setup a cron to automatically warn when XXXX-XX-XX is passed without this tag being updated?

This would simplify maintenance of such lists, for which I guess OpenSSH wouldn't be the sole user. And I don't think there is a big hurry for upgrade (or else everyone will hear about it): from my limited experience, when a cipher breaks, it breaks slowly over time, and not in a few months. Like, everyone have known for a while that SHA1 is not really secure, yet it still stays relatively OK for quite a lot of use cases.

@infinisil infinisil deleted the sshd-options branch July 12, 2018 00:31
@peterhoeg
Copy link
Member

Instead of having to maintain a list of ciphers, which will be outdated as mentioned by grahamc, why not instead have our default settings blacklist the known bad ones using the "-" operator? This way when upstream adds "fancy-new-cipher", it will still be available and we are sure to block the bad ones.

@infinisil
Copy link
Member Author

@peterhoeg new != secure though, e.g. The NIST ones which were added not too long ago "can't be trusted" according to the article. Also new algs might be experimental and not heavily tested. If openssh only adds new ones to the defaults when they have been well reviewed, I'm onboard with that idea.

@peterhoeg
Copy link
Member

If openssh only adds new ones to the defaults when they have been well reviewed

I have no idea how they do this, but we essentially have 3 options:

  1. Leave it to upstream (no maintenance, but insecure)
  2. Maintain our own white-list which will get out of date
  3. Maintain our own black-list which may mean the introduction of possibly insecure ciphers

I think option 3 is "Least-Bad (tm)".

@pstn
Copy link
Contributor

pstn commented Aug 26, 2018

Could this please be back ported to stable? Currently there is no way to specify ciphers there and the defaults lead to breakage with syncorg git integration on android.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants