ssh: Refactor, fix bugs & harden #3885
Conversation
|
LGTM. PR title seems to imply the current ssh profiles are broken. If this is not the case, please consider changing it to something like 'Refactor & harden ssh' or whatever you prefer. |
There was a problem hiding this comment.
Nice job. I've been running with these extra options added to my ssh-agent.local:
- private-cache
- private-dev
- memory-deny-write-execute
While you're working on our SSH profiles, can you test if those can be integrated into ssh-agent.profile? Especially mdrw would be a nice hardening to have IMO.
|
(I just noticed that there are some things missing, so I converted it to draft for now) |
And move the scattered `noblacklist ${HOME}/.ssh` entries into it.
Also, add it to profile.template, as reminded by @rusty-snake at
netblue30#3885 (review)
I'm working on adding the missing paths to disable-programs.inc. @glitsj16 commented 22 hours ago:
Ah, I didn't know what to put, so I just left in the branch name. The "fix" @glitsj16 left a comment:
Thanks.
I had started adding those and then I noticed that there a few more that can be @rusty-snake left a comment:
Thanks. Good catch; I didn't know about that list. I added that to the first |
Please do, we can always open a new thread to discuss potential hardening later on. |
That was added on the commit e93fbf3 ("disable ssh-agent sockets in disable-programs.inc"). Currently, it's the only ssh-related entry on disable-programs.inc. Further, it seems that all the other socket blacklists live on disable-common.inc. Also, even though this socket does not necessarily allow arbitrary command execution on the local machine (like some paths on disable-common.inc do), it could still do so for remote systems. Put it above the "top secret" section, like the terminal sockets are above the terminal server section.
And move the scattered `noblacklist ${HOME}/.ssh` entries into it.
Command used to find the relevant files:
$ grep -Fnr 'noblacklist ${HOME}/.ssh' etc
Also, add it to profile.template, as reminded by @rusty-snake at
netblue30#3885 (review)
|
Sorry for the delay, I was busy with IRL stuff. (rambling below) On my last reply I was stunned because I hadn't noticed the ssh auth socket Just in case that there was a reason for it to be on disable-programs.inc, I'm
I didn't rebase it to master to make it easier to see the changes, but I can do
Alright, thanks. |
|
Sorry, I just noticed a big problem:
And I suppose that I hadn't encountered this problem before because on my (not very |
I think you're making this hard on yourself. Suggestion:
|
Yeah, I guess so. The read-only part was an idea that I had long after I had
There is a git fetch origin
git log -p --reverse master..origin/pr/3885Or if you don't fetch PRs by default: git fetch origin pull/3885/head
git log -p --reverse master..FETCH_HEADI find it much easier to review things this way than through the web UI,
Agreed; I just removed the last commit and force-pushed. |
See etc/templates/profile.template.
And move the scattered `noblacklist ${HOME}/.ssh` entries into it.
Command used to find the relevant files:
$ grep -Fnr 'noblacklist ${HOME}/.ssh' etc
Also, add it to profile.template, as reminded by @rusty-snake at
netblue30#3885 (review)
This is the system-wide equivalent of ~/.ssh/config.
$ pacman -Q openssh
openssh 8.4p1-2
Reasons for blacklisting both /etc/ssh and /etc/ssh/* on
disable-common.inc:
Leave /etc/ssh that way so that profiles without allow-ssh.inc remain
unable to see inside of /etc/ssh. And blacklist /etc/ssh/* so that
profiles with allow-ssh.inc are able to access only nonblacklisted files
inside of /etc/ssh.
ssh_config (allowed on allow-ssh.inc) is the only file in /etc/ssh that
is used by ssh(1). The other paths are only used by sshd(8), so stop
allowing them on ssh.profile and ssh-agent.profile. Path examples from
sshd(8):
* /etc/ssh/moduli
* /etc/ssh/ssh_host_ecdsa_key
* /etc/ssh/ssh_host_ecdsa_key.pub
* /etc/ssh/ssh_known_hosts
* /etc/ssh/sshd_config
* /etc/ssh/sshrc
$ pacman -Q openssh
openssh 8.4p1-2
Leaving it limited to only ssh, ssh-agent and seahorse by default seems
unnecessarily restrictive.
From ssh(1):
> The most convenient way to use public key or certificate
> authentication may be with an authentication agent. See ssh-agent(1)
> and (optionally) the AddKeysToAgent directive in ssh_config(5) for
> more information.
$ pacman -Q openssh
openssh 8.4p1-2
With ssh-agent(1) running in the background (and with the private key(s)
loaded through ssh-add(1)), ssh(1) doesn't need direct access to the
actual key pair(s), so you could probably get away with this on
allow-ssh.local:
ignore noblacklist ${HOME}/.ssh
noblacklist ${HOME}/.ssh/config
noblacklist ${HOME}/.ssh/config.d
noblacklist ${HOME}/.ssh/known_hosts
And then this on the profiles of ssh key pair managers, such as
seahorse.local:
noblacklist ${HOME}/.ssh
The paths are taken from ssh(1) and sshd(8).
$ pacman -Q openssh
openssh 8.4p1-2
These are only used by sshd(8), so always blacklist them:
* ~/.rhosts: controls remote access to the local machine
* ~/.shosts: same as above
* ~/.ssh/authorized_keys: same as above
* ~/.ssh/authorized_keys2: same as above
* ~/.ssh/environment: potentially allows arbitrary command execution on
the local machine
* ~/.ssh/rc: allows arbitrary command execution on the local machine
* /etc/hosts.equiv: system-wide equivalent of ~/.rhosts
Note: There are files in /etc/ssh that are equivalent to some of the
above ones, but they are already blocked by `blacklist /etc/ssh/*`.
Note2: From sshd(8):
> If the file ~/.ssh/rc exists, sh(1) runs it after reading the
> environment files but before starting the user's shell or command.
So even if the user shell is set to /usr/bin/firejail and
disable-common.inc is loaded, this patch shouldn't interfere with sshd.
This file is actually used by ssh(1), so just mark it read-only:
* ~/.ssh/config: allows arbitrary command execution on the remote
machine (with e.g.: RemoteCommand) and also defines the connection
strength
Since version 7.3p1 (released on 2016-08-01), openssh supports including
other config files on ssh_config(5)[1][2]. This is the conventional
path for storing them[3], so mark it read-only:
* ~/.ssh/config.d: same as above
P.S. See also the explanation on the commit b5542fc
("disable-common.inc: read-only access to ~/.ssh/authorized_keys"),
which last touched/added the "Remote access" section.
[1]: https://anongit.mindrot.org/openssh.git/commit/?id=dc7990be865450574c7940c9880567f5d2555b37
[2]: https://www.openssh.com/txt/release-7.3
[3]: https://superuser.com/a/1142813
There was a problem hiding this comment.
Looks ready for merging to me. Do you want to add anything here or would you prefer seeing this merged and do other work in separate PR('s)?
By now you've seen that my git fu is very basic and I do use the Web UI mostly. Together with your earlier remarks about not squashing your commits I would like to suggest asking a more experienced collaborator with merge rights to proceed with this if that's all right. I really appreciate the git tips you gave but it'll take me some time to dig deeper into using git from CLI before I feel comfortable. And I don't want to mess up your nice work here. @rusty-snake What do you think, could you take care of merging this if you're satisfied with the latest changes?
It's been a while since I've been focused on the remaining commits, so I'd be By the way, could you mark the (from second to last) reviews about the missing
No worries, git takes a while to learn. I think the CLI is mostly about
Some that are good to know:
Networking commands:
And lastly the "dangerous" (and very useful) commands that "change history",
It sounds like a lot (and there are inconsistent commands/flags), but some From a high level perspective, for me there were two important milestones:
Anyway, if you get stuck feel free to mention or email me.
Sure; thanks for the consideration. The key point is to avoid the "squash and merge" option in the web UI. I'd |
|
@kmk3 Resolved the outstanding items as per your request. We encountered some CI issues lately but this one seems to have gone thru as expected. Thanks again for your work and the extra tips on using git from CLI, much appreciated. |
This partially reverts commit d94f547 ("disable all ssh utilities in disable-common.inc", 2023-08-20). Certain files in ~/.ssh are only used by sshd (not by ssh), so always blacklist them. Also, ssh itself does not need write access to the configuration files, so make them read-only by default. For details, see commit 2ec3f3a ("disable-common.inc: add missing openssh paths", 2021-01-09) / PR #3885. Cc: @netblue30
Note: As mentioned on #3845, please don't squash any commits; use normal merge
or rebase instead.