"Adam D. Barratt" <a...@adam-barratt.org.uk> writes:
> Ah, yes, I missed that on my first skim through the diff due to the
> horrible indent level. :-/
Take a guess what was one of the first refactoring operations done to
the code base by new upstream :-)
> Looking through the diff again, there are a lot of changes which don't
> appear to be required and make the diff harder to review; one of the
> principles of stable updates is that diffs should be minimal wherever
> possible.
Understood; I was aiming for that, but I appreciate knowing where this
can be improved.
> Please could you prepare and send an updated diff which does not include
> the removal of old commented out code and the removal of functions from
> "import" lines where the code is no longer used?
Done now, and attached to this message.
diff -Nru burn-0.4.3/debian/changelog burn-0.4.3/debian/changelog
--- burn-0.4.3/debian/changelog 2009-08-23 15:12:43.000000000 +1000
+++ burn-0.4.3/debian/changelog 2009-08-27 13:40:56.000000000 +1000
@@ -1,3 +1,12 @@
+burn (0.4.3-2.2) stable; urgency=high
+
+ * Non-maintainer upload.
+ * Security fix for “TEMP-0542329 burn: Insecure escaping of file names”.
+ * Backport fix for secure handling of child process command arguments.
+ (Closes: Bug#542329)
+
+ -- Ben Finney <ben+deb...@benfinney.id.au> Thu, 27 Aug 2009 13:35:51 +1000
+
burn (0.4.3-2.1) unstable; urgency=medium
* Non-maintainer upload.
diff -Nru burn-0.4.3/debian/control burn-0.4.3/debian/control
--- burn-0.4.3/debian/control 2009-08-23 15:12:43.000000000 +1000
+++ burn-0.4.3/debian/control 2009-08-27 13:40:56.000000000 +1000
@@ -2,7 +2,7 @@
Section: otherosfs
Priority: optional
Maintainer: Gaetano Paolone (bigpaul) <bigp...@debian.org>
-Build-Depends: debhelper (>= 4.0.0)
+Build-Depends: debhelper (>= 4.0.0), quilt (>= 0.40)
Standards-Version: 3.6.0
Package: burn
diff -Nru burn-0.4.3/debian/patches/01.subprocess.patch burn-0.4.3/debian/patches/01.subprocess.patch
--- burn-0.4.3/debian/patches/01.subprocess.patch 1970-01-01 10:00:00.000000000 +1000
+++ burn-0.4.3/debian/patches/01.subprocess.patch 2009-08-27 13:40:56.000000000 +1000
@@ -0,0 +1,433 @@
+Author: Ben Finney <ben+deb...@benfinney.id.au>
+Description: Bug#542329: burn: Quotation marks in filenames aren't handled properly.
+ Use ‘subprocess’ module, with command argument sanitisation, for
+ all invocation and interaction with child processes.
+
+=== modified file 'burn'
+--- old/burn 2005-03-16 19:37:40 +0000
++++ new/burn 2009-08-27 03:14:53 +0000
+@@ -53,6 +53,8 @@
+ import ogg.vorbis
+ import gettext
+ import popen2, pwd
++import subprocess
++import shlex
+ import tty, termios, time, fnmatch
+
+ gettext.bindtextdomain('burn', '/usr/share/locale/it/LC_MESSAGES/')
+@@ -147,12 +149,15 @@
+
+ def check_media_empty():
+ """check if cdrom is empty using cdrdao"""
+- # giuppi's function
+- device = config.get('CD-writer','device')
+- driver = config.get('CD-writer','driver')
+- media_raw_info=popen2.popen2('%s disk-info --device %s --driver %s 2>/dev/null'%(cdrdao,device,driver))
++ command_args = [
++ cdrdao, "disk-info",
++ "--device", config.get('CD-writer', 'device'),
++ "--driver", config.get('CD-writer', 'driver'),
++ ]
++ command_process = subprocess.Popen(
++ command_args, close_fds=True,
++ stdout=subprocess.PIPE, stderr=open(os.devnull))
+- lines=media_raw_info[0].readlines()
+- for line in lines:
++ for line in command_process.stdout:
+ if line.startswith('CD-R empty'):
+ if line.split(':')[1].strip()=='yes':
+ return True
+@@ -175,12 +180,15 @@
+
+ def get_media_capacity():
+ """get media capacity using cdrdao"""
+- # giuppi's function
+- device = config.get('CD-writer','device')
+- driver = config.get('CD-writer','driver')
+- media_raw_info=popen2.popen2('%s disk-info --device %s --driver %s 2>/dev/null'%(cdrdao,device,driver))
+- lines=media_raw_info[0].readlines()
+- for line in lines:
++ command_args = [
++ cdrdao, "disk-info",
++ "--device", config.get('CD-writer', 'device'),
++ "--driver", config.get('CD-writer', 'driver'),
++ ]
++ command_process = subprocess.Popen(
++ command_args, close_fds=True,
++ stdout=subprocess.PIPE, stderr=open(os.devnull))
++ for line in command_process.stdout:
+ if line.startswith('Total Capacity'):
+ if line.split(':')[1].strip()=='n/a':
+ return None
+@@ -284,23 +292,20 @@
+ class ISO:
+ "ISO class"
+ path_o = []
+- mkisofs_line = ''
++ mkisofs_args = []
+ windows = config.get('ISO','windows_read')
+ tempdir = config.get('ISO','tempdir')
+ image_name = config.get('ISO','image')
+ mount_dir = config.get('ISO','mount_dir')
+ dest = normpath(tempdir + image_name)
+- def mkisofs_line_append(self, addenda):
+- """append string (addenda) to mkisofs_line"""
+- self.mkisofs_line = self.mkisofs_line + addenda
+- def mkisofs_line_view(self):
+- """view command in mkisofs_line"""
+- return self.mkisofs_line
+ def create(self):
+- """exec command in mkisofs_line"""
++ """ Execute data track recording command. """
+ print _('Creating temporary image: '), self.dest
+ pbar=progressBar(0,100,30)
+- stderr=popen2.popen3(self.mkisofs_line)[2]
++ command_process = subprocess.Popen(
++ self.mkisofs_args, close_fds=True,
++ stderr=subprocess.PIPE)
++ stderr = command_process.stderr
+ progress=0
+ while 1:
+ line=stderr.readline()
+@@ -315,7 +320,7 @@
+ print pbar, "\r",
+ sys.stdout.flush()
+ def destroy(self):
+- """exec command in mkisofs_line"""
++ """ Remove the image file. """
+ remove(self.dest)
+ def ask_mount(self, image):
+ """asks if user wants to mount image"""
+@@ -330,16 +335,18 @@
+ def mount(self, image):
+ """mount image in self.mount_dir"""
+ if exists(self.mount_dir) and isdir(self.mount_dir):
+- mount_loop = "mount -o loop " + image + " " + self.mount_dir
++ command_args = [
++ "mount", "-o", "loop",
++ image, self.mount_dir]
+- if system(mount_loop):
++ if subprocess.call(command_args):
+ print _('Unable to mount '), image, _('. Please check if you have permissions to mount it on '), self.mount_dir
+ sys.exit()
+ self.ask_after_mount(self.mount_dir)
+ else:
+ err(self.mount_dir + _(' is not valid as a mount point'))
+ sys.exit()
+- umount_loop = "umount " + self.mount_dir
+- system(umount_loop)
++ command_args = ["umount", self.mount_dir]
++ subprocess.call(command_args)
+
+ def ask_after_mount(self, dirname):
+ """Choose what to do with the mounted image..."""
+@@ -396,7 +403,7 @@
+ class CDROM:
+ "CDROM class"
+
+- cdrecord_line = ''
++ cdrecord_args = []
+ cdrdao=config.get('executables','cdrdao')
+ speed = config.get('CD-writer','speed')
+ device = config.get('CD-writer','device')
+@@ -411,7 +418,15 @@
+ if not empty:
+ if not options.multisession:
+ print _('Error. Please insert a blank CD/DVD.')
+- os.system('%s -eject dev=%s &>/dev/null'%(cdrecord,config.get('CD-writer','device')))
++ device = config.get(
++ 'CD-writer','device')
++ command_args = [
++ cdrecord, "-eject",
++ "dev=%(device)s" % vars()]
++ subprocess.call(
++ command_args,
++ stdout=open(os.devnull),
++ stderr=open(os.devnull))
+ sys.exit()
+
+ self.size = get_media_capacity()
+@@ -421,15 +436,9 @@
+ else:
+ self.size = config.get('Media','size')
+
+- def cdrecord_line_append(self, addenda):
+- """appen string (addenda) to cdrecord_line"""
+- self.cdrecord_line = self.cdrecord_line + addenda
+ def cdrecord_simulate(self):
+ """simulate burning"""
+- self.cdrecord_line = self.cdrecord_line + '-dummy '
++ self.cdrecord_args.append("-dummy")
+- def cdrecord_line_view(self):
+- """view command in cdrecord_line"""
+- return self.cdrecord_line
+ def size_compare(self, tobeburned, space_needed):
+ """Checks free space for temporary files and CD oversize"""
+ free_disk_space = int(iso.freespace())
+@@ -456,62 +465,62 @@
+
+ def line_create(self):
+ """cdrecord line generation"""
+- self.cdrecord_line = ''
+- self.cdrecord_line_append(cdrecord + ' -v -pad ')
++ self.cdrecord_args = []
++ self.cdrecord_args.extend([cdrecord, "-v", "-pad"])
+ #check for dummy (simulate)
+ if options.simulate:
+ self.cdrecord_simulate()
+ #check for eject
+ if options.eject:
+- self.cdrecord_line_append('-eject ')
++ self.cdrecord_args.append("-eject")
+ #check burning speed
+ if self.speed:
+- self.cdrecord_line_append('speed=' + self.speed + ' ')
++ self.cdrecord_args.append("speed=%(speed)s" % vars(self))
+ else:
+ print _('no burning speed defined, using 2x')
+- self.cdrecord_line_append('speed=2' + ' ')
++ self.cdrecord_args.append("speed=2")
+ #check burn device
+ if self.device:
+- self.cdrecord_line_append('dev=' + self.device + ' ')
++ self.cdrecord_args.append("dev=%(device)s" % vars(self))
+ else:
+ print _('no device specified.')
+ sys.exit()
+ #for the ones who have buffer underrun protection
+ if self.burnfree:
+- self.cdrecord_line_append('driveropts=burnfree ')
++ self.cdrecord_args.append("driveropts=burnfree")
+ #enable multisession
+ if options.multisession:
+- self.cdrecord_line_append('-multi ')
++ self.cdrecord_args.append("-multi")
+ if options.data_cd or options.iso_cd:
+- self.cdrecord_line_append('-data ' + iso.dest)
++ self.cdrecord_args.extend(["-data", iso.dest])
+
+ def create(self):
+- """exec command in cdrecord_line"""
++ """ Execute track recording command. """
+ print
+ print _('Press a key to begin recording...')
+ getch()
+ print _('Please wait...')
+- #print self.cdrecord_line
+- system(self.cdrecord_line)
++ #print self.cdrecord_args
++ subprocess.call(self.cdrecord_args)
+
+- def double_dao_create(self, cdrdao_line):
+- """exec command in cdrdao_line if you have TWO unit (reader and writer)"""
++ def double_dao_create(self, cdrdao_args):
++ """exec command in cdrdao_args if you have TWO unit (reader and writer)"""
+ print
+ print _('Place the source CD in the CD drive')
+ print _('and place a blank media in the recording unit.')
+ print _('Press a key to begin on-the-fly copy')
+ getch()
+- system(cdrdao_line)
+- #print cdrdao_line
++ subprocess.call(cdrdao_args)
++ #print cdrdao_args
+
+- def single_dao_create(self, cdrdao_line): #exec command in cdrdao_line
+- """exec command in cdrdao_line if you have only ONE unit (writer)"""
++ def single_dao_create(self, cdrdao_args):
++ """exec command in cdrdao_args if you have only ONE unit (writer)"""
+ print
+ print _('Place source CD in the recording unit.')
+ print _('Press a key to begin reading...')
+ getch()
+- system(cdrdao_line)
+- #print cdrdao_line
++ subprocess.call(cdrdao_args)
++ #print cdrdao_args
+
+ def another_copy(self):
+ """burn image untill user says no"""
+@@ -772,7 +781,7 @@
+ path = ''
+ path_preserved = ''
+ path_changed = ''
+- path_excluded = ''
++ paths_excluded = []
+ msinfo_values = ''
+ size = 0
+ first_time_multisession = 0
+@@ -834,12 +843,12 @@
+ if exists(join(root,i)):
+ if isfile(join(root,i)):
+ size = size - getsize(join(root,i))
+- path_excluded = path_excluded + '-x ' + '\'' + join(root,i) + '\'' + ' '
++ paths_excluded.append(join(root, i))
+ for d in dirs:
+ if fnmatch.fnmatch(d, x):
+ if exists(join(root,d)):
+ size = size - du(join(root,d), 0)
+- path_excluded = path_excluded + '-x ' + '\'' + join(root,d) + '\'' + ' '
++ paths_excluded.append(join(root, d))
+ print
+ print _('Size without exclusions: '), "\t", testsize/1048576, "Mb"
+
+@@ -851,18 +860,18 @@
+ sys.exit()
+ cdrom.size_compare(size, size)
+ #mkisofs line generation
+- iso.mkisofs_line_append(mkisofs + ' -R ') #-quiet
++ iso.mkisofs_args.extend([mkisofs, "-R"])
+ if exists(iso.tempdir):
+- iso.mkisofs_line_append('-o ' + iso.dest + ' ')
++ iso.mkisofs_args.extend(["-o", iso.dest])
+ else:
+ err(_('Error: ') + iso.tempdir + _(' does not exist'))
+ sys.exit()
+ #check Joliet option to be added
+ if iso.windows == 'yes':
+- iso.mkisofs_line_append('-J -joliet-long ')
++ iso.mkisofs_args.extend(["-J", "-joliet-long"])
+ #check exclude files - directories
+- if path_excluded:
+- iso.mkisofs_line_append(path_excluded + ' ')
++ for path_excluded in paths_excluded:
++ iso.mkisofs_args.extend(["-x", path_excluded])
+ if options.multisession:
+ multisession_choose = iso.ask_multisession()
+ if multisession_choose == 2:
+@@ -870,13 +879,20 @@
+ print _('Place target CD in CD/DVD writer unit and press a key...')
+ getch()
+ print _('Please wait...')
+- msinfo = commands.getstatusoutput(cdrecord + ' -msinfo dev=' + cdrom.device + ' 2>/dev/null')
+- iso.mkisofs_line_append(' -C ' + msinfo[1] + ' -M ' + cdrom.device + ' ')
++ command_args = [
++ cdrecord, "-msinfo",
++ "-dev=%(device)s" % vars(cdrom)]
++ command_process = subprocess.Popen(
++ command_args,
++ stdout=subprocess.PIPE, stderr=open(os.devnull))
++ msinfo = command_process.communicate()[0]
++ iso.mkisofs_args.extend(
++ ["-C", msinfo, "-M", cdrom.device])
+ elif not multisession_choose == 1:
+ sys.exit()
+ else:
+ first_time_multisession = 1
+- iso.mkisofs_line_append('-graft-points ' + global_path)
++ iso.mkisofs_args.extend(["-graft-points", global_path])
+ cdrom.line_create()
+ #a = cdrom.cdrecord_line_view()
+ #print a
+@@ -925,37 +941,43 @@
+ if cdrom.device == cdrom.source_device or cdrom.source_device == '':
+ single_drive_mode = 1
+ #print single_drive_mode
+- cdrdao_line = cdrdao + ' '
++ cdrdao_args = [cdrdao]
+
+ #if options.simulate:
+- # cdrdao_line = cdrdao_line + "simulate "
++ # cdrdao_args.append("simulate")
+ if single_drive_mode == 1:
+- cdrdao_line = cdrdao_line + "copy "
++ cdrdao_args.append("copy")
+ if options.simulate:
+- cdrdao_line = cdrdao_line + "--simulate "
++ cdrdao_args.append("--simulate")
+ if options.eject:
+- cdrdao_line = cdrdao_line + "--eject "
++ cdrdao_args.append("--eject")
+- cdrdao_line = cdrdao_line + "--datafile " + iso.dest + " --device " + cdrom.device + " "
++ cdrdao_args.extend([
++ "--datafile", iso.dest,
++ "--device", cdrom.device])
+ if not cdrom.driver == '':
+- cdrdao_line = cdrdao_line + " --driver " + cdrom.driver + " "
++ cdrdao_args.extend(["--driver", cdrom.driver])
+- cdrdao_line = cdrdao_line + " --speed " + cdrom.speed + " --fast-toc"
+- #print cdrdao_line
+- cdrom.single_dao_create(cdrdao_line)
++ cdrdao_args.extend(
++ ["--speed", cdrom.speed, "--fast-toc"])
++ #print cdrdao_args
++ cdrom.single_dao_create(cdrdao_args)
+ else:
+- cdrdao_line = cdrdao_line + "copy "
++ cdrdao_args.append("copy")
+ if options.simulate:
+- cdrdao_line = cdrdao_line + "--simulate "
++ cdrdao_args.append("--simulate")
+ if options.eject:
+- cdrdao_line = cdrdao_line + "--eject "
++ cdrdao_args.append("--eject")
+- cdrdao_line = cdrdao_line + "--device " + cdrom.device + " "
++ cdrdao_args.extend(["--device", cdrom.device])
+ if not cdrom.driver == '':
+- cdrdao_line = cdrdao_line + " --driver " + cdrom.driver + " "
++ cdrdao_args.extend(["--driver", cdrom.driver])
+- cdrdao_line = cdrdao_line + "--source-device " + cdrom.source_device + " "
++ cdrdao_args.extend(["--source-device", cdrom.source_device])
+ if not cdrom.source_driver == '':
+- cdrdao_line = cdrdao_line + "--source-driver " + cdrom.source_driver + " "
++ cdrdao_args.extend(
++ ["--source-driver", cdrom.source_driver])
+- cdrdao_line = cdrdao_line + " --speed " + cdrom.speed + " --on-the-fly --fast-toc "
+- #print cdrdao_line
+- cdrom.double_dao_create(cdrdao_line)
++ cdrdao_args.extend([
++ "--speed", cdrom.speed,
++ "--on-the-fly", "--fast-toc"])
++ #print cdrdao_args
++ cdrom.double_dao_create(cdrdao_args)
+ sys.exit()
+ #------------------------------------------------------
+ if mode == 4: #AUDIO
+@@ -1083,8 +1105,11 @@
+ dev=ao.AudioDevice('wav', filename=normpath(iso.tempdir + 'burn_' + `counter`) +'.wav', overwrite=True)
+ if ext == '.mp3':
+ if external_decod > 0:
+- mp3_line = mp3_decoder + ' ' + mp3_decoder_option + ' ' + normpath(iso.tempdir + 'burn_' + `counter`) + '.wav ' + "\"" + y + "\""
+- system(mp3_line)
++ command_args = [mp3_decoder]
++ command_args.extend(shlex.split(mp3_decoder_option))
++ command_args.append(normpath(iso.tempdir + 'burn_' + repr(counter) + '.wav'))
++ command_args.append(y)
++ subprocess.call(command_args)
+ else:
+ pbar=progressBar(0,100,30)
+ af = mad.MadFile(y)
+@@ -1104,8 +1129,11 @@
+ elif ext == '.ogg':
+ SIZE = 4096
+ if external_decod > 0:
+- ogg_line = ogg_decoder + ' ' + ogg_decoder_option + ' ' + normpath(iso.tempdir + 'burn_' + `counter`) + '.wav ' + "\"" + y + "\""
+- system(ogg_line)
++ command_args = [ogg_decoder]
++ command_args.extend(shlex.split(ogg_decoder_option))
++ command_args.append(normpath(iso.tempdir + 'burn_' + repr(counter) + '.wav'))
++ command_args.append(y)
++ subprocess.call(command_args)
+ else:
+ pbar=progressBar(0,100,30)
+ af = ogg.vorbis.VorbisFile(y)
+@@ -1135,15 +1163,12 @@
+
+ #----------------- cdrecord line generation -------------------------
+
+- #building cdrecord audio line
+- for x in audio_list:
+- tracks = tracks + '\"' + x + '\"' + ' '
+- #tracks = tracks + x + ' '
+ #creates cdrecord line
+ cdrom.line_create()
+ #appendig tracks to cdrecord line
+- cdrom.cdrecord_line_append('-audio ' + tracks)
+- a = cdrom.cdrecord_line_view()
++ cdrom.cdrecord_args.append("-audio")
++ cdrom.cdrecord_args.extend(audio_list)
++ a = cdrom.cdrecord_args
+ #print a
+ #burning CD
+ cdrom.create()
+
diff -Nru burn-0.4.3/debian/patches/series burn-0.4.3/debian/patches/series
--- burn-0.4.3/debian/patches/series 1970-01-01 10:00:00.000000000 +1000
+++ burn-0.4.3/debian/patches/series 2009-08-27 13:40:56.000000000 +1000
@@ -0,0 +1 @@
+01.subprocess.patch
diff -Nru burn-0.4.3/debian/rules burn-0.4.3/debian/rules
--- burn-0.4.3/debian/rules 2009-08-23 15:12:43.000000000 +1000
+++ burn-0.4.3/debian/rules 2009-08-27 13:40:56.000000000 +1000
@@ -3,8 +3,10 @@
#export DH_VERBOSE=1
export DH_COMPAT=3
+include /usr/share/quilt/quilt.make
+
build: build-stamp
-build-stamp:
+build-stamp: ${QUILT_STAMPFN}
dh_testdir
# Add here commands to compile the package.
@@ -12,7 +14,7 @@
touch build-stamp
-clean:
+clean: unpatch
dh_testdir
dh_testroot
rm -f build-stamp configure-stamp
@@ -33,9 +35,8 @@
chmod 0755 `pwd`/debian/burn/usr/bin/*
cp -p burn.conf `pwd`/debian/burn/etc
cp -p burn.conf-dist `pwd`/debian/burn/usr/share/burn/
-
+
touch install-stamp
-
# Build architecture-independent files here.
binary-indep: build install
--
\ “The truth is the most valuable thing we have. Let us economize |
`\ it.” —Mark Twain, _Following the Equator_ |
_o__) |
Ben Finney
