On 2022-07-29, Rainer Weikusat <rweikusat@talktalk.net> wrote:
> Kaz Kylheku <480-992-1380@kylheku.com> writes:
>> On 2022-07-29, Tavis Ormandy <taviso@gmail.com> wrote:
>>> On 2022-07-29, Kaz Kylheku wrote:
>>>> But, on Linux, I'm not able to hard link a root-owned file
>>>> into /tmp. I tried this:
>>>>
>>>> # touch /tmp/foo # as root
>>>>
>>>> /tmp $ ln foo bar # as myself
>>>> ln: failed to create hard link 'foo' => 'bar': Operation not permitted
>>>>
>>>
>>> It's configurable, it's the fs.protected_hardlinks sysctl. AFAIK it's
>>> not the default, your distro probably sets it.
>>
>> Indeed it is; and the purpose of that sysctl is preventing exactly this
>> /tmp muckery.
>>
>> Making that configurable at all seems like a poor decision, not to
>> speak of the which way it defaults.
>
> Sudden changes of semantics which have existed for a long time are
> usually imprudent. People often don't appreciate "Of course, it doesn't
> work. But look how secure it is!"

But distros have turned this on and people don't even know about it.

I can hardly think of a valid use case for linking a file into /tmp
that you do not own!

Hard linking /your own file/ is allowed.

/tmp$ touch foo
/tmp$ ln foo bar

What is the use case for linking a file /that you do not own/ into /tmp?

That's very dubious given how /tmp works; in /tmp you're supposed to
access your own stuff and keep out of anyone else's.

You cannot remove (i.e. unlink) an object from /tmp you don't own; why
would the opposite operation, link, be allowed?

Also, root is allowed to hard link a non-root-owned file into /tmp; this
setting/behavior doesn't affect the superuser. Systems applications
running as superuser aren't affected by the restriction.

The case pretty solid for ending the cautious period of configurability
and making it a hard-coded behavior.

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal

On 2022-07-29, Kaz Kylheku <480-992-1380@kylheku.com> wrote:
> On 2022-07-29, Kaz Kylheku <480-992-1380@kylheku.com> wrote:
>> Basically whenever the safepath_check function encounters an absolute
>> path, either initially or via a symlink, it needs to check that path
>> against some unsafe patterns, like /proc/<digits>/cwd.
>
> Of course, that's not sufficient because of the many ways that same
> object can be named, like:
>
> /proc/net/../<digits>/fdinfo/../././cwd

OK; there is an updated version, git hash
2f27d6c386daff041017b7aaec51d0e50e603a8e.

The pattern check now simplifies paths, squeezing out "..", "." and
empty components; I wrote a string-to-string filtering function for
this. No file system access takes place.

The resulting simplified path is checked against a regular expression
via regexec, then thrown away. (It would be insecure to use the
simplified path for the main checks. because /good/good/evil/../good
simplifies to /good/good/good.)

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal

On 2022-07-30, Kaz Kylheku wrote:
> On 2022-07-29, Kaz Kylheku <480-992-1380@kylheku.com> wrote:
>> On 2022-07-29, Kaz Kylheku <480-992-1380@kylheku.com> wrote:
>>> Basically whenever the safepath_check function encounters an absolute
>>> path, either initially or via a symlink, it needs to check that path
>>> against some unsafe patterns, like /proc/<digits>/cwd.
>>
>> Of course, that's not sufficient because of the many ways that same
>> object can be named, like:
>>
>> /proc/net/../<digits>/fdinfo/../././cwd
>
> OK; there is an updated version, git hash
> 2f27d6c386daff041017b7aaec51d0e50e603a8e.
>

I think it's buggy, it doesn't handle "messy" symlinks correctly.

A symlink can be "messy" foo -> bar/, or "clean" foo -> bar.

I also think there is a security bug with multi-level symlinks.

If you do something like

$ ln -s /etc foo
$ ln -s foo bar

I think checking "bar/passwd" will actually test "barpasswd". I realize
this was just a bug and not a design flaw. I can take a look after you
fix it if you like it.

Do you consider attacks in s[ug]id context valid, or do you want
to call that out of scope? :)

Tavis.

--
_o) $ lynx lock.cmpxchg8b.com
/\\ _o) _o) $ finger taviso@sdf.org
_\_V _( ) _( ) @taviso

On 2022-07-30, Tavis Ormandy <taviso@gmail.com> wrote:
> On 2022-07-30, Kaz Kylheku wrote:
>> On 2022-07-29, Kaz Kylheku <480-992-1380@kylheku.com> wrote:
>>> On 2022-07-29, Kaz Kylheku <480-992-1380@kylheku.com> wrote:
>>>> Basically whenever the safepath_check function encounters an absolute
>>>> path, either initially or via a symlink, it needs to check that path
>>>> against some unsafe patterns, like /proc/<digits>/cwd.
>>>
>>> Of course, that's not sufficient because of the many ways that same
>>> object can be named, like:
>>>
>>> /proc/net/../<digits>/fdinfo/../././cwd
>>
>> OK; there is an updated version, git hash
>> 2f27d6c386daff041017b7aaec51d0e50e603a8e.
>>
>
> I think it's buggy, it doesn't handle "messy" symlinks correctly.
>
> A symlink can be "messy" foo -> bar/, or "clean" foo -> bar.
>
> I also think there is a security bug with multi-level symlinks.
>
> If you do something like
>
> $ ln -s /etc foo
> $ ln -s foo bar
>
> I think checking "bar/passwd" will actually test "barpasswd". I realize
> this was just a bug and not a design flaw. I can take a look after you
> fix it if you like it.

I put out two fixes; one for the buggy splicing (missing "+ 1"
in one fo the cases, causing the slash to be deleted).

I also revised the treatment of consecutive slashes, and trailing
slashes in link targets.

When a path component is delimited by a following slash, if there are
multiple slashes, the last of them is found and that then delimits the
component. E.g. foo///bar will delimit a "foo//" component, punching
a null byte temporarily over the third slash. The rest of the path
is then "bar". At the start of an absolute path, we skip all slashes,
not only one.

When a link target already ends with a trailing slash, and there
is a remaining path to be grafted on, we don't add a slash between
them; we take advantage of the target's trailing slash.

This also brings about conformance to a requirement stated in
POSIX like this

"If the contents of the symbolic link consist solely of <slash>
characters, then all leading <slash> characters of the remaining
pathname shall be omitted from the resulting combined pathname,
leaving only the leading <slash> characters from the symbolic link
contents."

I have no idea what that is actually for, but it would make
a difference in an implementation in which a name like //host/..
with exactly two slashes has a special meaning. Whereas
three or more slashes has the same meaning as one: ///host == /host.

So, with that requirement being obeyed you could make a symlink like

net -> //

and then net/host should produce the expected //host and not ///host.

> Do you consider attacks in s[ug]id context valid, or do you want
> to call that out of scope? :)

I care about the function being useful to a root user
(geteuid() == 0) case, and that includes when getuid() != 0,
which is important. A setuid root program should be able to use
the function for something (e.g. reading a config file that should
be secured) before dropping privileges.

I care more about setuid root than setuid non-root, and setuid
than setgid. :)

On this topic, I have a feature idea. There could be an API whereby the
application can declare friends. Friends are users or groups whom the
user trusts. For instance if Alice declares that Bob is a friend, then
Alice trusts path components that are controlled by Bob in the same way
as those controlled by Alice.

I believe this reflects the Unix model of sharing; Unix users can share
files in common directories only if they trust each other. If Bob has
shared a file with Alice, at any time, Bob can replace that with a
malicious symbolic link, just after Alice has checked that it isn't.

Friendship isn't recorded in Unix anywhere; the closest is the
group model. If some people trust each other, they can persuade
superuser to create a group which contains them.

There is no concept in Unix of selecting which groups you trust.
Superuser may assign you to various multiple groups. You might trust
some small group of three people of which you're a member, and regard a
directory writable to such a group as safe; yet you might not trust a
directory writable to "users"; a catch-all group to which hundreds of
users belong, most of whom you don't even know.

This is where the friendship API would include group friends also: you
could declare that you trust a certain group.

safepath_check has hard-coded trust for any group which contains
no members other than the effective user and superuser. In Linux
distros, there is a practice of parallel groups: like if you
create a user bob, a group bob is created and bob is its only
member; the ID numbers are synchronized also. safepatch_check
trusts this group (by virtue of it only containing the user).

A "~/.safepathrc" config file could make these friendship declarations
persistent. (Of course, .safepathrc will be checked with
safepath_check before being used. :)

There wouldn't be an /etc/safepathrc; friendship is personal
thing that can't be centrally dictated. :)

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal

On 2022-07-30, Kaz Kylheku wrote:
>> I think checking "bar/passwd" will actually test "barpasswd". I realize
>> this was just a bug and not a design flaw. I can take a look after you
>> fix it if you like it.
>
> I put out two fixes; one for the buggy splicing (missing "+ 1"
> in one fo the cases, causing the slash to be deleted).
>
> I also revised the treatment of consecutive slashes, and trailing
> slashes in link targets.

Thanks, seems okay to me.

>
>> Do you consider attacks in s[ug]id context valid, or do you want
>> to call that out of scope? :)
>
>
> I care more about setuid root than setuid non-root, and setuid
> than setgid. :)
>

Cool, in that case, here is another trick. If you open a descriptor to a
file and then unlink it, "(deleted)" is appended to the target by the
kernel. This allows a user to effectively change a symlink they don't own.

*but* here's the fun thing - it's not a real link, if you open it you
get the original unlinked file.

Reproduce like this, it requires fs.protected_hardlinks=0 (the default),
although I know another way to do it if you want to enforce that via
sysctl().

$ ln /etc/passwd '/tmp/foo (deleted)'
$ echo pwned > /tmp/foo
$ exec 3< /tmp/foo
$ rm /tmp/foo
$ ls -l /dev/fd/3
lr-x------ 1 taviso taviso 64 Jul 30 08:52 /dev/fd/3 -> '/tmp/foo (deleted)'

# file descriptor inherited, because it's not O_CLOEXEC:
$ ./testsp /dev/fd/3
safepath_check("/dev/fd/3") == path appears safe

# Look, it's not the real file!
$ cat /dev/fd/3
pwned

I think you have to concede it is quite difficult to use symlinks
safely, so jra did have a point :)

Tavis.

--
_o) $ lynx lock.cmpxchg8b.com
/\\ _o) _o) $ finger taviso@sdf.org
_\_V _( ) _( ) @taviso

On 2022-07-30, Tavis Ormandy <taviso@gmail.com> wrote:
> safepath_check("/dev/fd/3") == path appears safe

because /dev/fd is a symlink, this is the same as /proc/self/fd/3.

this falls under the regex, but isn't checked because i restricted the
check to euid == 0.

probably, no symlinks should be trusted under /proc no matter who
the effective user is.

that doesn't require a regex, just a flag indicating "we currently
processing a /proc absolute path", and fail every symlink if so.

The only problem is that /dev/fd/<n> is an important feature in
that it is used by bash process substitution.

probably "if we are in /proc, and see a symlink, don't trust
it if root, or else if the target contains (deleted)".

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal

On 2022-07-30, Kaz Kylheku <480-992-1380@kylheku.com> wrote:
> probably "if we are in /proc, and see a symlink, don't trust
> it if root, or else if the target contains (deleted)".

Unfortunately, that is not a good idea because it's an instant TOCtoTOU;
the link can change from "whatever" to "whatever (deleted)" at any time,
and that change can be perpetrated by a context that perpetrated
without that being conditional on the permissions of the /proc
directory where the link lives.

The only reasonable conclusion is that symlinks under /proc (and thus
/dev/fd) cannot be trusted. When safepath_check traverses /proc,
it must report almost any encounter with a symlink as unsafe.

Symlinks related to /proc's structure like /proc/self -> /proc/<n>
are probably safe; just the ill-conceived ones that reference external
files have to be blacklisted.

Programs cannot trust paths generated by Bash process substitution.

Even on Linux, Bash should be configured to implement process
substitution using named FIFOs.

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal

On 2022-07-30, Kaz Kylheku wrote:
> On 2022-07-30, Tavis Ormandy <taviso@gmail.com> wrote:
>> safepath_check("/dev/fd/3") == path appears safe
>
> because /dev/fd is a symlink, this is the same as /proc/self/fd/3.
>

Of course, but I don't understand the point - you're rejecting this attack?

It's okay, just want to make sure I understand the rules!

If you want to make things harder for me you could also disallow /tmp,
use close_range() and also require the other fs.protected_* sysctls! I
think this makes it not very useful, but that was Jeremy's point, using
symlinks safely is either hard or very restrictive!

Tavis.

--
_o) $ lynx lock.cmpxchg8b.com
/\\ _o) _o) $ finger taviso@sdf.org
_\_V _( ) _( ) @taviso

On 2022-07-30, Tavis Ormandy <taviso@gmail.com> wrote:
> On 2022-07-30, Kaz Kylheku wrote:
>> On 2022-07-30, Tavis Ormandy <taviso@gmail.com> wrote:
>>> safepath_check("/dev/fd/3") == path appears safe
>>
>> because /dev/fd is a symlink, this is the same as /proc/self/fd/3.
>>
>
> Of course, but I don't understand the point - you're rejecting this attack?

No; I was just making a note of that this falls under an existing
namespace already covered by a check. I already fixed it so that it's
applied regardless of the effective user ID.

> It's okay, just want to make sure I understand the rules!
>
> If you want to make things harder for me you could also disallow /tmp,

That's not actually such a bad idea. Programs should generally not
not open paths in /tmp that they didn't exclusively create themselves.

The module should have an API for white-listing paths. If some
path like /tmp/foo is somehow received from a trusted source, it could
be marked as trusted; then that could later be revoked again.

> use close_range() and also require the other fs.protected_* sysctls! I

Speaking of fs.protected_*: it's unfortunate that the unprivilged
user is not allowed to examine these settings! How crazy is that?

We we cannot ask the qeustion "are we on a Linux kernel with hard link
protection enabled?" (Though the hypothesis can be tested in
time-wasting, clumsy empirical ways.)

I just put in a bit of a countermeasure against hard link attacks: any
symlink with a hard link count > 1 is declared unsafe. The attacker must
be able to unlink the original object to bring the link count down to 1.
(Which may be possible if the attacker finds a suitable link in an
unsecured directory, but I have a sense that we are raising the bar
somewhat.)

> think this makes it not very useful, but that was Jeremy's point, using
> symlinks safely is either hard or very restrictive!

I'd say that using the file system interface of Linux safely is hard.

Jeremy is saying that the POSIX filesystem model is not safely usable;
but POSIX doesn't specify any of this /proc braindamage.

You cannot really blame the symlink, as such, for any of this.

Some of it is not even normal symlinks, like the dangling "/path/to/file
(deleted)" that magically points to a file that is not in the namespace.

(That could have been designed differently, while retaining the the same
approach: the link could rename to "/proc/deleted/path/to/file", where
/proc/deleted/ doesn't exist. The link would work in the same magic
way, but attackers have no way to create /proc/deleted/path/to/file.
User space doing its own symlink resolution would safely conclude it's a
dangling link.)

--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal

Kaz Kylheku <480-992-1380@kylheku.com> writes:
> On 2022-07-29, Rainer Weikusat <rweikusat@talktalk.net> wrote:
>> Kaz Kylheku <480-992-1380@kylheku.com> writes:
>>> On 2022-07-29, Tavis Ormandy <taviso@gmail.com> wrote:
>>>> On 2022-07-29, Kaz Kylheku wrote:
>>>>> But, on Linux, I'm not able to hard link a root-owned file
>>>>> into /tmp. I tried this:
>>>>>
>>>>> # touch /tmp/foo # as root
>>>>>
>>>>> /tmp $ ln foo bar # as myself
>>>>> ln: failed to create hard link 'foo' => 'bar': Operation not permitted
>>>>>
>>>>
>>>> It's configurable, it's the fs.protected_hardlinks sysctl. AFAIK it's
>>>> not the default, your distro probably sets it.
>>>
>>> Indeed it is; and the purpose of that sysctl is preventing exactly this
>>> /tmp muckery.
>>>
>>> Making that configurable at all seems like a poor decision, not to
>>> speak of the which way it defaults.
>>
>> Sudden changes of semantics which have existed for a long time are
>> usually imprudent. People often don't appreciate "Of course, it doesn't
>> work. But look how secure it is!"
>
> But distros have turned this on and people don't even know about it.
>
> I can hardly think of a valid use case for linking a file into /tmp
> that you do not own!
>
> Hard linking /your own file/ is allowed.
>
> /tmp$ touch foo
> /tmp$ ln foo bar
>
> What is the use case for linking a file /that you do not own/ into
> /tmp?

Someone who can write to a directory can create new directory entries.

> That's very dubious given how /tmp works; in /tmp you're supposed to
> access your own stuff and keep out of anyone else's.

And /tmp is just a directory.

> You cannot remove (i.e. unlink) an object from /tmp you don't own; why
> would the opposite operation, link, be allowed?

That's because /tmp usually has the so-called sticky bit set which stops
unlinking of files one doesn't own from directories one also doesn't
own.

> The case pretty solid for ending the cautious period of configurability
> and making it a hard-coded behavior.

Why?

I did a little internet search on this but the only issue I found was
that of a program which used the directory its executable happened to be
stored in to locate some supposedly trusted subprograms it would run
under certain circumstances. That's clearly broken as designed.

The patch which implemented this securit feature asserted that it would
solve actual problems but didn't really spell them out (minus claiming
that this would enable users to create backups of setuid-executables
with security holes in them which would make fixing these more
difficult). IMHO, that's another fine example of "Yes, we can!"-coding
by Google employees.