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

Server implementation does not check for auth before serving later requests #1175

Closed
bitprophet opened this issue Mar 13, 2018 · 10 comments
Closed

Comments

@bitprophet
Copy link
Member

bitprophet commented Mar 13, 2018

What follows is a direct paste from a private gist used to workshop the issue a bit late last week; have completed tests/impl (from 1.17+) and will be pushing those shortly. (wanted the issue number set in stone first...:D)

We have a CVE for this issue: CVE-2018-7750.

Intro

Email from one Matthijs Kooijman (@matthijskooijman) dated 2018.03.02 notes that Paramiko's server implementation may be connected to by clients that do not implement the auth step, and happily serves up commands/etc to such un-authed clients. He found that AsyncSSH (another Python lib that does not use Paramiko) has the same issue. Finally, he states the RFC is unclear as to whether this is purposeful.

Let's double check both the RFCs and then our favorite reference implementation, OpenSSH.

Should neither provide a useful clue, my gut says the server implementation should track whether we've sent SSH_MSG_USERAUTH_SUCCESS (99% sure we already do track this) and default to rejecting any connection-level messages (like SSH_MSG_CHANNEL_OPEN or SSH_MSG_GLOBAL_REQUEST) unless that flag is True.

RFC scan

tl;dr it is indeed kinda vague, there are two kinda-disagreeing undercurrents, neither of which are ironclad:

  • It is assumed that the connection protocol (which is where command exec occurs) runs on top of / after setting up, the transport (initial kex/handshake/etc) and auth (user auth) protocols.
  • The specific server implementation, and/or operator of an instantiation of such an implementation, has significant leeway in how they implement and/or configure the server, re: when and how user-auth occurs.

Specifics:

  • RFC 4251 (protocol arch)

    • 1: pretty clear that the intent is that a user auth step always occurs, followed by a service request:

      The client sends a service request once a secure transport layer
      connection has been established. A second service request is sent after
      user authentication is complete.

    • 4.3: third bullet point re: policy issues that 'SHOULD' be addressed, highlights that auth specifics are up to the site/operator:

      The authentication methods that are to be required by the server for
      each user. The server's policy MAY require multiple authentication for
      some or all users. The required algorithms MAY depend on the location
      from where the user is trying to gain access.

    • 9.4.3: this whole section arguably applies, but it's very vague. However it seems to back up my hunch that at core, this is up to the server implementer and/or operator, e.g:

      At the discretion of the implementers, this default policy may be along
      the lines of anything-goes where there are no restrictions placed upon
      users [...]

  • RFC 4252 (auth protocol)

    • 4: more vague implications that the server can do whatever it wants, e.g. the below quote about none auth implies the authors at least partly considered servers that intentionally don't care about auth at all (though the specific discussion is about the actual, explicit use of the none auth type message, which is distinct from "did not submit auth at all"):

      The "none" method is reserved, and MUST NOT be listed as supported.
      However, it MAY be sent by the client. The server MUST always reject
      this request, unless the client is to be granted access without any
      authentication, in which case, the server MUST accept this request.

    • 5.3: this (like 4251.1) states that the server should start up the requested service after sending auth-success. One could read this to imply that services SHOULD NOT start UNLESS auth has occurred, but it's not explicit...

    • 7: notes that implementations MUST implement public-key auth, though I note this is distinct from requiring that it is enabled (clearly, many real servers only offer password auth, for example.)

  • RFC 4253 (transport protocol)

    • 10: implies client may request "a service" after initial (high level) kex, where that service is one of userauth or connection. The transport level of the protocol thus doesn't appear to actually care or enforce that one performs auth before connection.
  • RFC 4254 (connection protocol)

    • 1: once again implies that connection is "designed to" occur after/on top of auth.

    • 11: again, it's 'assumed':

      This protocol is assumed to run on top of a secure, authenticated
      transport. User authentication and protection against network-level
      attacks are assumed to be provided by the underlying protocols.

OpenSSH's implementation

My old friend and the only C codebase I have any familiarity with whatsoever, openssh-portable...

Synopsis

After all the below, the tl;dr seems to be:

  • OpenSSH sets up a dispatch table to determine how it responds to protocol messages/packets
  • This table gets reinitialized depending on 'phase' of execution: while awaiting auth, it is only set to respond to auth-related messages, then after successful auth, it retools the table to only respond to post-auth-related messages like channel opens.
  • Thus, the case under test ends up being a simple "What even are you talking about? What's a channel open?" NotImplementedError style situation - no auth step, no idea how to handle anything beyond auth.

Deep dive

Initial distillation

  • Seems at first that "huh, the user can get a session as long as they exist locally, without necessarily passing auth" which would be bad but would also act like Paramiko.
  • However, realized: that ->valid flag is set by input_userauth_request, meaning the client has to actually submit auth in order to set it. (Ditto ->pw.)
  • So if a user attempts to send a channel open request without authing, both authctxt->pw and authctxt->valid will be null, and thus session_open should call line 1767 and thus result in fatal("no user for session").

Testing with live OpenSSH server

Proving this with a live install is interesting:

  • Ran local docker container executing Ubuntu + OpenSSH 7.2 on port 2222 with nothing but root password auth by default

  • Executed Matthijs's client-test.py with nothing but the port number changed

  • Did not get expected no user for session but instead seem to have just confused the poor thing:

    debug1: SSH2_MSG_NEWKEYS received [preauth]
    debug1: KEX done [preauth]
    dispatch_protocol_error: type 90 seq 3 [preauth]
    debug1: Received SSH2_MSG_UNIMPLEMENTED for 3 [preauth]
    

Second dive

End result

  • The RFC isn't terrifically clear beyond "well, we kind of assume you're not gonna open channels and such unless you've already authed", but it's not a SHOULD or a MUST.
  • OpenSSH has chosen to implement this as a strict "only respond to the messages you can handle at the current stage" setup, where auth comes before connection (as in the RFC.) Trying to do things out-of-order results in a simple SSH_MSG_UNIMPLEMENTED.
  • Paramiko does not do things quite that way: instead, as one might guess from the bug description, it simply sets up all possible dispatch targets (anything implemented by the Transport across its two handler tables, and the AuthHandler handler table) and then dispatches depending on message type:

    paramiko/paramiko/transport.py

    Lines 1891 to 1917 in 27a8ed1

    if ptype in self._handler_table:
    self._handler_table[ptype](self, m)
    elif ptype in self._channel_handler_table:
    chanid = m.get_int()
    chan = self._channels.get(chanid)
    if chan is not None:
    self._channel_handler_table[ptype](chan, m)
    elif chanid in self.channels_seen:
    self._log(DEBUG, 'Ignoring message for dead channel {:d}'.format(chanid)) # noqa
    else:
    self._log(ERROR, 'Channel request for unknown channel {:d}'.format(chanid)) # noqa
    break
    elif (
    self.auth_handler is not None and
    ptype in self.auth_handler._handler_table
    ):
    handler = self.auth_handler._handler_table[ptype]
    handler(self.auth_handler, m)
    if len(self._expected_packet) > 0:
    continue
    else:
    err = 'Oops, unhandled type {:d}'.format(ptype)
    self._log(WARNING, err)
    msg = Message()
    msg.add_byte(cMSG_UNIMPLEMENTED)
    msg.add_int(m.seqno)
    self._send_message(msg)

Doing nothing certainly seems like a bad idea: this is clearly a massive security flaw, and the only reason I did all the above investigation is because software has an irritating history of "but I was relying on that bug / looseness in the spec / whatever!". Given the main reference implementation disallows it, I'm inclined to assume nobody could possibly rely on this.

So there's two obvious fixes for Paramiko:

  • The "OpenSSH is the Bible" approach: update Transport's dispatching to be more like OpenSSH's and only enable certain message types depending on the state of self.is_authenticated (which, impressively, appears to only ever be used in __repr__...!!!)
    • This could be problematic given that Transport is frustratingly bimodal and is used both for server and client operations - we'd have to make sure that we're not preventing a not-yet-authed client from dispatching on necessary responses because it's not authed yet, for example.
    • Of note, AsyncSSH is taking this approach in their fix, and are simply leveraging the fact that RFC 4250 compatible protocol numbers mean one can just go "is the message identifier greater than the highest possible auth related number? Are you authed yet? No? Screw off!" - seems possible on our end, though doesn't really change the previous point about ensuring client-side cases are protected.
  • The "that's too much work right now" approach: simply rub some more references to .auth_handler and/or .is_authenticated in specific spots such as Server.check_channel_request (here)
    • Except this, too, is problematic because of how Transport, Server and AuthHandler split up responsibilities & exposures to one another. By default a Server has no direct access to the Transport running it, or that Transport's AuthHandler (which, in server mode, is set up during rekeying [including initial kex].)
      • Changing this would be backwards incompatible (e.g. enforcing an actual __init__ on Server subclasses for the passing-in of a reference to one of the other objects, since right now Server doesn't even define one!) though I would like to examine it sometime. Now probably not the best time though.
    • Could still put specific, small-scoped changes in Transport though, such as around the check_channel_request calls here:

      paramiko/paramiko/transport.py

      Lines 2531 to 2537 in 27a8ed1

      reason = self.server_object.check_channel_direct_tcpip_request(
      my_chanid,
      (origin_addr, origin_port),
      (dest_addr, dest_port)
      )
      else:
      reason = self.server_object.check_channel_request(

My gut says to take a quick stab at the 1st approach but to fall back to the
2nd if the 1st cannot be done relatively painlessly.

Either way, re: the actual action to take seems poorly defined, but esp given OpenSSH simply spits out a bunch of question marks and not a "useful" error; the RFCs (4250, 4254) list only 4 default 'error' types, of which OPEN_FAILED_ADMINISTRATIVELY_PROHIBITED seems the closest fit. And indeed Paramiko uses it for eg bogus channel types, in some legacy tests.

@bitprophet
Copy link
Member Author

bitprophet commented Mar 13, 2018

A later note for testing: in addition to responding to MSG_CHANNEL_OPEN with MSG_CHANNEL_OPEN_FAILURE and the abovenoted error number, we also want to respond to other post-auth, pre-channel-id requests which mostly just includes MSG_GLOBAL_REQUEST (can be arbitrary, but is commonly used for port forwarding and other non-channel-specific requests.)

Those are just responded to with MSG_GLOBAL_REQUEST_FAILURE, which has no other fields for an error reason or anything.

@bitprophet
Copy link
Member Author

bitprophet commented Mar 13, 2018

Also also, I am finding that there are some Travis-level issues outstanding for most recent branches, orthogonal to this ticket (python 3.7-dev is exploding for no good reason) so this will not appear to go green there until I tackle that afterwards.

@bitprophet
Copy link
Member Author

More notes: in addition to writing the about-to-be-pushed tests, I also executed a standalone set of scripts provided by Matthijs, with the same net effect & result ("succeeds", as in the channel opens, without the fix; "fails", with the correct exception, with the fix.)

bitprophet added a commit that referenced this issue Mar 13, 2018
At least, insofar as the new tests pass...!
bitprophet added a commit that referenced this issue Mar 13, 2018
bitprophet added a commit that referenced this issue Mar 13, 2018
At least, insofar as the new tests pass...!
@bitprophet
Copy link
Member Author

Gotta figure out when I will stop backporting anything to 1.x, it is such a huge logistical hassle at this point despite overall code compatibility 😩

@bitprophet
Copy link
Member Author

bitprophet commented Mar 13, 2018

This is now on PyPI as Paramiko versions 1.17.6, 1.18.5, 2.0.8, 2.1.5, 2.2.3, 2.3.2, and 2.4.1.

Side note: I cannot bloody imagine doing this level of back and forth branch-wankery on Subversion. Very glad git has been my friend the last decade or so.

@stevebeattie
Copy link

Nice fix, thanks. Should the server side in _ensure_authed() or in run() be doing any logging (possibly at DEBUG level) if it sees these pre-auth attempts to open channels?

@stevebeattie
Copy link

Here's the fix backported to paramiko 1.10 that we will likely use in Ubuntu 14.04 LTS in case it's useful to anyone else.

dkhapun pushed a commit to cyberx-labs/paramiko that referenced this issue Jun 7, 2018
At least, insofar as the new tests pass...!
@six8
Copy link

six8 commented Aug 20, 2018

Where can I find Matthijs's client-test.py? I'd like to see if our specific implementation of Paramiko server is affected. We will upgrade to the latest version, but we have some old installations that will not be patched quickly.

@bitprophet
Copy link
Member Author

@six8 I probably didn't share it inline before due to sensitivity reasons but at this point with the patch out, I don't see why not. It's pretty innocuous either way:

#!/usr/bin/env python

import paramiko
import logging

hostname = "localhost"
port = 2200
skip_auth = True

try:
    logging.basicConfig(level=logging.DEBUG)

    client = paramiko.SSHClient()
    client.load_system_host_keys()
    client.set_missing_host_key_policy(paramiko.WarningPolicy())

    # Override the _auth method to let Paramiko skip the auth step
    if skip_auth:
        client._auth = lambda *args, **kwargs: None

    client.connect(hostname, port=port)
    client.exec_command('ls')

finally:
    client.close()

@six8
Copy link

six8 commented Aug 30, 2018

@bitprophet thanks. Easy enough.

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

No branches or pull requests

3 participants