[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20111104163648.GA10002@openwall.com>
Date: Fri, 4 Nov 2011 20:36:48 +0400
From: Solar Designer <solar@...nwall.com>
To: oss-security@...ts.openwall.com
Subject: Re: CVE request: unsafe use of /tmp in multiple CPAN modules
John,
Thank you for reporting these!
On Fri, Nov 04, 2011 at 09:46:45AM -0500, John Lightsey wrote:
> PAR::Packer - PAR packed files are extracted to unsafe and predictable
> temporary directories
>
> https://rt.cpan.org/Public/Bug/Display.html?id=69560
I think that your description for this one happens to encourage a poor
fix for it. Specifically, starting the description by "par_mktmpdir()
makes no effort to verify that the /tmp/par-<username> directory is safe
to use" may result in this function being patched to do such checks,
which I think would be a poor fix. A better fix would be to properly
create a temporary files directory, with a less predictable name and
with due retries (with new names) if the directory already exists -
preferably using File::Temp's tempdir().
> Parallel::ForkManager - Insecure /tmp file handling
>
> https://rt.cpan.org/Public/Bug/Display.html?id=68298
Sounds like the classic issue there.
> File::Temp - _is_safe() allows unsafe traversal of symlinks
>
> https://rt.cpan.org/Public/Bug/Display.html?id=69106
This one is tricky. I am not convinced that it is CVE-worthy and that
your proposed fix is the right thing to do. To me, File::Temp's MEDIUM
and HIGH safety levels are questionable; if I were the maintainer, I
would probably not introduce these, or I'd explicitly document them as
safety and not security features. Here's what I mean by this subtle
distinction:
Basically, with these safety levels, File::Temp provides certain safety
checks against a Perl script author or user shooting themselves in the
foot. Then the question is whether those safety checks are supposed to
be exhaustive or not. In other words, whether this is merely safety
against inadvertent misuses or also security against malicious uses of
properly written scripts that actually deliberately rely on these safety
levels. Re-wording this even further, is a script that deliberately
relies on these safety levels for security properly written? I think
not. Maybe it'd make more sense to assign CVEs to such scripts.
Currently, the three safety levels (STANDARD, MEDIUM, and HIGH) are
documented to do certain specific checks, but the intended use and
whether those checks are supposed to be exhaustive (sufficient for
security) or not is unclear from the documentation. It can be said that
the behavior is consistent with documentation (it says what checks are
performed at what level, and as far as I'm aware this matches the code),
and thus there's no security vulnerability.
Specifically, HIGH only cares about parent directories if chown giveaway
is potentially allowed by the OS. To me, this is flawed logic (but it
is documented). Maybe this stuff was introduced when the initial
motivation behind the safety checks was forgotten (the safety vs.
security distinction), although that's just a guess. I am also guessing
that the logic when implementing HIGH was that without chown giveaway,
safe ownership and permissions on the target directory itself would seem
to imply that the directory was deliberately created by a trusted user,
and thus its parent directories were presumably trusted by that user
(and don't need to be checked by the script). This assumption works
fine for safety, but not for security. The question here is where the
pathname being checked comes from. Is it trusted input (and only needs
safety checks) or is it untrusted and potentially deliberately malicious
input (and thus needs exhaustive security checks)? No answer to this is
given in the documentation, that I can see. If the pathname is
potentially malicious, then on one hand parent directories need to be
checked regardless of chown giveaway availability, but on the other hand
even a safely-looking directory and full path leading to it do not imply
that it is safe to create arbitrary files in this directory (e.g., think
/etc/cron.d).
Thus, File::Temp's HIGH safety level makes no sense to me, and I see no
way to fix it. For safety, it is no better than MEDIUM (chown giveaway
is irrelevant in the safety context). For security, it is insufficient
and it can't be made sufficient.
As to the proposed fix (symlink-safety.patch), it partially helps in
certain special misuse cases. Namely, when the pathname is not
untrusted/malicious, but is poorly chosen, yet it contains just one
unsafe component. However, even in that case this fix doesn't protect
from hard-linking of an existing suitable symlink (of a trusted user)
into /tmp (possibly under a different name, although the symlink target
name remains that of the original symlink). And the limitation of
working for just one unsafe path component is no good; perhaps HIGH's
checks of parent directories would be better enabled unconditionally,
and even then this stuff is highly questionable.
Whatever we do or don't do to the code, I think the most important fix
would be to the documentation. We need to document that these are not
security features and that they must not be relied upon. Maybe we
should even deprecate HIGH because it doesn't make sense for any use.
> Batch::BatchRun - Unsafe /tmp file usage
>
> https://rt.cpan.org/Public/Bug/Display.html?id=69594
Another classic here.
Alexander
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
