Review Request 129526: RFE: kwallet-pam should use XDG_RUNTIME_DIR instead of /tmp for the socketPath

[David Faure]

> On Jan. 8, 2017, 4:09 p.m., David Faure wrote:
> > pam_kwallet.c, line 422
> > <https://git.reviewboard.kde.org/r/129526/diff/1/?file=486385#file486385line422>
> >
> >     trailing spaces
> 
> Damjan Georgievski wrote:
>     > according to http://standards.freedesktop.org/basedir-spec/latest/, one is supposed to check permissions
>     
>     I don't see it in the specs, and it says: „The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700.“ - but it might be a sensible thing to check (although there are race conditions in checking and only trying to use it later).
>     
>     > trailing spaces
>     
>     ughh, what do I do now, "Update diff"?
> 
> David Faure wrote:
>     Yes, these "MUST" are exactly what I meant the code is supposed to check before using XDG_RUNTIME_DIR :)
>     
>     I'm confused by your reply, you say "I don't see it in the spec" and then you quote exactly what I am referring to.
>     
>     There is no race condition in checking for "I own it and it's 0700" before using, because this can only change if root intervenes, another user cannot do anything about a dir that he doesn't own and that is 0700. And if root is compromised, all is lost anyway ;)
> 
> Damjan Georgievski wrote:
>     > I'm confused by your reply, you say "I don't see it in the spec" and then you quote exactly what I am referring to.
>     
>     huh, I might be mistaken, but the way I read it, the creator of $XDG_RUNTIME_DIR MUST do those things, otherwise it shouldn't set the environment variable.
> 
> David Faure wrote:
>     I see. It's indeed unclear in the spec. I guess it was clearer in the initial discussion on the ML...
>     
>     Every one else checks though ;)
>     https://git.enlightenment.org/core/enlightenment.git/commit/?id=2520c73d04049f1c84c67cc5e3e55c10d2078025
>     https://github.com/whitequark/rust-xdg/pull/10/files
>     + what I did in QStandardPaths.
>     
>     Anyway, compared to /tmp I guess a world-writable socket dir isn't worse, I'm not going to veto it since the spec is unclear, I'll let someone else approve this though. Meanwhile I'll ask for clarification on the xdg list ;)

Lennart clarified this on the xdg list:


"When I wrote this I always had in mind that the component setting
XDG_RUNTIME_DIR is responsible for preparating the dir the right way,
and that apps may simply trust that the dir is properly set up when
they see the environment variable set.

That said, people do weird stuff with su/sudo. It might or might not
make sense for apps to superficially check ownership of the dir before
using it. However I am very sure apps should never try to "fix" it it
doesn't match their expectations, as that most likely would make
things worse, not better in such su/sudo setups."


So I'm OK with no checks.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129526/#review101873
-----------------------------------------------------------


On Jan. 8, 2017, 4:59 p.m., Damjan Georgievski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129526/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2017, 4:59 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 365722
>     https://bugs.kde.org/show_bug.cgi?id=365722
> 
> 
> Repository: kwallet-pam
> 
> 
> Description
> -------
> 
> Most recent Linux distributions setup a per-user XDG_RUNTIME_DIR as a tmpfs, which is also tied to their session lifecycle. Typically this is in /run/user/1000/.
> 
> My suggestion is to use $XDG_RUNTIME_DIR/kwallet5.socket if XDG_RUNTIME_DIR exists, or fallback to /tmp/kwallet5_${username}.socket if it doesn't.
> 
> Reproducible: Always
> 
> 
> Diffs
> -----
> 
>   pam_kwallet.c 809ab9a 
> 
> Diff: https://git.reviewboard.kde.org/r/129526/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Damjan Georgievski
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170114/48cd8b06/attachment-0001.html>