[MIR] curtin

Bug #1220434 reported by Scott Moser on 2013-09-03
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
curtin (Ubuntu)
Undecided
Unassigned

Bug Description

This is a main inclusion request for curtin.

 * I've gone through UbuntuMainInclusionRequirements there are no issues that I saw.
 * Rationale: curtin will be used by MAAS for doing fast installation of nodes.
 * Security: Source code review is requested of lpcurtin. many facets of security review are not all that important since the code is to be run as root and reinstall the operating system. Thats not to say that security is not an issue, just that things like privilege escalation are not of concern.
 * all dependencies of curtin are in main.

Michael Terry (mterry) wrote :

Assigning to ubuntu-security for requested security review.

Changed in curtin (Ubuntu):
assignee: nobody → Ubuntu Security Team (ubuntu-security)
Seth Arnold (seth-arnold) wrote :

I reviewed curtin version 0.1.0~bzr94-0ubuntu1 as checked into saucy. This
should not be considered a full security audit, but rather a quick gauge
of code quality.

- curtin is a quick-and-easy cloud image installer
- Build-depends on pep8, pyflakes, usual python, nose, yaml
- Depends upon util-linux, curl, wget
- Python script in /usr/bin/curtin
- No daemons
- No dbus
- No initscripts
- No setuid
- No sudo
- No udev
- No cron
- Small test suite, mostly checks configuration handling, runs at build
- Clean build logs
- Extensive process spawning, used arrays for arguments, had provisions
  for not logging details of privileged commands if needed
- Files are extensively opened, read, written, sometimes copied, nearly
  everything looked safe enough, small suggestions below
- One LOG.debug() call used supplied format string, everything else looked safe
- No environment use
- Intended to run as root, does not manage its own privileges
- No cryptography
- Does not itself do networking
- Uses mkdtemp() when temporary files are needed
- Does not use webkit
- Does not use javascript
- Does not use policykit

curtin is clear, well-written code; I believe maintenance of curtin should
be straight-forward enough and since it is intended to run with high
privileges and thus use configuration supplied by 'trusted users', should
have relatively small attack surfaces.

My biggest complaints are actually with the README.txt file -- no
integrity checking is performed on the downloaded images or a related
shell script.

Future versions should include the xkvm script in the package (perhaps in
/usr/share/doc/curtin/examples/ if that'd be more appropriate) and should
include verifying the authenticity of downloaded Ubuntu disk and
filesystem images.

Here's the issues I found, in roughly the order I'd suggest working on
them, in the hopes that my notes can save someone some time:

- doc/devel/README.txt does not check gpg signatures on -disk1.img or
  -root.tar.gz downloads
- doc/devel/README.txt downloads and does not check authenticity of
  http://sn.im/xkvm, places it in ~/bin and adds it to the instance's path
- is_mounted() can probably be confused by mountpoints with space, tab, or
  newlines in name; do_mount() looks like it will happily mount them, but
  do_umount() cannot umount them.
  'src' is completely unused. os.path.ismount() is probably safer.
- do_mount() 'src' is unused in is_mounted(), thus this routine may not
  give correct results
- meta_simple() writes 'ext4' to /etc/fstab regardless of --fstype
- cmd_install() unsafe logging method:
  LOG.debug(workingd.env())
- Lintian warnings:
  W: curtin: binary-without-manpage usr/bin/curtin
  E: python3-curtin: bad-provided-package-name python3:any
  E: python-curtin: bad-provided-package-name python:any
- write_file() it would be safer to replace os.chmod() with
  os.fchmod(fp.fileno())

Security team ACK for promoting curtain to main.

Thanks

Changed in curtin (Ubuntu):
assignee: Ubuntu Security Team (ubuntu-security) → nobody
Michael Terry (mterry) wrote :

Looked at packaging, seems fine. Just needs a team bug subscriber, and it can be approved.

Changed in curtin (Ubuntu):
status: New → Incomplete
Scott Moser (smoser) wrote :

Michael,
  Per https://bugs.launchpad.net/ubuntu/+source/curtin/+subscriptions , the ubuntu-server-team is subscribed to the bug.

Adam Conrad (adconrad) wrote :

Promoted to main.

Changed in curtin (Ubuntu):
status: Incomplete → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers