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

Enable OpenSSH for Windows #63

Closed
wants to merge 350 commits into from
Closed

Conversation

@manojampalam
Copy link

manojampalam commented Mar 16, 2017

Seeking feedback on changes in OpenSSH sources added to support Windows platforms. The changes are not complete yet (code reviews and significant testing are still pending) but since most of the main line functionality is in place, we thought we would check with experts on changes done so far and if they seem feasible for a possible integration upstream sometime in future. Please focus on changes within the root folder and in openbsd-compat.

Following is done to keep most of Unix-based OpenSSH source code intact and to make it work on Windows.

  • POSIX SDK and single threaded POSIX to Win32 adapter that implements
    • network, file, pipe and terminal IO (interrupt-able blocking and non-blocking).
    • file descriptors and select
    • Signals
    • Other misc POSIX calls applicable on Windows. Ex dirent and passwd routines among many others.
  • Unicode translation surface that handles UTF-8 <--> UTF-16 conversion between POSIX to Win32 API calls.
    • Windows command line UTF-16 input is captured on wmain entry point, converted to UTF-8 that gets fed to OpenSSH main() routines
    • POSIX and C runtime routines that exist on Windows are overridden to support UTF-8 based I/O.
  • A handful of Unix specific calls, that are not applicable on Windows are implemented as No-OPs.

That said, following lead to inclusion of some Windows specific code in OpenSSH sources

  • At an architectural level, Windows cannot support Unix like privilege separation model that heavily relies on fork() and setuid() calls. Fork() does not exist on Windows and setuid() does not map with how Windows security works. Also we have concluded that running ssh-agent in user session has some security implications (applicable in Unix too but more so on Windows). Based on this reasoning, we have rearchitected OpenSSH components in a way that's secure on Windows with minimal deviation in source code layout.
    • Sshd is implemented as a service running in low privileged mode.
    • A Windows version of ssh-agent is implemented as a service running as ROOT/SYSTEM. This apart from serving key requests (like Unix version does), also acts as a privileged broker for sshd. For Windows, ssh-agent (on the daemon side) also serves client authentication.
  • Fork() calls are replaced with a custom spawn_child call. In sshd, additional state information is added to identify and differentiate logic in parent (listener) and child (connection worker) processes.
  • Client authentication - Auth on Windows ends up with a user security token. Subsystem processes are run in the context of client user.
  • POSIX calls based on uid and gid that cannot be mapped to Windows that has a more complex identity representation.
  • Differences between Unix and Windows file/dir paths.

The changes done so far ensured that the original code for Unix remains untouched. This is so as to not cause any regressions in Unix land and to ensure easier review of the initial set of changes.

@AkihiroSuda AkihiroSuda mentioned this pull request Mar 29, 2017
2 of 5 tasks complete
@djmdjm

This comment has been minimized.

Copy link
Contributor

djmdjm commented Apr 21, 2017

I've finally found some time to start looking at this. Some high-level comments first. Please keep in mind that I'm not very familiar with Windows programming and haven't done any for quite a few years.

I'm unclear why you re-used ssh-agent to perform what seems to be a very different role in the server. It seems to acting in a role that is analogous to the privilege separation monitor process on Unix. I can understand why you'd want a separate persistent process to handle parts of the authentication flow, but why built it in to ssh-agent? Could you bring back something more similar to OpenSSH Unix style privsep by making sshd.c:server_accept_loop() spawn two processes (monitor and unprivileged child) instead of one?

The authentication callouts to the ssh-agent process seem to get back a token pointer that is stored in authctxt->methoddata and later used in setting up the session. Given the token pointer is passed as a 32 bit value, how does this work on 64 bit systems?

Removing privilege separation and sandboxing is quite a loss IMO. I guess splitting some functions into ssh-agent provides some of the benefits of privilege separation but it still seems like a subset of the protections it provides, e.g. in Unix privilege separation the host's private keys are not directly available to the low-privilege process. This doesn't seem to be the case with the current ssh-agent arrangement (though it could be). Also, I think (remember that my Win32 experience is years rusty though) that sandboxing in Unix OpenSSH more drastically limits what the low-privilege process can do - e.g. rendering it unable to open network connections and probe kernel attack surface for local privilege escalations.

One specific comment: process_authagent_request() in authagent-request.c doesn't check opn_len is sufficient before calling memcmp()

Generally, I don't see us being able to merge this patch any time soon because there are way too many ifdefs for us to maintain upstream given our general lack of experience developing Windows applications - we'd end up accidentally breaking Windows support too often for it to be viable.

That being said, I think we could work towards minimising large parts of it. A good place to start might be in subprocess handling - there are a few cases where we need to fire off subprocesses (e.g. sftp and scp). If we made a more convenient API to do so then we could probably avoid a bunch of ifdefs. We could use something like your spawn_child API as a basis...

@manojampalam

This comment has been minimized.

Copy link
Author

manojampalam commented Apr 24, 2017

Thanks @djmdjm for looking into this.

Here's some back ground. In Windows, it’s a standard practice to run any Network facing service under the context of NetworkService. This is a special low privilege machine account that (at least in theory) should do no harm to the system if its compromised from a security standpoint. Otherwise, Windows services rarely need to run as root. Authentication is carried over by talking to SSPs (GSSAPI provider equivalents) that are hosted by security subsystem. The end result of client authentication is a security token that represents the client user.

When I approached privilege separation for Windows, I had to weigh in its benefits vs its complexity. It does provide a more controlled and sandboxed environment to process untrusted network data. But due to the fact that there isn't a fork() equivalent in Windows, having a side by side implementation (using a spawn_child approach) is going to make it more complex from a code management stand point. By disabling the current "privilege separation code" entirely for Windows, I felt we can keep the assumptions clean and simple while introducing and enabling Windows support. Perhaps, we could look into complexity introduced by solving this problem for sshd initial fork (for connection worker) and reevaluate the feasibility of introducing privilege separation for Windows at a later point.

Otherwise, running the daemon as a less privileged service itself would provide us with reasonable security to start with. At this point we had to consider how to secure "host keys". For Windows, running ssh-agent as a root service solved this problem without introducing any common code changes or complexity. Ssh-agent for Windows can persist key registrations by securely storing them using user's or system's security context. To keep host keys secure on Windows, it is recommended to register them with ssh-agent. That said, the ssh-agent model itself is still intact and users can use other agents of their choice (by setting SSH_AUTH_SOCK).

The next problem to consider is authentication. To generate client user tokens for SSH key based authentication, one needs to be root. As you indicated, If privilege separation was enabled for Windows, and sshd was running as root, this could be a simple case of delegating a privileged operation to the monitor. But considering that this was just one and the only requirement for privilege escalation, I decided to go with something Windows specific - a high privileged key authentication agent. Since ssh-agent version for Windows already runs as root, I added the key authentication capability into it. I understand the confusion this resulted in. I will separate out the "key-management" agent and "key-authentication" agent end points. From sshd standpoint, it will open separate connections to these 2 agents. "key-management" agent will continue to be controlled by SSH_AUTH_SOCK while the "key-authentication" agent is going to be internal and Windows specific.

Regarding encoding of tokens as 32-bit values, this is a Windows artifact. Windows handles (Unix fd equivalents) though a (void*) type - they are guaranteed and can only contain 32-bit values. This is a requirement stemming from having the ability to pass handles between 32-bit and 64-bit processes.

manojampalam and others added 21 commits Apr 25, 2017
* shell: Cleanup shellhost (use func SendKeyStroke)

* shell: Fix console key mapping

Fix IntelliSense error:
a value of type "const char [6]" cannot be used to initialize an entity of type "char [5]"
…#115)

* shell: Add func FreeQueueEvent for correct uninitialize shell wrapper

* shell: Close all handles on exit
…ys, public keys. (#110)

1. Add file permission check when load or add ssh_config, authorized_keys, private keys, host keys,.
2. set the owner and ACE for create secure file, ex, private key in ssh-keygen.exe
3. Update script OpenSSHTestHelper.psm1 to be able to run Install-OpenSSH if the sshd is running on the machine.
4. add OpenSSHBinPath to path.
5. change indents in agentconfig.c
6. update test script to represent the changes
7. Add tests for:
* authorized_keys and ssh-keygen testing
* host keys file perm testing
* user private key file perm testing
* ssh-add test
* user ssh_config
RegEnumValueW may return ERROR_MORE_DATA (234).
If lpData is NULL and lpcbData is non-NULL, the function stores the size of the data, in bytes, in the variable pointed to by lpcbData. This enables an application to determine the best way to allocate a buffer for the data.
If the buffer specified by lpData is not large enough to hold the data, the function returns ERROR_MORE_DATA and stores the required buffer size in the variable pointed to by lpcbData. In this case, the contents of lpData are undefined.
Updated README for test case indexing guidelines, added ssh_config and updated existing test cases
bingbing8 and others added 22 commits Jun 19, 2018
… script (#324)

Add debug msg, replace API call incompatible with onecore , add build script
Escaped $ character within variable strings used in PROMPT environment variable to distinguish them the special characters used by the PROMPT command.
After creating a user token, the SeServiceLogonRight is now removed from the account so it does not create an orphaned reference in the local security policy.
Other small code changes for code style consistency within the file.

PowerShell/Win32-OpenSSH#1202
Current group membership resolution though very effective, is very slow. In a typical domain joined enterprise machine, adding a simple entry like the following in sshd_config
AllowGroups administrators
will incur a long delay in remote session establishment as sshd tried to pull all groups associated with the domain user.

Changes in this PR optimize the general case scenarios where no wild cards are in use. Specifically rules like this are processed promptly:

AllowGroups group1, group2, group3 //with no wild cards
Match Group group1 //single group with no negation and wild cards

Optimization is done by resolve the groupname in rule immediately to SID and checking its membership against user token. Enumerating the entire group membership is done on a lazy on-demand basis.

Beyond the optimization, there are 2 functional changes

- removed domain prefix for builtin groups
- removed domain prefix'ed versions of local groups since we are strictly following the convention that local principals shouldn't have any domain qualification.
Fix descriptor leaks in win32 fstat implementation
PowerShell/Win32-OpenSSH#1209

According to the docs for _open_osfhandle, _close will close the underlying handle:
"To close a file opened with , call _close. The underlying handle is also closed by a call to _close, so it is not necessary to call the Win32 function CloseHandle on the original handle"
Modified user principal name lookup to default to the implicit form (SamAccountName@DnsDomainName) if no explicit user principal name attribute is found on the account.

PowerShell/Win32-OpenSSH#1213
- Logic to support conpty (currently disabled until validation is complete)
- fdopen() and fchmod() support for file handles
- support for auto updating known_hosts via ssh and ssh-keygen
- Support for dynamic Windows-size changes with PTY
- Changes to support OneCore SDK
- Test cases
Cranked version 7.7.2.0
remove the window resize logic in ssh-shellhost.exe
Update vsts scripts to upload unit tests as artifacts
change the file to text type to show the diff
Fix of PowerShell/Win32-OpenSSH#1174 to grant non-admin permission to log events
Change the file type to text so it will show the diff in the future
Fix of PowerShell/Win32-OpenSSH#1139. Now user can build use solution file without manual steps
1. Added prebuildevent to copy libressl
2. When there is no '.git' in the environment, $psscriptroot is the default location to look for the solution and log file
Revert the isolation changes on Admin and Operational Channels. They are enable by default and setting them to custom isolation adds 2 more independent autologgers on the system.
 Added support of posix_spawnp.
1. fix of issue PowerShell/Win32-OpenSSH#1185
2. add End2End tests
…15473(#346)

 - Updated code to dynamic load Lsa functions until RS5 SDK includes them
 - Add conpty support in openssh
- Fixed Wierd characters (?25l) are seen, when logged in from ssh client
- Backspace doesn't work in powershell window
- Changes to support ssh-shellhost as an alternative shell
- Added support to have ssh-shellhost work as a standby shell (ssh-shellhost -c "cmdline") simply executes cmdline via CreateProcess
- Added E2E test cases and fixed unittests broken from prior changes
- Added PTY launch interface that supports both conpty and ssh-shellhost pty.
- Implemented PTY control channel in ssh-shellhost that supports Window resize events.
- Fixed regression with starting a PTY session with an explicit command
- modified ssh-shellhost pty argument to ---pty to remove ambiguity in cases when both -p and -c are present in commandline. Ex. ssh-shellhost.exe -c "myprogram -p -c argument"
@essentialexch

This comment has been minimized.

Copy link

essentialexch commented on auth-options.c in 1ce9e20 Oct 12, 2018

There is a memory leak here. A free(opt) is needed before the return.

This comment has been minimized.

Copy link
Collaborator

manojampalam replied Nov 5, 2018

can you please check if the issue exists in the upstream repo and report the issue if appropriate ? https://github.com/openssh/openssh-portable

This comment has been minimized.

Copy link

essentialexch replied Nov 5, 2018

Yes, the problem exists upstream. But I see no way to report problems other than submitting a patch. And I don't have the proper build environment for that.

djmdjm and others added 5 commits Oct 22, 2018
Patch from rosenp at gmail.com via openssh-unix-dev.
In Windows, unprivileged worker runs as a runtime generated virtual account. There should be no requirement to have a real account under the name of unprivileged user (sshd).
@manojampalam

This comment has been minimized.

Copy link
Author

manojampalam commented Nov 9, 2018

Closing this PR. Windows fork has moved on significantly since this version and is now architecturally aligned with Unix (specifically around privilege separation). Its POSIX compat library is now mostly compatible and we are following up with individual PRs to address Unix/Windows platform differences via platform abstraction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.