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.

Sign up for GitHub

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

Jump to bottom

remote sudo password bypass via vim #39

Closed
Fuseteam opened this issue Feb 10, 2021 · 8 comments · Fixed by #43 or #45
Closed

remote sudo password bypass via vim #39

Fuseteam opened this issue Feb 10, 2021 · 8 comments · Fixed by #43 or #45
Labels
bug Something isn't working security

Comments

@Fuseteam
Copy link

on a remote machine I get prompted for a password when I run a sudo commandline from the cli
however I don't get prompted for my password if I

  • open a file with vim
  • save with :w !sudo tee %
@mcdope
Copy link
Owner

mcdope commented Feb 10, 2021

More details please. What's your config? What exactly means "remote" (I assume SSH)?

@Fuseteam
Copy link
Author

Fuseteam commented Feb 10, 2021 •
edited

yes remote is ssh, in my config "deny_remote" is not set so I assume it is "true"
i can confirm it is indeed true as when I run say sudo su i do get denied access and I do need to input my password
it is when i run say vim /etc/security/pam_usb.conf, which opens the file in readonly mode, and save the file with :w !sudo tee % that it doesn't prompt for my password as usual

@mcdope
Copy link
Owner

mcdope commented Feb 10, 2021 •
edited

I guess that's an edgecase we can't solve. Issue most likely will be: as soon as a process is run, be it vim or mc or doom or whatever, and it issues a su(do) request it is a local request.

I will take a look and see if we can fix this, but I have a feeling that this isn't fixable.

@mcdope mcdope added bug Something isn't working security labels Feb 10, 2021
@Fuseteam
Copy link
Author

I guess that's an edgecase we can't solve. Issue most likely will be: as soon as a process is run, be it vim or mc or doom or whatever, and it issues a su(do) request it is a local request.

I will take a look and see if we can fix this, but I have a feeling that this isn't fixable.

ah that would explain it yes, hmm

@mcdope
Copy link
Owner

mcdope commented Feb 12, 2021

Investigated it, this was introduced with the fix for #8

Funny thing is that I even left a comment that this could be an issue :D #8 (comment)

I will change the option to default to false. Until the new release is ready you can fix this by setting unknown_pts_as_local to false. However, then some truly local sudo requests will fail too.

mcdope added a commit that referenced this issue Feb 12, 2021
@mcdope
#39: Set to default false
d86a9f0
@mcdope
Copy link
Owner

mcdope commented Feb 12, 2021

Wiki updated

https://github.com/mcdope/pam_usb/wiki/Configuration#note-about-unknown_pts_as_local

@mcdope mcdope mentioned this issue Feb 12, 2021
Merged
mcdope added a commit that referenced this issue Feb 12, 2021
@mcdope
Merge pull request #43 from mcdope/issue-39-remote-sudo-password-bypa…
c6fde1b 
…ss-via-vim

#39: remote sudo password bypass via vim

Back when we fixed #8 we introduced a sideeffect, allowing sudo bypass if connected to remote host as userX when userX has his already configured media connected.

This changes the default value for the then introduced option unknown_pts_as_local to false to fix this.

Closes #39
@mcdope
Copy link
Owner

mcdope commented Feb 12, 2021

Fix released as 0.7.2, repo is updated.

https://apt.mcdope.org/libpam-usb_0.7.2_amd64.deb

@mcdope mcdope mentioned this issue Feb 13, 2021
Closed
mcdope added a commit that referenced this issue Feb 13, 2021
@mcdope
#8/#39: Rework to use process based checking, renders useless - to be…
20bef8e 
… removed
This was referenced Feb 13, 2021
Merged
Open
mcdope added a commit that referenced this issue Feb 13, 2021
@mcdope
Remove unknown_pts_as_local option (#8/#39)
f9e90cc
@Fuseteam
Copy link
Author

Investigated it, this was introduced with the fix for #8

Funny thing is that I even left a comment that this could be an issue :D #8 (comment)

I will change the option to default to false. Until the new release is ready you can fix this by setting unknown_pts_as_local to false. However, then some truly local sudo requests will fail too.

cool :D thanks!

mcdope added a commit that referenced this issue Feb 14, 2021
@mcdope
Merge pull request #45 from mcdope/issue-8-rework-deny_remote-option
ac725b6 
#8/#39: Rework deny_remote / remove unknown_pts_as_local

This reworks deny_remote handling to use process based checking.

Instead of checking utmp and hoping there is an entry for the current session, which there often isn't for virtual terminals, we now check the chain of parent processes. If any parent process is sshd or telnetd we deny authentication.

This renders unknown_pts_as_local obsolete - removed.

Closes #8 (again)
See #39
@mcdope mcdope linked a pull request Feb 14, 2021 that will close this issue
#8/#39: Rework deny_remote / remove unknown_pts_as_local #45
Merged
This was referenced Mar 1, 2021
Merged
Open
mcdope added a commit that referenced this issue Aug 31, 2021
@mcdope
#51: fix terminal multiplexers bypass remote sudo password (#54)
b5ec9eb 
The tty only approach had downsides (see #8), its fix had downsides too (see #39), and the name approach had downsides (see #51) too, let's try all together plus some additional magic.

This modifies local check to:
* check for sshd/telnetd in process chain - for the obvious cases
* check for tmux in process chain, if found parse its environment to determine tmux client id to determine session tty for utmp check
* if tmux found, but session tty not, check for remote clients attached to tmux
* check for DISPLAY, if found use that for utmp check
* in case no remote daemon was found, tmux wasn't detected, and DISPLAY is not set - fall back to good ol' ttyname() which should now be safe since we handled all edge cases before

Closes #51 


* #51: process:c add get_process_tty()

* #51: local.c: Re-add utmp code, to be used by parent pid using process.c [WIP]

* #51: Add @todo

* #51: [WIP] Rework get_process_tty(), check for X session, add more debug logging

* #51: [WIP] If tmux detected use it to detect the login tty

* #51: local.c use new tmux/display/tty approach in all cases

* #51: process.c: remove get_process_tty(), local.c: rename from to session_tty

* #51: [Debian] [Packaging] Re-add 'Standards-Version', got lost somehow

* #51: local.c: replace 4-spaces with tabs to keep uniform formatting

* #51: remove libprocps depency again

* #51: local.c: remove current_tty - used only for logging / making the code order nicer

* local.c: spaces...

* local.c: fix alt-tab-typo and some formatting

* #51: Test for open udp port 177 (XDMCP negotiation), if not found allow (when display manager is found)

* #51: Remove port check stuff again, XDMCP is a pain to setup for testing and is insecure anyway

* #51: local.c: whitelist graphical logins by service tag, remove xdmcp leftovers

* #51: Make ttyname() approach default fallback for all cases

* #51: local.c: extract tmux magic to tmux.c

* #51: local.c/Makefile: make use of tmux.c, adjust to new chain

* #51: Iterate all tty methods, add 'tmux var from parent proc', cleanup formatting

* #51: local.c: add pusb_get_tty_by_xorg_display(), used to get tty by DISPLAY var (for SDDM sessions)

* #51: Fix DISPLAY fallback, add more debug, expect console and pts

* #51: Add pusb_ prefix to new functions

* #51: Add pusb_ prefix to new functions 2nd edition

* #51: Whitelist sddm too

* #51: [WIP] [deb} Update news and changelog

* #51: Fix incorrect return handling reported in #51 (comment)

* #51: Fix derp

* actions: make sure no previously build debs are installed again...

* #51: Check for remotely connected clients to local tmux sessions

* #51: This and that

* #51: Fix v6 detection of connected tmux sessions

* #51: Cleanup

* #51: 'Fix' debug output

* #51: Fix warning unitialized for tmux_pid

* #51/#64: Replace utmp with utmpx stuff / posix compliance

* #51: tmux.c: extend regex to capture full 'attach' argument too

* #51: Fix last case of byobu/tmux

* #51: Remove version update, will be done in seperate PR

* #51: Cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working security
Projects
None yet
Milestone
No milestone
2 participants
@mcdope @Fuseteam