Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 217311
Summary: | Review Request: xarchiver - Archive manager for Xfce | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Christoph Wickert <cwickert> |
Component: | Package Review | Assignee: | Patrice Dumas <pertusus> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | cq92j9y+rlkr0w, kevin, pertusus, splinux25 |
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | http://bugzilla.xfce.org/show_bug.cgi?id=2616 | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-12-17 03:14:55 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779, 215241 |
Description
Christoph Wickert
2006-11-26 22:45:18 UTC
*** Bug 198098 has been marked as a duplicate of this bug. *** * The LEGGIMI file should certainly be tagged with: %lang(it) %doc LEGGIMI * I think that the icon should also be in /usr/share/icons/hicolor/48x48/apps/ since it is specific of the size (I guess it should also be kept in /usr/share/pixmap for the about). Thanks for the comments, both fixed in SPEC: http://home.arcor.de/christoph.wickert/fedora/extras/review/SPECS/xarchiver.spec SRPM: http://home.arcor.de/christoph.wickert/fedora/extras/review/SRPMS/xarchiver-0.4.2-0.3.rc2.fc7.src.rpm More thoughts? Have you thought about my suggestion in bug #215241 comment #10? Unused dependency on sonames. I haven't investigated where they come from, most probable is .pc file not using correctly *.private (libm and libdl are certainly not problematic): /usr/lib/libatk-1.0.so.0 /lib/libm.so.6 /usr/lib/libpangocairo-1.0.so.0 /usr/lib/libpango-1.0.so.0 /usr/lib/libcairo.so.2 /lib/libgmodule-2.0.so.0 /lib/libdl.so.2 There is a requires for binutils, for ar, for .deb. Also reading src/deb.c, it seems to me that the files in /tmp are not safely created, opening the possibility of a symlink in tmp attack. Maybe the manipulation should be done in a /tmp subdir. I haven't checked the other /tmp use, some look clearly right, for others there is a need to look at the code. in src/callback.c, in xa_activate_link, I think it would be relevant to add a search from htmlview, and add a Requires for htmlview. Otherwise a Requires firefox could be used, but I think it would be much better to call htmlview. There is also a missing requires of cpio for rpm. Thanks for catching these. I added the missing Requires to the specfile. Also updated desktop-file-install to add more mime-types (application/x-ar, application/x-cd-image, application/x-deb, application/x-rpm, ...) to xarchiver.desktop. Yesterday I updated to 0.4.4, today I found 0.4.6 released. Fixes the icon issue from comment #2, no changes regarding the unused direct dependencies. Most of the xfce pacakges are really bad in this. SPEC: http://home.arcor.de/christoph.wickert/fedora/extras/review/SPECS/xarchiver.spec SRPM: http://home.arcor.de/christoph.wickert/fedora/extras/review/SRPMS/xarchiver-0.4.6-1.fc7.src.rpm I will contact upstream about the deb thing. I don't think the x-lha and x-lhz should be in mime in .desktop file since lha is not in fedora. I think it is the same for x-rar. I also don't think that deb and rpm should be included in the mime handled by xarchiver since it doesn't really seems to handle rpm or deb as rpm or deb but unarchive them. x-ar seems wrong to me too. Regarding htmlview, a patch is also needed, currently it is not used in the code! (In reply to comment #6) > I don't think the x-lha and x-lhz should be in mime in > .desktop file since lha is not in fedora. I think it is the > same for x-rar. I also don't think that deb and rpm should > be included in the mime handled by xarchiver since it > doesn't really seems to handle rpm or deb as rpm or deb but > unarchive them. x-ar seems wrong to me too. All of these mimetypes are included in file-roller and/or ark packages from Core, too. And why not adding xarchiver for rpms and debs? file-roller or ark also can't do more then list/extact debs/rpms, noone expects an achive manager to install packages. I use file-roller a lot to quickly look into rpms, get the spec from an srpm etc. The default action still is system-install-packages, so IMO adding xarchiver can't do no harm. > Regarding htmlview, a patch is also needed, currently it is > not used in the code! I created a small patch for htmlview, also added epiphany, konqueror and seamonkey. http://home.arcor.de/christoph.wickert/fedora/extras/review/SPECS/xarchiver.spec http://home.arcor.de/christoph.wickert/fedora/extras/review/SRPMS/xarchiver-0.4.6-2.fc7.src.rpm (In reply to comment #7) > All of these mimetypes are included in file-roller and/or ark packages from > Core, too. That's not a good reason. Since we don't ship lha and rar we shouldn't open the corresponding files. file-roller and ark are broken. Also remember that core packages haven't gone through review and may contain mistakes. This one is not an obvious mistake, it may have been done on purpose, but still it is a mistake in my opinion. x-ar isn't in the mimetype database. x-archive is, but since xarchive fails to open ar archives, I am not sure that it is right. > And why not adding xarchiver for rpms and debs? file-roller or ark > also can't do more then list/extact debs/rpms, noone expects an achive manager > to install packages. I use file-roller a lot to quickly look into rpms, get the > spec from an srpm etc. The default action still is system-install-packages, so > IMO adding xarchiver can't do no harm. Ok for rpm and debs, the system of default seems right to me. And if one don't have the default app (pirut for rpm) installed, why not xarchiver. I get a segfault with .a. It seems that it is detected as a .deb? (gdb) run Starting program: /usr/bin/xarchiver libz.a [Thread debugging using libthread_db enabled] [New Thread -1209128752 (LWP 2679)] Program received signal SIGSEGV, Segmentation fault. [Switching to Thread -1209128752 (LWP 2679)] 0x00c971eb in strlen () from /lib/libc.so.6 (gdb) bt #0 0x00c971eb in strlen () from /lib/libc.so.6 #1 0x077a4729 in g_strconcat () from /lib/libglib-2.0.so.0 #2 0x0805b91d in OpenDeb (archive=0xa1ddb48) at deb.c:32 #3 0x080580b7 in xa_open_archive (menuitem=0x0, data=0xa1ddb28) at callbacks.c:387 #4 0x0804f78d in main (argc=Cannot access memory at address 0x1 ) at main.c:235 #5 0x00c40e5c in __libc_start_main (main=0x804efb0 <main>, argc=2, ubp_av=0xbfa3d2a4, init=0x806a470 <__libc_csu_init>, fini=0x806a460 <__libc_csu_fini>, rtld_fini=0x727490 <_dl_fini>, stack_end=0xbfa3d29c) at libc-start.c:222 #6 0x0804df41 in _start () (gdb) After a quick reading of the code, the .a is indeed considered to be a debian archive because it begins with !<arch> like .deb. It should be better to test for !<arch>\ndebian (tests in callback.c line 1195), like libmagic does. But the segfault should be investigated prior. Maybe upstream should better use libmagic to detect file type? Also in /usr/share/file/magic one can see that there are more than one kind of debian archive. Seems like file-roller and ark don't use libmagic, though. Still on the subject of ar archives, ark and file-roller seems to handle them correctly, maybe it could be an interesting feature for xarchiver? Hey Patrice. Are you going to review this package? If not, I would be happy to do so, but it looks like you have already looked it over quite a bit... ;) I am indeed reviewing it, but I don't assign it to me until I am ready to approve, because there is a disagreament about the mimetypes, so if somebody disagrees with me on that subject, I don't want to stop him from reviewing the package. To summarize my position * I don't want to accept the package with the mimetypes for lha and rar without further discussion * the segfault with the .a is not a blocker to me for devel, but it is for other branches (it may open a security risk). * otherwise the package is right. #comment 10 is not blocking, it is more like suggestions for upstream. Sorry for slowing this down... (In reply to comment #8) > > x-ar isn't in the mimetype database. x-archive is, but since xarchive > fails to open ar archives, I am not sure that it is right. Of x-ar was wrong. My fault, if I did not add this mimetype most likely nobody would have tried to open an .a file ;) Also I should have tested it before. (In reply to comment #13) > > * I don't want to accept the package with the mimetypes > for lha and rar without further discussion As I have not tested lha I have also removed x-lha and x-lhz now. I still would like rar in. If required programm (from the other repo) is not installed, xarchiver will show a message that tells you to install it. This should IMHO be allowed. > * the segfault with the .a is not a blocker to me for > devel, but it is for other branches (it may open a > security risk). Can you please clairfy the symlink-attack problem from comment #4 a little? My hacking skills are too low to explain Guiseppe what you mean. Seems like he already noticed there's something not sane, see http://bugzilla.xfce.org/show_bug.cgi?id=2616 (In reply to comment #14) > As I have not tested lha I have also removed x-lha and x-lhz now. I still would > like rar in. If required programm (from the other repo) is not installed, > xarchiver will show a message that tells you to install it. This should IMHO be > allowed. Ok for rar. > Can you please clairfy the symlink-attack problem from comment #4 a little? My > hacking skills are too low to explain Guiseppe what you mean. Seems like he > already noticed there's something not sane, see > http://bugzilla.xfce.org/show_bug.cgi?id=2616 If a program creates a file below /tmp with a predictable name, it opens a possibility for this well known attack. In short an attacker have to create the conditions for a race condition by slowing down xarchiver, then creates a symlink in /tmp which overwrites a file. A longer story is: the attacker waits for you to begin opening a .deb, slows xarchiver, create a symlink in /tmp/ with the predictable name pointing to one of your file, and this file content will be overwritten by the newly created file content. A simple fix is to use mkdtemp or mkstemp to create the directory or the file with an unpredictable name. The opening of ar archives has been fixed in SVN r24084. What's left to do? I'll have to package the svn version. I'm not really sure that the segfault is really fixed. Update to 0.4.9-0.1.svn_r24096: - fixes segfault from comment #9 - xa now checks for debs correctly (comment #10) - remove all patches, included upstream now - remove mimetypes x-ar, x-lha and lx-hz - remove mimetype x-deb as long as debs are extracted to /tmp/data.tar.gz SPEC: http://home.arcor.de/christoph.wickert/fedora/extras/review/SPECS/xarchiver.spec SRPM: http://home.arcor.de/christoph.wickert/fedora/extras/review/SRPMS/xarchiver-0.4.9-0.1.20061213svn.fc6.src.rpm In the spec file comment, there is svn co http://svn.xfce.org/svn/goodies/xfce4-websearch-plugin/trunk xfce4-websearch-plugin I think it should be svn co -r24096 http://svn.xfce.org/svn/xfce/xarchiver/trunk xarchiver Otherwise * rpmlint gives: W: xarchiver mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 12) * name is right * follow guidelines * svn snapshot used for good reasons * .desktop file shipped * icons installed scriptlets used correctly * match upstream (verified with a diff) * %files section right needs work: Should the BR be gettext or gettext-devel? Currently it seems to need some autoconf macros from gettext-devel, but even after they are not needed anymore isn't gettext-devel needed? APPROVED, with the gettext question answered, and the proper comment for source generation added. Please, don't push to FC-6 or FC-5 until the security issue has been solved. (In reply to comment #19) > In the spec file comment, there is > svn co http://svn.xfce.org/svn/goodies/xfce4-websearch-plugin/trunk > xfce4-websearch-plugin Oops, thanks for catching this. > Should the BR be gettext or gettext-devel? Currently it seems > to need some autoconf macros from gettext-devel, but even after > they are not needed anymore isn't gettext-devel needed? Should be gettext for %find_lang. Because this package is a SVN snapshot it also BuildRequires xfce-dev-tools, which have a dependency on gettext-devel (I think you know, because you reviewed it) > APPROVED, with the gettext question answered, and the proper comment > for source generation added. Ok, will do that tomorrow, now I'm going to bed. I promise not to publish this for Core 5/6 until http://bugzilla.xfce.org/show_bug.cgi?id=2616 has been resolved. (In reply to comment #20) > Should be gettext for %find_lang. Because this package is a SVN snapshot it also > BuildRequires xfce-dev-tools, which have a dependency on gettext-devel (I think > you know, because you reviewed it) Right. 23856 (xarchiver): Build on target fedora-development-extras succeeded. Build logs may be found at http://buildsys.fedoraproject.org/logs/fedora-development-extras/23856-xarchiver-0.4.9-0.1.20061213svn.fc7/ Closing NEXTRELEASE Bug http://bugzilla.xfce.org/show_bug.cgi?id=2616 has been resolved. So when will xarchiver land on Fedora Extras for FC5/6? Please give me another two days. I'll need some more time to investigate the solution. Extracting debs now is safe, the file name no longer is predictable. CVS Sync is requested, archiver and thunar-archive-plugin should be in the repo soon. Package Change Request ====================== Package Name: xarchiver New Branches: EL-4 EL-5 Owners: cwickert cvs done. |