Bug#698551: autopkgtest [spec]: spec may allow test names that escapes the "source directory"

2013-11-19 Thread Martin Pitt 
Hey Niels,

Niels Thykier [2013-01-20 12:50 +0100]:
> Test names are separated by whitespace and should contain only
> characters which are legal in package names, plus `/'.
> """
> 
> First, it is unclear to me what exactly is meant by "only characters
> which are legal in package names".  I read it as that any character
> legal in the package and addition to that the symbol "/".

Right, but that indeed seems to be an overzealous claim in the spec,
I'll fix that.  The adt-run code explicity disallows this, presumably
to avoid directory traversal problems as you mentioned:

if '/' in tname:
raise Unsupported(base[' lno'],
  'test name may not contain / character')

There is the "Tests-Directory:" field if you really want to put tests
into a different dir. That one must not be absolute; you can still do
tricks like "../../../etc/..", but as you say this is hardly a
security issue, so let's not overthink this.

But this also pointed out a different bug if you actually try this:

adt-run: unexpected, exceptional, error:
Traceback (most recent call last):
  File "/home/martin/debian/autopkgtest/runner/adt-run", line 1962, in main
process_actions()
  File "/home/martin/debian/autopkgtest/runner/adt-run", line 1935, in 
process_actions
act, os.path.join(act.arg, 'debian/tests/control'))
  File "/home/martin/debian/autopkgtest/runner/adt-run", line 1328, in 
read_control
t = Test(tname, base, act)
  File "/home/martin/debian/autopkgtest/runner/adt-run", line 1069, in __init__
raise Unsupported(base[' lno'],
KeyError: ' lno'

So, I'll write a test which reproduces this crash, makes sure that
tests with / are disallowed, and fix the spec.

Thanks,

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature

Bug#698551: autopkgtest [spec]: spec may allow test names that escapes the "source directory"

2013-01-20 Thread Niels Thykier 
Package: autopkgtest
Severity: normal

Hi,

I read the current autopkgtest draft[1] and I stumbled upon:

"""
  Tests:  [ ...]

[...]

Test names are separated by whitespace and should contain only
characters which are legal in package names, plus `/'.
"""

First, it is unclear to me what exactly is meant by "only characters
which are legal in package names".  I read it as that any character
legal in the package and addition to that the symbol "/".  According
to the Policy[2] that would be[3]:

  [a-z0-9\+-\./]+

Now this allows for tests called:

  /etc/origins/debian

  ../../../../etc/origins/debian


Even if my understanding of the original regex is wrong, it will almost
certainly allow:

  autopkgtest/../../../../../etc

It is hardly a security issue, as any (sane) attacker would just put
some malicious code in the test itself and be done with it.  However,
I would still like to have it clarified if the above test names are
intended to be valid.
  Perhaps it could be further restricted to state that all tests must
be contained within the unpacked source tree itself (i.e. if a test is
a symlink, the target must remain within the the source tree).

~Niels

[1] 
http://anonscm.debian.org/gitweb/?p=autopkgtest/autopkgtest.git;a=blob_plain;f=doc/README.package-tests;hb=HEAD

[2] http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Source

[3] It is possible that you intended it to be:

[a-z][a-z0-9\+-\./]+

Or some other variant thereof.


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org