dyne / tomb

the Crypto Undertaker
https://dyne.org/software/tomb
GNU General Public License v3.0
1.32k stars 150 forks source link

Wrong last user in tomb list #198

Closed arthaud closed 9 years ago

arthaud commented 9 years ago

Hi,

When I'm using tomb list, the last visited user is wrong.

It's because you are looking for :$uid: in /etc/passwd. Actually, my /etc/passwd looks like:

maxima:x:1000:100:maxima:/home/maxima:/bin/zsh
skype:x:1001:1000::/home/skype:/bin/false

and therefore, looking for :1000:, user skype is found instead of me.

This happens here: https://github.com/dyne/Tomb/blob/master/tomb#L1821-L1824 and there: https://github.com/dyne/Tomb/blob/master/tomb#L1986-L1989

You could probably use getent passwd $uid

hellekin commented 9 years ago

Right, is getent available for *BSD?

Otherwise using awk could work as well:

tombuser=$(awk -F: '/^[^:]+:x:'$tombuid':/ {print $1}' /etc/passwd)

Using getent:

tombuser=$(getent passwd $tombuid | awk -F: '{print $1}')
arthaud commented 9 years ago

I don't know. Your example using awk seems good.

hellekin commented 9 years ago

Minimal patch, replace:

[[ "$ee" =~ ":${tombuid}:" ]] && {

with

[[ "$ee" =~ ":x:${tombuid}:" ]] && {

(Both my proposals assume use of shadow passwords.)

boyska commented 9 years ago

On Tue, Apr 14, 2015 at 01:40:28PM -0700, hellekin wrote:

Otherwise using awk could work as well:

tombuser=$(awk -F: '/^[^:]+:x:'$tombuid':/ {print $1}' /etc/passwd)

I'd prefer

awk -F: '$4 == '$tombuid' { print $1 }'
tombuser=$(getent passwd $tombuid | awk -F: '{print $1}')

this is maybe clearer with cut:

getent passwd $tombuid | cut -d: -f1
hellekin commented 9 years ago

Fair enough.

hellekin commented 9 years ago

@jaromil I put the fix into a branch. Please ack and close the issue.

jaromil commented 9 years ago

ACK. Thanks @arthaud, well spotted. Three observations:

1) Tomb is not compabible with BSD. If we aim for that, we need some BSD expert to join our team first, then we can target use of tcplay instead of cryptsetup (hence modularize that part of the code) and then finalize. Until we take this route the only concern against using getent is about avoiding the spawn of a new binary (hence depending from its integrity, yet this isn't a critical role for it in the script and its not suid when run).

2) ZSh has native support for simple string-token operations, which is preferable than spawning extra processes. In this case an half native formula would be:

getent passwd $tombuid | sysread tombuser
tombuser=${tombuser[(ws@:@)1]}

or without the zmodule zsh/system (which I think we aren't using in Tomb, perhaps we should)

tombuser=$(getent passwd $tombuid)
tombuser=${tombuser[(ws@:@)1]}

@hellekin if you like to update your pull request with the latter, I agree to merge it.

3) We may want to avoid using getent and have a fully native ZSh approach to parsing passwd

boyska commented 9 years ago

point 3 is conceptually wrong. getent does not just read /etc/passwd, it

get entries from Name Service Switch libraries

so if you want to reimplement it (but why?) you should actually reimplement a parser for /etc/nsswitch.conf; good luck with that.

jaromil commented 9 years ago

Eventually, in 3) I mention we may parse /etc/passwd natively, not cover all getent functionalities.

boyska commented 9 years ago

So you might not found the user. Not every user is in /etc/passwd.

jaromil commented 9 years ago

You are right. Lets resort to solution 2). Wait for hellekin to respond so we don't waste his branch, he is on another timezone.