Prev: block: replace BKL with global mutex
Next: [RFC PATCH] gcov: Add ARM eABI support
From: Alan Cox on 3 Jun 2010 05:40
> Past objections and rebuttals could be summarized as:

You've forgotten to update this with the list of the objections from your
last few days postings.

You've forgotten to update it as suggested so its a security policy


Do you plan to post this daily until we get fed up of seeing it ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Kees Cook on 3 Jun 2010 14:50
On Thu, Jun 03, 2010 at 10:41:49AM +0100, Alan Cox wrote:
> > Past objections and rebuttals could be summarized as:
>
> You've forgotten to update this with the list of the objections from your
> last few days postings.

I didn't think the recent discussions added anything thematically new.
"It changes how symlinks work" is a variation on "breaks POSIX", and
"should be done with per-user /tmp" is a variabtion on "userspace should
fix it". I can certainly reword the commit log, though.

> You've forgotten to update it as suggested so its a security policy

It is a sysctl with a CONFIG, which is what Eric Paris was asking for.
I apologize if I missed something, but if there are further improvements
desired, I'm happy to add patches.

> Do you plan to post this daily until we get fed up of seeing it ?

I plan on getting this functionality into the kernel. As such, whenever
I've been directed to improve it before it will be accepted, I will send
an updated version. Having the lifecycle of this patch blocking on me
seems counter-productive and slightly rude.

At this point, I believe I've addressed the specific concerns that Al Viro,
Eric Paris, and a few others pointed out. What else needs fixing?

Thanks,

-Kees

--
Kees Cook
Ubuntu Security Team
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Kees Cook on 3 Jun 2010 17:10
On Thu, Jun 03, 2010 at 01:02:48PM -0700, Eric W. Biederman wrote:
> Kees Cook <kees.cook(a)canonical.com> writes:
> > A long-standing class of security issues is the symlink-based
> > time-of-check-time-of-use race, most commonly seen in world-writable
> > directories like /tmp. The common method of exploitation of this flaw
>
> Nacked-by: "Eric W. Biederman" <ebiederm(a)xmission.com>
>
> This approach to fix the problem to of /tmp looks to me like it
> will have the opposite effect. I think this patch will encourage
> more badly written applications.

How to safely deal with /tmp has been well understood for well over
a decade. I don't think this change would "encourage" poor code.

> This patch does not actually fix the problem of recurring security
> issues in /tmp. Fundamentally /tmp is a design flaw. You have
> evidence that even the introduction of the sticky bit, and O_NOFOLLOW
> did not fix this design flaw, I don't see how yet another kernel feature
> will make finally make /tmp secure. The design flaw in /tmp is that we
> encourage storing files that we care little about (temporary files) in
> a location that is shared between everyone. From a security point of
> view sharing of files needs to be done very carefully, and as such
> should only be done for files that we really care about and intend to
> share.

I agree about /tmp being perilous. This change, however, fixes a class of
problems that exist now and has a simple solution.

> Furthermore the set of applications with security problems you are intending
> to address are privileged applications. There is no excuse not to audit
> and fix those applications. If you include this patch I see no reason why
> audits won't get more lax and other related security issues won't crop up.

If there were no excuse, then all software would be audited. This just
isn't the reality of our world. Auditing is not a binary process and
auditing for security issues is, unfortunately, not a high priority for
many projects or organizations. If auditing was critically important,
able to find all flaws, and there was no excuse not to do it, then Linux
would never have any security vulnerabilities. I do not believe in
perfect security; I believe in layered security. Find and fix what we
can when we can. This change provides coverage for all software running
on the kernel for this flaw.

> The real solution to this problem of inadvertently sharing things that are
> not safe to be shared is not to have a shared /tmp at all.
>
> Per user tmp can be as simple as:
> export TMPDIR=$HOME/tmp/
> chmod 0500 /tmp

Again, I agree the traditional /tmp should ultimately go away. It is,
however, not that simple today. I'm all for having distros go this
route and fix the various issues surrounding getting rid of the common
/tmp area. This patch is to solve one specific problem with no trade-off.

> So is your interest in actually fixing this problem, or do you just
> want to encourage more bad applications to be written without worrying
> about inadvertent sharing in /tmp?

I obviously don't believe it is one or the other. I want to fix /tmp
symlink races today and then see to fixing /tmp sharing next.

-Kees

--
Kees Cook
Ubuntu Security Team
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Al Viro on 4 Jun 2010 00:40
On Thu, Jun 03, 2010 at 11:40:54AM -0700, Kees Cook wrote:

> At this point, I believe I've addressed the specific concerns that Al Viro,
> Eric Paris, and a few others pointed out. What else needs fixing?

The hell you have. Let me spell it out for you:

1) You _still_ have not posted the analysis of changes it causes, let alone
explained why they are the right thing to do.

2) You are still doing that for each symlink, no matter where in the path
it might be. Do (1) and you'll see why it is a BS.

3) You have not bothered to explain why e.g. stat(2) should fail on such
symlinks. Nevermind figuring out which syscalls need that and which do
not. Again, (1) would be the starting point required for the rest. And
it is needed to decide how to deal with these checks. Really.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Kees Cook on 4 Jun 2010 02:30
On Fri, Jun 04, 2010 at 05:39:06AM +0100, Al Viro wrote:
> On Thu, Jun 03, 2010 at 11:40:54AM -0700, Kees Cook wrote:
>
> > At this point, I believe I've addressed the specific concerns that Al Viro,
> > Eric Paris, and a few others pointed out. What else needs fixing?
>
> The hell you have. Let me spell it out for you:
>
> 1) You _still_ have not posted the analysis of changes it causes, let alone
> explained why they are the right thing to do.

I have to say I don't know specifically what you want here. Do you
want runtime analysis? What sort of analysis? What would be meaningful
for you? I have a small testsuite[1] that I've been using to validate
my patch attempts. That's a form of analysis.

As for "why", I thought that was already pretty clear: stop the largest
class of malicious symlink races that read, write, or truncate files
through a single-depth symlink living in a sticky world-writable
directory.

I cannot know everything, but I can demonstrate that this method is
well tested in the traditionally security-hardened distros like OWL
and grsecurity. This isn't some new crazy idea, it's an old crazy idea
that happens to provide a solid protection. Could it be better? Maybe.
But let's work towards incremental improvement.

> 2) You are still doing that for each symlink, no matter where in the path
> it might be. Do (1) and you'll see why it is a BS.

I've asked for help on this, and I'm sorry I keep getting it wrong.
Based on your hints on earlier patches, I chose do_filp_open() for v4 of
the patch. It seems I was right, at least in that part, based on your
comment[2] on LWN that actually answers the question I asked earlier[3]
on lkml that went unanswered.

I'd really like to be improving this patch in one thread of discussion
instead of having to go collecting hints from multiple sources. Your
objection here now is that I still did it wrong. Can you please help?
This is the point of collaborative development, right? It seems to
me that it's really close (near "do_last", in the check for "trailing
symlink") so I'd really appreciate some better direction.

> 3) You have not bothered to explain why e.g. stat(2) should fail on such
> symlinks. Nevermind figuring out which syscalls need that and which do
> not. Again, (1) would be the starting point required for the rest. And
> it is needed to decide how to deal with these checks. Really.

Well, I think this is a bit of a tangent, but okay. I would say stat(2)
is blocked because it's inconsistent to allow stat but block open.
But I also feel like this is a trick question. :) I was originally for
blocking all following, since that doesn't seem to actually break any
valid use-cases. But I'm happy to more finely specify the limitations.
I was figuring it provided a more understandable behavior in the more
general case.

You mention in the later LWN comments:

> It's not a matter of overhead; it's not that large to start with. The
> real issue here is that you generally want the behaviour of system to
> make sense and its mental model to be understandable. I.e. the rationale
> for restrictions should be simple and specific; the more arbitrary they
> are, the worse.
>
> Again, I have no serious objections against that kind of restrictions.
> open()/creat() and probably truncate() on the final step of pathname
> resolution. But it really needs to be done right.

Can you propose a patch you're happy with? You seem to have a very clear
understanding of what you want to see that would fix this, and you know the
code much better than I do.

Thanks,

-Kees

[1] http://bazaar.launchpad.net/%7Eubuntu-bugcontrol/qa-regression-testing/master/annotate/head%3A/scripts/test-kernel-hardening.py#L62
[2] http://lwn.net/Articles/390889/
[3] http://lkml.org/lkml/2010/6/1/473

--
Kees Cook
Ubuntu Security Team
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: block: replace BKL with global mutex
Next: [RFC PATCH] gcov: Add ARM eABI support