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

su: race while checking for incorrect password #363

Closed
dw opened this issue Sep 11, 2018 · 1 comment

Comments

@dw
Copy link
Owner

commented Sep 11, 2018

When running with a heavily loaded box:

#!/bin/bash -ex

PROCS=$(grep -c ^processor /proc/cpuinfo)
NPROCS=$(( $PROCS * 6 ))

for i in `seq 1 $NPROCS` ; do
    python -c 'while 1: pass' &
    pids="$pids $!"
done

echo $pids
trap "kill $pids" EXIT
sleep 86400

mitogen.su can fail to spot an incorrect password prompt, because su calls write() twice, once to write "su: ", and again to write "incorrect password". It is possible for su to be preempted between writes and for su.py to observe the partial buffer.

^[[0;34m[pid 29881] 19:47:30.344160 D mitogen.ctx.ssh.localhost:2203: mitogen.su: mitogen.su.Stream(u'local.474'): received '\n'^[[0m
^[[0;34m[pid 29881] 19:47:35.858508 D mitogen.ctx.ssh.localhost:2202: mitogen.su: mitogen.su.Stream(u'local.2517'): received 'su: '^[[0m
^[[0;34m[pid 29881] 19:47:35.860656 D mitogen.ctx.ssh.localhost:2201: mitogen.su: mitogen.su.Stream(u'local.496'): received 'su: Authentication failure\n'^[[0m
^[[0;34m[pid 29881] 19:47:35.860912 D mitogen.ctx.ssh.localhost:2201: mitogen: mitogen.su.Stream(u'local.496'): child process still alive, sending SIGTERM^[[0m
^[[0;34m[pid 29881] 19:47:35.861723 D mitogen.ctx.ssh.localhost:2202: mitogen.su: mitogen.su.Stream(u'local.2517'): received 'incorrect password\n'^[[0m
^[[0;34m[pid 29881] 19:47:35.861997 D mitogen.ctx.ssh.localhost:2202: mitogen: mitogen.su.Stream(u'local.2517'): child process exit status was 32000^[[0m
^[[0;34m[pid 2770] 19:47:35.863444 D mitogen: mitogen.core.Stream(u'unix_listener.29881').on_disconnect()^[[0m
^[[0;34m[pid 2774] 19:47:35.863463 D mitogen: mitogen.core.Stream(u'unix_listener.29881').on_disconnect()^[[0m
^[[0;34m[pid 2774] 19:47:35.863985 D mitogen: Waker(Broker(0x7f2f0b32d190) rfd=19, wfd=20).on_disconnect()^[[0m
^[[0;34m[pid 29881] 19:47:35.864297 D mitogen: mitogen.core.Stream(u'unix_client.2774').on_disconnect()^[[0m
^[[0;34m[pid 29881] 19:47:35.864974 D mitogen: mitogen.core.Stream(u'unix_client.2770').on_disconnect()^[[0m
^[[0;34m[pid 2770] 19:47:35.864044 D mitogen: Waker(Broker(0x7f2f0ae854d0) rfd=19, wfd=20).on_disconnect()^[[0m
^[[0;31mfatal: /home/dmw/src/mitogen/tests/ansible/integration/become/su_password.yml:28: [target-centos6]: FAILED! => {^[[0m
^[[0;31m  msg: error occurred on host target-centos6: EOF on stream; last 300 bytes received: u'Password: \nsu: incorrect password\n'^[[0m
@dw

This comment has been minimized.

Copy link
Owner Author

commented Mar 9, 2019

Fixed as part of #419

@dw dw closed this Mar 9, 2019

dw added a commit that referenced this issue Mar 12, 2019

Refactor Stream, introduce quasi-asynchronous connect, much more
Split Stream into many, many classes

  * mitogen.parent.Connection: Handles connection setup logic only.
    * Maintain references to stdout and stderr streams.
    * Manages TimerList timer to cancel connection attempt after
      deadline
    * Blocking setup code replaced by async equivalents running on the
      broker

  * mitogen.parent.Options: Tracks connection-specific options. This
    keeps the connection class small, but more importantly, it is
    generic to the future desire to build and execute command lines
    without starting a full connection.

  * mitogen.core.Protocol: Handles program behaviour relating to events
    on a stream. Protocol performs no IO of its own, instead deferring
    it to Stream and Side. This makes testing much easier, and means
    libssh can reimplement Stream and Side to reuse MitogenProtocol

  * mitogen.core.MitogenProtocol: Guts of the old Mitogen stream
    implementtion

  * mitogen.core.BufferedWriter: Guts of the old Mitogen buffered
    transmit implementation, made generic

  * mitogen.core.DelineatedProtocol: Guts of the old IoLogger, knows how
    to split up input and pass it on to a
    on_line_received()/on_partial_line_received() callback.

  * mitogen.parent.BootstrapProtocol: Asynchronous equivalent of the old
    blocking connect code. Waits for various prompts (MITO001 etc) and
    writes the bootstrap using a BufferedWriter. On success, switches
    the stream to MitogenProtocol.

  * mitogen.core.Message: move encoding parts of MitogenProtocol out to
    Message (where it belongs) and write a bunch of new tests for
    pickling.

  * The bizarre Stream.construct() is gone now, Option.__init__ is its
    own constructor. Should fix many LGTM errors.

* Update all connection methods:  Every connection method is updated to
  use async logic, defining protocols as required to handle interactive
  prompts like in SSH or su. Add new real integration tests for at least
  doas and su.

* Eliminate manual fd management: File descriptors are trapped in file
  objects at their point of origin, and Side is updated to use file
  objects rather than raw descriptors. This eliminates a whole class of
  bugs where unrelated FDs could be closed by the wrong component. Now
  an FD's open/closed status is fused to it everywhere in the library.

* Halve file descriptor usage: now FD open/close state is tracked by
  its file object, we don't need to duplicate FDs everywhere so that
  receive/transmit side can be closed independently. Instead both sides
  back on to the same file object. Closes #26, Closes #470.

* Remove most uses of dup/dup2: Closes #256. File descriptors are
  trapped in a common file object and shared among classes. The
  remaining few uses for dup/dup2 are as close to minimal as possible.

* Introduce mitogen.parent.Process: uniform interface for subprocesses
  created either via mitogen.fork or the subprocess module. Remove all
  the crap where we steal a pid from subprocess guts. Now we use
  subprocess to manage its processes as it should be. Closes #169 by
  using the new Timers facility to poll for a slow-to-exit subprocess.

* Fix su password race: Closes #363. DelineatedProtocol naturally
  retries partially received lines, preventing the cause of the original
  race.

* Delete old blocking IO utility functions
  iter_read()/write_all()/discard_until().

Closes #26
Closes #147
Closes #169
Closes #256
Closes #363
Closes #419
Closes #470

dw added a commit that referenced this issue Jul 15, 2019

dw added a commit that referenced this issue Jul 22, 2019

Refactor Stream, introduce quasi-asynchronous connect, much more
Split Stream into many, many classes

  * mitogen.parent.Connection: Handles connection setup logic only.
    * Maintain references to stdout and stderr streams.
    * Manages TimerList timer to cancel connection attempt after
      deadline
    * Blocking setup code replaced by async equivalents running on the
      broker

  * mitogen.parent.Options: Tracks connection-specific options. This
    keeps the connection class small, but more importantly, it is
    generic to the future desire to build and execute command lines
    without starting a full connection.

  * mitogen.core.Protocol: Handles program behaviour relating to events
    on a stream. Protocol performs no IO of its own, instead deferring
    it to Stream and Side. This makes testing much easier, and means
    libssh can reimplement Stream and Side to reuse MitogenProtocol

  * mitogen.core.MitogenProtocol: Guts of the old Mitogen stream
    implementtion

  * mitogen.core.BufferedWriter: Guts of the old Mitogen buffered
    transmit implementation, made generic

  * mitogen.core.DelineatedProtocol: Guts of the old IoLogger, knows how
    to split up input and pass it on to a
    on_line_received()/on_partial_line_received() callback.

  * mitogen.parent.BootstrapProtocol: Asynchronous equivalent of the old
    blocking connect code. Waits for various prompts (MITO001 etc) and
    writes the bootstrap using a BufferedWriter. On success, switches
    the stream to MitogenProtocol.

  * mitogen.core.Message: move encoding parts of MitogenProtocol out to
    Message (where it belongs) and write a bunch of new tests for
    pickling.

  * The bizarre Stream.construct() is gone now, Option.__init__ is its
    own constructor. Should fix many LGTM errors.

* Update all connection methods:  Every connection method is updated to
  use async logic, defining protocols as required to handle interactive
  prompts like in SSH or su. Add new real integration tests for at least
  doas and su.

* Eliminate manual fd management: File descriptors are trapped in file
  objects at their point of origin, and Side is updated to use file
  objects rather than raw descriptors. This eliminates a whole class of
  bugs where unrelated FDs could be closed by the wrong component. Now
  an FD's open/closed status is fused to it everywhere in the library.

* Halve file descriptor usage: now FD open/close state is tracked by
  its file object, we don't need to duplicate FDs everywhere so that
  receive/transmit side can be closed independently. Instead both sides
  back on to the same file object. Closes #26, Closes #470.

* Remove most uses of dup/dup2: Closes #256. File descriptors are
  trapped in a common file object and shared among classes. The
  remaining few uses for dup/dup2 are as close to minimal as possible.

* Introduce mitogen.parent.Process: uniform interface for subprocesses
  created either via mitogen.fork or the subprocess module. Remove all
  the crap where we steal a pid from subprocess guts. Now we use
  subprocess to manage its processes as it should be. Closes #169 by
  using the new Timers facility to poll for a slow-to-exit subprocess.

* Fix su password race: Closes #363. DelineatedProtocol naturally
  retries partially received lines, preventing the cause of the original
  race.

* Delete old blocking IO utility functions
  iter_read()/write_all()/discard_until().

Closes #26
Closes #147
Closes #169
Closes #256
Closes #363
Closes #419
Closes #470

dw added a commit that referenced this issue Jul 22, 2019

dw added a commit that referenced this issue Jul 22, 2019

Refactor Stream, introduce quasi-asynchronous connect, much more
Split Stream into many, many classes

  * mitogen.parent.Connection: Handles connection setup logic only.
    * Maintain references to stdout and stderr streams.
    * Manages TimerList timer to cancel connection attempt after
      deadline
    * Blocking setup code replaced by async equivalents running on the
      broker

  * mitogen.parent.Options: Tracks connection-specific options. This
    keeps the connection class small, but more importantly, it is
    generic to the future desire to build and execute command lines
    without starting a full connection.

  * mitogen.core.Protocol: Handles program behaviour relating to events
    on a stream. Protocol performs no IO of its own, instead deferring
    it to Stream and Side. This makes testing much easier, and means
    libssh can reimplement Stream and Side to reuse MitogenProtocol

  * mitogen.core.MitogenProtocol: Guts of the old Mitogen stream
    implementtion

  * mitogen.core.BufferedWriter: Guts of the old Mitogen buffered
    transmit implementation, made generic

  * mitogen.core.DelineatedProtocol: Guts of the old IoLogger, knows how
    to split up input and pass it on to a
    on_line_received()/on_partial_line_received() callback.

  * mitogen.parent.BootstrapProtocol: Asynchronous equivalent of the old
    blocking connect code. Waits for various prompts (MITO001 etc) and
    writes the bootstrap using a BufferedWriter. On success, switches
    the stream to MitogenProtocol.

  * mitogen.core.Message: move encoding parts of MitogenProtocol out to
    Message (where it belongs) and write a bunch of new tests for
    pickling.

  * The bizarre Stream.construct() is gone now, Option.__init__ is its
    own constructor. Should fix many LGTM errors.

* Update all connection methods:  Every connection method is updated to
  use async logic, defining protocols as required to handle interactive
  prompts like in SSH or su. Add new real integration tests for at least
  doas and su.

* Eliminate manual fd management: File descriptors are trapped in file
  objects at their point of origin, and Side is updated to use file
  objects rather than raw descriptors. This eliminates a whole class of
  bugs where unrelated FDs could be closed by the wrong component. Now
  an FD's open/closed status is fused to it everywhere in the library.

* Halve file descriptor usage: now FD open/close state is tracked by
  its file object, we don't need to duplicate FDs everywhere so that
  receive/transmit side can be closed independently. Instead both sides
  back on to the same file object. Closes #26, Closes #470.

* Remove most uses of dup/dup2: Closes #256. File descriptors are
  trapped in a common file object and shared among classes. The
  remaining few uses for dup/dup2 are as close to minimal as possible.

* Introduce mitogen.parent.Process: uniform interface for subprocesses
  created either via mitogen.fork or the subprocess module. Remove all
  the crap where we steal a pid from subprocess guts. Now we use
  subprocess to manage its processes as it should be. Closes #169 by
  using the new Timers facility to poll for a slow-to-exit subprocess.

* Fix su password race: Closes #363. DelineatedProtocol naturally
  retries partially received lines, preventing the cause of the original
  race.

* Delete old blocking IO utility functions
  iter_read()/write_all()/discard_until().

Closes #26
Closes #147
Closes #169
Closes #256
Closes #363
Closes #419
Closes #470

dw added a commit that referenced this issue Jul 22, 2019

dw added a commit that referenced this issue Jul 22, 2019

dw added a commit that referenced this issue Jul 28, 2019

Merge remote-tracking branch 'origin/stream-refactor'
* origin/stream-refactor:
  [stream-refactor] Py3.x test fixes
  [stream-refactor] mark py24 as allow-fail
  [stream-refactor] Debian Docker container image initctl
  [stream-refactor] replace cutpaste with Stream.accept() in mitogen.unix
  [stream-refactor] fix flake8 errors
  [stream-refactor] fix testlib assertion format string
  [stream-refactor] make mitogen-fuse work on Linux
  [stream-refactor] repair preamble_size.py again
  [stream-refactor] don't abort Connection until all buffers are empty
  Normalize docstring formatting
  [stream-refactor] fix LogHandler.uncork() race
  [stream-refactor] BufferedWriter must disconenct Stream, not Protocol
  [stream-refactor] statically link doas binary using musl
  [stream-refactor] stop writing to /tmp/foo in fd_check.py.
  [stream-refactor] yet another 2.4 issue in create_child_test
  [stream-refactor] fix Py2.4 failure by implementing missing Timer method
  [stream-refactor] allow up to 30 seconds to connect in unix_test
  [stream-refactor] mark setns module as requiring Python >2.4
  [stream-refactor] another 2.4 fix for create_child_test
  .travis.yml: Add reverse shell spawn for Travis too
  core: better Side attribute docstrings
  [stream-refactor] remove one more getuser() usage
  [stream-refactor] allow doas_test to succeed on CentOS
  Pin idna==2.7 when running on Python<2.7.
  [stream-refactor] Py2.4 compat fix for iter_split_test.
  [stream-refactor] add descriptive task names to _container_prep
  [stream-refactor] 3.x socket.send() requires bytes
  [stream-refactor] fix 2.4 syntax error.
  [stream-refactor] avoid os.wait3() for Py2.4.
  Allow specifying -vvv to debops_tests.
  [stream-refactor] send MITO002 earlier
  module_finder: pass raw file to compile()
  [stream-refactor] merge stdout+stderr when reporting EofError
  [stream-refactor] fix crash in detach() / during async/multiple_items_loop.yml
  [stream-refactor] fix crash in runner/forking_active.yml
  [stream-refactor] replace old detach_popen() reference
  ansible: fixturize creation of MuxProcess
  unix: ensure mitogen.context_id is reset when client disconnects
  [stream-refactor] make syntax 2.4 compatible
  [stream-refactor] make trusty our Travis dist.
  [stream-refactor] fix su_test failure (issue #363)
  [stream-refactor] more readable log string format
  [stream-refactor] dont doubly log last partial line
  [stream-refactor] import fd_check.py used by create_child_test
  [stream-refactor] port mitogen.buildah, added to master since work began
  [stream-refactor] fix unix.Listener construction
  [stream-refactor] fix crash when no stderr present.
  [stream-refactor] fix Process constructor invocation
  Add tests/ansible/.*.pid to gitignore (for ansible_mitogen/process.py)
  Add extra/ to gitignore
  import release-notes script.
  [stream-refactor] repaired rest of create_child_test.
  [stream-refactor] rename Process attrs, fix up more create_child_test
  [stream-refactor] import incomplete create_child_test
  issue #482: tests: check for zombie process after test.
  issue #363: add test.
  tests: clean up old-style SSH exception catch
  issue #271: add mitogen__permdenied user to Docker image.
  ssh: fix issue #271 regression due to refactor, add test.
  Refactor Stream, introduce quasi-asynchronous connect, much more
  core: teach iter_split() to break on callback returning False.
  issue #507: log fatal errors to syslog.
  testlib: have LogCapturer.raw() return unicode on 2.x.
  core/master: docstring, repr, and debug log message cleanups
  parent: remove unused Timer parameter.
  tests: jail_test fixes.
  parent: docstring improvements, cfmakeraw() regression.
  core: introduce Protocol, DelimitedProtocol and BufferedWriter.
  core: introduce mitogen.core.pipe()
  tests/bench: import ssh-roundtrip.py.
  tests: note location of related tests.
  tests: add real test for doas.
  tests: install OpenBSD doas port in Debian image.
  tests: add setns_test that works if password localhost sudo works.
  Import minimal jail_test.
  core: move message encoding to Message.pack(), add+refactor tests.
  master: expect forwarded logs to be in UTF-8.
  tests: add some UTF-8 to ssh_login_banner to encourage breakage.
  core: bootstrap FD management improvements
  core: pending timers should keep broker alive.
  core: more succinct iter_split().
  core: replace UTF8_CODEC with encodings.utf_8.encode() function.
  docs: remove bytearray from supported types list.
  core: docstring style cleanups, dead code.
  testlib: disable lsof warnings due to Docker crap
  parent: discard cancelled events in TimerList.get_timeout().
  core: split out iter_split() for use in parent.py.
  parent: various style cleanups, remove unused function.
  issue #170: add TimerList docstrings.
  core: eliminate some quadratric behaviour from IoLogger
  issue #170: update Changelog; closes #170.
  issue #170: add timers to internals.rst.
  issue #170: implement timers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.