Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nul in pathname #6117

Closed
p6rt opened this issue Mar 2, 2017 · 13 comments
Closed

nul in pathname #6117

p6rt opened this issue Mar 2, 2017 · 13 comments
Labels
Bug

Comments

@p6rt
Copy link

@p6rt p6rt commented Mar 2, 2017

Migrated from rt.perl.org#130900 (status was 'resolved')

Searchable as RT130900$

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

From zefram@fysh.org

spurt("foo\x[0]bar", "wibble")
True

The above writes to a file named "foo". It is less than awesome that
Perl 6 accepts "foo\x[0]bar" as a pathname, when it doesn't resemble
any pathname valid on the OS, and silently treats it as a quite
different pathname. It would be better for it to signal an error, that
"foo\x[0]bar" isn't a pathname. It's not an outright bug that it aliases
pathnames in this manner, but it is liable to confuse other code that
manipulates pathnames. Code like this​:

$*SPEC.splitpath("foo/bar\x[0]baz/quux").perl
("", "foo/bar\0baz/", "quux")

This *is* an outright bug. If the string containing a nul is to be
accepted as a way of stating a pathname, as it is by spurt(), then
IO​::Spec needs to know about that rule, and needs to parse pathnames
accordingly. The above should behave the same as splitting "foo/bar",
and thus should return ("", "foo/", "bar"). It could optionally retain
the nul and everything following it as part of the basename component
of the output, but though technically correct that would be confusing.
Of course, if spurt() et al did not accept nuls in pathnames, then
splitpath ought to error on such inputs too.

Watch out for "\x[0]\x[308]" when trying to detect nuls.

-zefram

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

From @timo

On Thu, 02 Mar 2017 02​:29​:27 -0800, zefram@​fysh.org wrote​:

Watch out for "\x[0]\x[308]" when trying to detect nuls.

-zefram

Thankfully, \0 won't accept diacritics, though i'd have to look at the NFG algo to see if that's valid for every diacritic in unicode, or just some subset.

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

The RT System itself - Status changed from 'new' to 'open'

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

From @geekosaur

On Thu, Mar 2, 2017 at 5​:29 AM, Zefram <perl6-bugs-followup@​perl.org> wrote​:

It is less than awesome that
Perl 6 accepts "foo\x[0]bar" as a pathname, when it doesn't resemble
any pathname valid on the OS

It's also less than awesome that POSIX, at least, doesn't let you confirm
that a filename is syntactically valid, either on the OS or on a specific
filesystem. And hardcoding that knowledge into Perl (any version) would be
a severe mistake. The code is a "bug" but Perl cannot be prescient for you,
or hack the kernel and filesystem drivers to find out what is valid.

--
brandon s allbery kf8nh sine nomine associates
allbery.b@​gmail.com ballbery@​sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

From @zoffixznet

FWIW, this bug makes at least mild exploitation possible, depending on how the program validates input​:

Setup​: dir and input check to ensure user-supplied path is not outside of it​:
  13​:41 IOninja m​: "/tmp/root/tmp".IO.mkdir; "/tmp/root/secret.txt".IO.spurt​: 'p4sswrd';
  13​:41 camelia rakudo-moar 9d138b​: ( no output )

Detection is triggered when `..` is supplied as the path​:
  13​:41 IOninja m​: chdir "/tmp/root/tmp"; my $user-input = ".."; $user-input.IO.resolve.starts-with​: $*CWD.resolve or die "hax!"; .say for dir $user-input
  13​:41 camelia rakudo-moar 9d138b​: OUTPUT​: «hax!␤ in block <unit> at <tmp> line 1␤␤»

But fails when we follow it with a nul byte, causing display of contents of parent directory​:
  13​:41 m​: chdir "/tmp/root/tmp"; my $user-input = "..\0"; $user-input.IO.resolve.starts-with​: $*CWD.resolve or die "hax!"; .say for dir $user-input
  13​:41 camelia rakudo-moar 9d138b​: OUTPUT​: «"..␀/tmp".IO␤"..␀/secret.txt".IO␤»
  13​:41 IOninja oops

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

From @geekosaur

To be clear​: the POSIX spec does, specifically, disallow NUL. *Only* NUL.

Which then leaves​:

- any character disallowed by specific filesystems (consider CIFS)
- the sequence `//` which is specifically undefined by POSIX

Among others. Is it correct to disallow NUL and thereby have a special case
where all other implementation-undefined filenames are not caught --- and
not catchable?

--
brandon s allbery kf8nh sine nomine associates
allbery.b@​gmail.com ballbery@​sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

From @geekosaur

On Thu, Mar 2, 2017 at 8​:56 AM, Brandon Allbery <allbery.b@​gmail.com> wrote​:

- the sequence `//` which is specifically undefined by POSIX

In the interests of heading off incoming pedanticness, possibly to "defend"
the indefensible​: this is only undefined as a prefix. It is an error as an
infix.

--
brandon s allbery kf8nh sine nomine associates
allbery.b@​gmail.com ballbery@​sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

From @toolforger

On 02.03.2017 14​:56, Brandon Allbery wrote​:

- any character disallowed by specific filesystems (consider CIFS)

Wouldn't these characters be caught by the file system (whichever that is)?

It would be pretty hard to do a reliable validation as soon as mounted
filesystems come into play.

Among others. Is it correct to disallow NUL and thereby have a special
case where all other implementation-undefined filenames are not caught
--- and not catchable?

Yes.
NUL is a precondition - you are not supposed to pass in a file name with
NULs in it, in theory that's nasal demon land, in practice most file
systems will truncate the file name at the first NUL (which gives rise
to all kinds of security issues for programs that are not aware of this
kind of behaviour and don't check for NULs in the string).

For the other invalid characters, the error will (well​: should) be
caught by the filesystem.
Trying to check that in advance is a security hole in waiting - somebody
might mount or unmount filesystems between check and use.

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

From @geekosaur

On Thu, Mar 2, 2017 at 9​:14 AM, Joachim Durchholz <jo@​durchholz.org> wrote​:

For the other invalid characters, the error will (well​: should) be caught
by the filesystem.
Trying to check that in advance is a security hole in waiting - somebody
might mount or unmount filesystems between check and use.

This, sadly, is not always true. The // prefix is a specific case in point.
CIFS/NTFS *does* catch invalid characters... unless the kernel filesystem
is buggy, which Linux's has been. Multiple times. The result of that is
files that you can write but fail to read back, sometimes on Linux and
sometimes on the remote / the Windows dual-boot, as appropriate. Again,
this knowledge *should not* be coded into perl; we can't be nannies for
Linux filesystem driver developers.

--
brandon s allbery kf8nh sine nomine associates
allbery.b@​gmail.com ballbery@​sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

From zefram@fysh.org

Brandon Allbery via RT wrote​:

It's also less than awesome that POSIX, at least, doesn't let you confirm
that a filename is syntactically valid,

At the syscall interface it's very simple​: any sequence of non-nul octets
may be used as a pathname. Since the pathname is passed in nul-delimited
form, this is not so much a rule about what's permitted as about what's
representable. Any rules beyond that about filename format (limited
length, limited character repertoire, or whatever) are enforced by the
kernel, and use of invalid names will produce appropriate syscall errors
(such as ENAMETOOLONG).

Perl 6 doesn't need to know about those rules that are enforced by the
kernel. The syscall errors are sufficient. What Perl 6 *does* need
to know about is the syntax at the syscall interface​: that a pathname
cannot contain a nul, and the manner in which a pathname splits into
filenames at directory separators.

-zefram

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 2, 2017

From zefram@fysh.org

Timo Paulssen via RT wrote​:

Thankfully, \0 won't accept diacritics,

Ah, quite right. I forgot about that rule. The text segmentation
standard says that there's a grapheme cluster boundary before and after
every control character, except in the middle of a CR-LF sequence.
Hence a nul codepoint (and thus a nul octet in UTF-8) cannot arise from
any grapheme cluster other than a bare nul.

-zefram

@p6rt
Copy link
Author

@p6rt p6rt commented Apr 10, 2017

From @zoffixznet

On Thu, 02 Mar 2017 02​:29​:27 -0800, zefram@​fysh.org wrote​:

spurt("foo\x[0]bar", "wibble")
True

The above writes to a file named "foo". It is less than awesome that
Perl 6 accepts "foo\x[0]bar" as a pathname, when it doesn't resemble
any pathname valid on the OS, and silently treats it as a quite
different pathname. It would be better for it to signal an error, that
"foo\x[0]bar" isn't a pathname. It's not an outright bug that it aliases
pathnames in this manner, but it is liable to confuse other code that
manipulates pathnames. Code like this​:

$*SPEC.splitpath("foo/bar\x[0]baz/quux").perl
("", "foo/bar\0baz/", "quux")

This *is* an outright bug. If the string containing a nul is to be
accepted as a way of stating a pathname, as it is by spurt(), then
IO​::Spec needs to know about that rule, and needs to parse pathnames
accordingly. The above should behave the same as splitting "foo/bar",
and thus should return ("", "foo/", "bar"). It could optionally retain
the nul and everything following it as part of the basename component
of the output, but though technically correct that would be confusing.
Of course, if spurt() et al did not accept nuls in pathnames, then
splitpath ought to error on such inputs too.

Watch out for "\x[0]\x[308]" when trying to detect nuls.

-zefram

Thank you for the report. This is now fixed in high-level IO routines and clases​:

  Fix​: rakudo/rakudo@e681498
  Tests​: Raku/roast@b16fbd3

For the case of $*SPEC.splitpath​: it's a low-level routine that most users won't
be using, unless they're developing their own path manipulation routines. So, in
that case, I'm leaving them without any nul detection, documenting the fact that
it needs to be done by the users of these low-level routines in​:

  Raku/doc@d436f3c

- IO grant

@p6rt
Copy link
Author

@p6rt p6rt commented Apr 10, 2017

@zoffixznet - Status changed from 'open' to 'resolved'

@p6rt p6rt closed this Apr 10, 2017
@p6rt p6rt added the Bug label Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant