Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=798071 Michael Scherer <misc@xxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |misc@xxxxxxxx --- Comment #1 from Michael Scherer <misc@xxxxxxxx> 2012-03-17 19:14:17 EDT --- I cannot sponsor you, but I can help to do review of the package ( not sure if I need to be a sponsor to do a formal review, so let's do a informal one for now, and I will check the wiki later ) 1) echo "#%PAM-1.0" >> pam/%{name} echo "auth include config-util" >> pam/%{name} is ugly, you should IMHO use heredocument : cat > pam/%{name} <<EOF #%PAM-1.0" >> pam/%{name} auth include config-util account include config-util session include config-util EOF 2) %clean rm -rf $RPM_BUILD_ROOT this is uneeded cf https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean ( as it was the same for every spec file on earth, it was set as default and should be removed, no need to keep useless code ) 3) %{_datadir}/%{name}/data/* this create unowned directory on installation that would not be removed on rpm removal or managed by rpm ( thus that could have weird permissions, etc ) so you should add either : %dir %{_datadir}/%{name} %dir %{_datadir}/%{name}/data or just : %{_datadir}/%{name}/ the 2nd one will take everything below, the first one would requires to precisely list what is in the directory. People recommend to do this usually, so no files are added without maintainer knowing it. ( of course, some maintainer do prefer the contrary ) 4) install -pm 0644 pam/* $RPM_BUILD_ROOT%{_sysconfdir}/pam.d/ ; rm -rf pam no need to clean the source in %install, this will be removed automatically by builder. 5) the tag Buildroot is not needed and should be removed 6) to finish, there is a security issue in your script, regarding log : def logfile(self, prefstri): return ("/tmp/faii." + prefstri + ".log") def binfile(self, prefstri): return ("/tmp/faii." + prefstri + ".bin") >From what I see, there is no verification performed on the file ( ie, does it exist before, is this a symlink to /etc/passwd ) and the program is run as root, so this permit to overwrite any file on the system. For example, if I create a directory in /tmp ( since /tmp/ is world writable ) called /tmp/faii.c.log, this could likely block you from using the software. Worst, if I do a link to /etc/passwd ( ln -s /etc/passwd /tmp/faii.c.log ), I think this will make the system unable to boot ( or a link to /dev/sda, should be fun too, or to /proc/sysrq-trigger ). Even without running as root, I could remove one of your file ( like, a thesis ) without you noticing. To correct that, the usual way os to create a temporary directory, and operate in it. You also need to create it with proper permission and atomically. See this module : http://docs.python.org/library/tempfile.html And this article would be a good explanation of the problem http://www.linuxsecurity.com/content/view/115462/151/ I would also suggest to create a helper script running as root to do only the necessary ( ie : write to the sdcard ) and make the rest as normal user. 7) as a side note, since you are the upstream, maybe you should directly ship the desktop in the upstream tarball. You should also take a look at using distribute, or setuptools for the distribution of the code. And a tarball named fedora-arm-installer-1.0.0.tar.gz would be better, as this is the name of the directory inside it, and permit to know the version of the code without opening it. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review