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

[CVE-2018-1000805] Server-side auth vulnerability #1283

Closed
bitprophet opened this Issue Sep 6, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@bitprophet
Member

bitprophet commented Sep 6, 2018

Background

Security vulnerability on Paramiko's server side (NOT client side), as reported by Daniel Hoffman of usd AG.

I was emailed by the above on 2018.08.31 and (due to being on vacation at the time) was able to review the initial information on today's date (2018.09.06). Daniel submitted the following:

  • sample vanilla-enough server-side code loop (using standard/public API members with no additions besides logging and specification of expected auth mechanisms)
  • sample client-side code which exploits the vulnerability in the server (TK)
  • detailed explanation of why the exploit works, with references to parts of Paramiko's server-side code
  • recommended fix

At time of writing, I have done a cursory read of the code snippets & explanation, and confirmed that the sample code does appear to exhibit the described exploit.

Actual, post-fix description

The crux of it is as follows:

  • The exploit is that malicious clients can trick the Paramiko server into thinking the client is authenticated, without providing any authentication. Yup. That's bad. Real bad.
  • The flaw is in the Transport class, which is (regrettably IMHO; it's not my design) used unadulterated for both client and server use cases. No subclassing or anything; only a few very minor branch points.
  • It, and its symbiont class AuthHandler, both maintain a few "handler tables" for dispatching on message types within the main event loop; e.g. see MSG_USERAUTH_REQUEST, dispatch to an auth request handler method.
  • Smart users will already have put the previous facts together to figure out that...the exploit is the client sending MSG_USERAUTH_SUCCESS to the server. This is normally a server-to-client only message!
    • The Paramiko server then handles this message using a handler (within AuthHandler) intended for use by client use cases, because there is no differentiation in play.
    • And because the code is so strongly shared, this ends up flipping a flag saying "the other end is authenticated", which the server code continues to reference to determine if the client passed auth.
  • Thus, a malicious client can simply send that bogus message to set itself as authenticated for the rest of the session. Open, sesame.
  • The fix has been to separate the handler tables into specific client vs server components (given that, at least for the AuthHandler where this all takes place, there are no bidirectionally useful message types) and serve up only one or the other during dispatch, never both.
    • This was done in a backwards compatible manner, replacing static dicts with a dict-returning @property.
  • Due to expediency, I have not yet expanded this fix to the other couple of tables involved, but my guess is that the AuthHandler's table was the one that needed it most; past the auth step, any incorrect messages should just result in errors from the server and not exploitable server state.
  • The fix has been backported to Paramiko 2.0 and up.

TODO

  • take up Daniel on his offer to create a CVE; less work for me, but also, a security company has more experience with that process than I do, namely "any at all"
  • double check the details of the vulnerability so that I understand it more deeply
  • consider whether the suggested fix is valid (it has to do with Paramiko's reuse of code across client/server use cases)
  • confirm exploit & fix are both valid to 1.17.x/1.18.x (at present I am still considering 1.17.x to be pseudo-LTS and getting bugfixes, though this will soon cease) and up - nope, 2.0 and up, sorry folks. it's gotta happen sometime
  • test, patch, confirm
  • changelog entry
    • NOTE: may want to wait for CVE filing if not done yet, so at least this one commit has the CVE number in it? also consider a rebase to add it to the main patch commit/s
  • merge up to all branches
  • push & release at once so fix is live
  • update this ticket with my notes/real description
  • send a security notice out via Tidelift as well as Twitter
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 12, 2018

Member

The reporter initiated the CVE generation process, but it's been a few days with no movement on the part of the CVE creating authority & they've got a visible backlog as well. I'm probably going to release this patch, update this ticket with the details, and then just update the changelog & ticket once the CVE is actually assigned, for documentation reasons.

Doesn't seem worthwhile to delay patch release just so we can bake it in, if it's going to take this long. We'll see, maybe I'll get lucky and the time it takes me to do the rest of my release cycle will give them enough of a window 😝

Member

bitprophet commented Sep 12, 2018

The reporter initiated the CVE generation process, but it's been a few days with no movement on the part of the CVE creating authority & they've got a visible backlog as well. I'm probably going to release this patch, update this ticket with the details, and then just update the changelog & ticket once the CVE is actually assigned, for documentation reasons.

Doesn't seem worthwhile to delay patch release just so we can bake it in, if it's going to take this long. We'll see, maybe I'll get lucky and the time it takes me to do the rest of my release cycle will give them enough of a window 😝

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 18, 2018

Member

Kicking around the idea of using this as the place to say "1.x is dead". If not now, when? I just put in the work in #1291 to make it even easier to forward port 2.0.x up to master, but extending that to 1.17/1.18 is an even bigger kettle of fish, and the problems with 1.18 adding features not present in 2.0/2.1 (preventing a straight merge-up) will never go away.

A counterpoint would be "but...it's a CVE level security issue!". However, see above: what about the next one? The one after that?

It's clear that people are recently digging into the 15 year old, barely ever really worked on server functionality; there will be other important bugs for the, I want to say, 0.1% of the userbase that uses this in server mode. Do I backport all of them? Support windows are always a function of maintainer time/effort, and I'm very short on that these days. Paramiko 2.0 came out in April 2016 and did not change anything but an install requirement. Users still on 1.17.x really need to get moving :)

Member

bitprophet commented Sep 18, 2018

Kicking around the idea of using this as the place to say "1.x is dead". If not now, when? I just put in the work in #1291 to make it even easier to forward port 2.0.x up to master, but extending that to 1.17/1.18 is an even bigger kettle of fish, and the problems with 1.18 adding features not present in 2.0/2.1 (preventing a straight merge-up) will never go away.

A counterpoint would be "but...it's a CVE level security issue!". However, see above: what about the next one? The one after that?

It's clear that people are recently digging into the 15 year old, barely ever really worked on server functionality; there will be other important bugs for the, I want to say, 0.1% of the userbase that uses this in server mode. Do I backport all of them? Support windows are always a function of maintainer time/effort, and I'm very short on that these days. Paramiko 2.0 came out in April 2016 and did not change anything but an install requirement. Users still on 1.17.x really need to get moving :)

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 18, 2018

Member

Unrelated: still no movement on the CVE-tracking spreadsheet I was linked to. I didn't actually get all the way to merging this today (though I did make progress) so we'll see if it pops up before I finish/merge...

Member

bitprophet commented Sep 18, 2018

Unrelated: still no movement on the CVE-tracking spreadsheet I was linked to. I didn't actually get all the way to merging this today (though I did make progress) so we'll see if it pops up before I finish/merge...

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 19, 2018

Member

Took longer to identify a good way to really test this, than anticipated, thanks to the codebase being emphatically not designed with testing in mind 😭

On the plus side I identified a strongly related pseudo-bug which has been fixed (doubt it affects anyone who isn't running Paramiko on both ends of a conversation, but...). Two threads yelling "MSG_UNIMPLEMENTED!" at each other until one of them shuts down 😂

Member

bitprophet commented Sep 19, 2018

Took longer to identify a good way to really test this, than anticipated, thanks to the codebase being emphatically not designed with testing in mind 😭

On the plus side I identified a strongly related pseudo-bug which has been fixed (doubt it affects anyone who isn't running Paramiko on both ends of a conversation, but...). Two threads yelling "MSG_UNIMPLEMENTED!" at each other until one of them shuts down 😂

bitprophet added a commit that referenced this issue Sep 19, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 19, 2018

Member

Updated desc with details. waiting for Travis to be all green before I release...though they seem to be having a bad day. EDIT: Enough matrix cells are passing that I think I am just gonna release now, actually.

Member

bitprophet commented Sep 19, 2018

Updated desc with details. waiting for Travis to be all green before I release...though they seem to be having a bad day. EDIT: Enough matrix cells are passing that I think I am just gonna release now, actually.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Sep 19, 2018

Member

2.0.9 et al are on PyPI 🎉

Member

bitprophet commented Sep 19, 2018

2.0.9 et al are on PyPI 🎉

@bitprophet bitprophet closed this Sep 19, 2018

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Sep 19, 2018

Considering that it's the second important security problem in a short time frame and that the exploit seems relatively easy to find, can't we anticiptate many other problems of this kind to pop up?

Considering that the server feature of paramiko is probably seldom used, couldn't we entertain the idea of stripping paramiko of its server functionality to avoid all this pain?

hsoft commented Sep 19, 2018

Considering that it's the second important security problem in a short time frame and that the exploit seems relatively easy to find, can't we anticiptate many other problems of this kind to pop up?

Considering that the server feature of paramiko is probably seldom used, couldn't we entertain the idea of stripping paramiko of its server functionality to avoid all this pain?

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Sep 19, 2018

Or, maybe, disable it unless an environment variable of the I_KNOW_WHAT_IM_DOING kind is set, and backport the de-activation in all previous branches (including 1.x) so you can finally close the books on those branches without feeling the guilt of leaving potential users at risk.

hsoft commented Sep 19, 2018

Or, maybe, disable it unless an environment variable of the I_KNOW_WHAT_IM_DOING kind is set, and backport the de-activation in all previous branches (including 1.x) so you can finally close the books on those branches without feeling the guilt of leaving potential users at risk.

@attritionorg

This comment has been minimized.

Show comment
Hide comment
@attritionorg

attritionorg Oct 8, 2018

CVE-2018-1000805

attritionorg commented Oct 8, 2018

CVE-2018-1000805

@bitprophet bitprophet changed the title from [CVE PENDING] Server-side auth vulnerability [DESC TK] to [CVE-2018-1000805] Server-side auth vulnerability Oct 8, 2018

bitprophet added a commit that referenced this issue Oct 8, 2018

mi-1-0-0 added a commit to mi-1-0-0/openstack-neo4j-service that referenced this issue Oct 10, 2018

fghaas added a commit to fghaas/hastexo-xblock that referenced this issue Oct 12, 2018

Bump paramiko version
The server-side vulnerability in Paramiko 2.1.5 does not affect us
(we're only using Paramiko in client mode), but it doesn't hurt
to require a version where the vulnerability is fixed.

References:
https://nvd.nist.gov/vuln/detail/CVE-2018-1000805
paramiko/paramiko#1283
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment