[PATCH] The check of !S_ISDIR(sb.st_mode) in second time is wrong since it's already known to be false at that point.

[Dmitri Paduchikh]

---
tmux.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tmux.c b/tmux.c
index 5a1988e..832be8a 100644
--- a/tmux.c
+++ b/tmux.c
@@ -149,8 +149,7 @@ makesocketpath(const char *label)
errno = ENOTDIR;
return (NULL);
}
- if (sb.st_uid != uid || (!S_ISDIR(sb.st_mode) &&
- sb.st_mode & (S_IRWXG|S_IRWXO)) != 0) {
+ if (sb.st_uid != uid || (sb.st_mode & (S_IRWXG|S_IRWXO)) != 0) {
errno = EACCES;
return (NULL);
}

[Dmitri Paduchikh]

Hello,

I am afraid that my first message wasn't clear. Not from first look at
least. Sorry about that. Let me elaborate, I think it is probably a
security bug. makesocketpath() checks !S_ISDIR(sb.st_mode); when true it
returns with error. But then it checks !S_ISDIR(sb.st_mode) in second
time and it is definitely false at this point:

if (sb.st_uid != uid || (!S_ISDIR(sb.st_mode) &&
sb.st_mode & (S_IRWXG|S_IRWXO)) != 0) {
errno = EACCES;
return (NULL);
}

Being false it effectively disables the permission check. Hence, for
example, world-writable directory will pass.

[Nicholas Marriott]

I'm not sure what the original intent of the change was and I don't
understand the commit message (below).

It seems like the second !S_ISDIR check is both unnecessary and
backwards... Thomas?

(Also I think we should probably still create tmux-%u under TMUX_TMPDIR,
so that is wrong too. Otherwise it is not secure if TMUX_TMPDIR is
shared.)


revision 1.123
date: 2013/10/10 12:03:22; author: nicm; state: Exp; lines: +5 -3;
Don't treat TMUX_TMPDIR as a potential file

The point of setting TMUX_TMPDIR is to then make any labels from -L go
to that directory. In the case of makesocketpath() with no TMUX_TMPDIR
set, would set both the path and the default socket to a file. The
checking of the permissions on the file worked fine in that case, but
when TMUX_TMPDIR is set, won't work on a directory.

This fixes the problem by ensuring the check on the permissions is
performed on directories only.

By Thomas Adam.

Post by Dmitri Paduchikh
Hello,
I am afraid that my first message wasn't clear. Not from first look at
least. Sorry about that. Let me elaborate, I think it is probably a
security bug. makesocketpath() checks !S_ISDIR(sb.st_mode); when true it
returns with error. But then it checks !S_ISDIR(sb.st_mode) in second
if (sb.st_uid != uid || (!S_ISDIR(sb.st_mode) &&
sb.st_mode & (S_IRWXG|S_IRWXO)) != 0) {
errno = EACCES;
return (NULL);
}
Being false it effectively disables the permission check. Hence, for
example, world-writable directory will pass.
------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
tmux-users mailing list
https://lists.sourceforge.net/lists/listinfo/tmux-users

[Thomas Adam]

On 18 January 2015 at 20:32, Nicholas Marriott

Post by Nicholas Marriott
I'm not sure what the original intent of the change was and I don't
understand the commit message (below).
It seems like the second !S_ISDIR check is both unnecessary and
backwards... Thomas?

Seems it was related to this: https://sourceforge.net/p/tmux/tickets/66/

Hmm. I thought the steps to reproduce that were clear; perhaps not,
and perhaps my patch is effectively useless.

-- Thomas Adam