ncmpcpp (version < 0. 5.4) can cause unexpected deletion of files

[C de-Avillez]

Binary package hint: ncmpcpp

From an email to the Ubuntu bugSquad:

Dear Madam/Sir,

I am using Ubuntu 10.04 and I installed program ncmpcpp 0.4.1. from Ubuntu
repositories. There's bug described on the page http://unkart.ovh.org/ncmpcpp/:

ATTENTION: Feature, that allows you to physically remove directories from your
disk while being in browser is BROKEN IN ALL VERSIONS < 0.5.4 and may, under
some random circumstances, cause UNWANTED DELETION OF OTHER FILES. It needs to
be manually enabled in configuration file though, so if you don't use it, you're
fine. Otherwise you should upgrade to 0.5.4 or higher version immediately.

I used the feature, that can cause UNWANTED DELETION OF OTHER FILES. (I read
the ATTENTION notice too late.)

It removed about 1/2 of my HOME folder!!!!!

I would like to ask you, if you can remove this program from all
repositories, where it is included or fix the bug or use another version.

Yours sincerely

Natty already has ncmpcpp version 0.5.4-1, which is not affected.

All versions packaged for Ubuntu:

   ncmpcpp | 0.3.4-1 | karmic/universe | source, amd64, i386
   ncmpcpp | 0.4.1-1 | lucid/universe | source, amd64, i386
   ncmpcpp | 0.5.2-1 | maverick/universe | source, amd64, i386
   ncmpcpp | 0.5.4-1 | natty/universe | source, amd64, i386

TESTCASE:
/!\ ***WARNING*** This test case is destructive /!\

0. Install mpd and add music files into the music directory and verify that the user running ncmpcpp has write privileges on this directory. Configure ncmpcpp to point to this directory and allow files and directory deletion.
1. Start mpd
2. Start ncmpcpp
3. Switch to brower mode
4. Select the directory
5. Press the 'Del' key to delete the directory
=> Verify that some /random/ directory/files are deleted and that the deletion of the selected directory fails. Repeat until it deletes unselected files/directories
6. Install the version from -proposed
7. Repeat steps 2 to 5 and verify that only the selected directory is deleted.

[Alessandro Aragione (layn)]

More than a bug I think it's human error as specified in the notice of the package the risk of any unexpected deletion of files. Nevertheless, I think is actually not necessary keep packages harmful to the system in the repository, or possibly eliminate the risk reported by the package itself. In the meantime, please upgrade to the next version of the file for a possible reorganization of the file in Home.

[C de-Avillez (hggdh2)]

I disagree:
(1) the fact that this configuration option is not set on install does not mean it will not be set later on. It is an user's choice.
(2) if the configuration option is set there is a real risk of unexpected data loss.
(3) there are no newer versions available on Ubuntu (except for Natty, which is not really something we want casual users to run right now).

[Brian Murray (brian-murray)]

Additionally, the web page seems to indicate that:

" It needs to be manually enabled in configuration file though, so if you don't use it, you're fine."

[Marcel Stimberg (marcelstimberg)]

I have no idea why it is questioned whether this is a valid a bug or not...‽ The webpage clearly confirms the bug, users do not get any warning that they shouldn't set the option allow_physical_directory_deletion to yes (we can't expect users to go to the project's webpage after installing a package) and the bug "has a severe impact on a small portion of Ubuntu users" (i.e. fulfilling the criteria for importance "High").

[C de-Avillez (hggdh2)]

Allowing for physical dir removal was introduced on 0.3.5, so Karmic and earlier releases are OK., and Lucid/Maverick are the only releases affected.

[C de-Avillez (hggdh2)]

here's a simple change to disable the option.

[Alessandro Aragione (layn)]

At this point I can not help but agree with C de-Avillez and consider the matter closed. I thank you all for your kind interest. I await any purpose other opinions in question

[Alessandro Aragione (layn)]

Thank you for taking the time to make Ubuntu better. Since what you submitted is a Feature Request to improve Ubuntu, you are invited to post your idea in Ubuntu Brainstorm at [WWW] https://brainstorm.ubuntu.com/ where it can be discussed, voted by the community and reviewed by developers. Thanks for taking the time to share your opinion!

[C de-Avillez (hggdh2)]

This is a simple patch: I decided to disable 'allow_physical_directory_deletion' if it is set, and output an error message (which will be routed to the error.log file) *if* the current configuration file has 'allow_physical_directory_deletion = "yes"'.

This is minimally invasive, and I confirmed the option is reset if present in ~/.ncmpcpp/config.

I am still to look at the Lucid version.

[Artur Rona (ari-tczew)]

Thanks for the patch. You have to fix some issues, following with https://wiki.ubuntu.com/SecurityTeam/UpdatePreparation.

debian/changelog: - s/maverick/maverick-security
                              - please remove information about updated maintainer field. it's not necessary.

debian/patches/disable-dir-removal.patch: there are not DEP3 tags. Please complete it following with https://wiki.ubuntu.com/UbuntuDevelopment/PatchTaggingGuidelines.

Please resubscribe sponsors when patch is ready.

[C de-Avillez (hggdh2)]

Updated patch attached.

This fix does not apply to Debian: rmadison lists only 0.5.4 on squeeze and sid.

[C de-Avillez (hggdh2)]

And here is the debdiff for Lucid.

[C de-Avillez (hggdh2)]

Artur helped me on -motu, and pointed some more issues on the patches. I am uploading new versions for both Lucid and Maverick.

For the record, and as a consolidation of the data on this bug:

This bug has never been reported to Mitre; as such, there is no CVE associated with it. The security exposure here is arbitrary data loss, caused by mishandling a directory removal. This was announced on the developer's site, with the text:

"ATTENTION: Feature, that allows you to physically remove directories from your disk while being in browser is BROKEN IN ALL VERSIONS < 0.5.4 and may, under some random circumstances, cause UNWANTED DELETION OF OTHER FILES. It needs to be manually enabled in configuration file though, so if you don't use it, you're fine. Otherwise you should upgrade to 0.5.4 or higher version immediately.".

A quick look on the upstream GIT (http://repo.or.cz/w/ncmpcpp.git) does not clearly show a patch for this issue.

I am uploading two debdiff's:

Lucid-security: ncmpcpp_0.4.1-1ubuntu0.1.debdiff
Maverick-security: ncmpcpp_0.5.2-1ubuntu0.1.debdiff

The patch itself is minimally intrusive, and just disables the option after startup (and after the configuration file -- if any -- has been read and processed. This is the single point where the configuration file is read and acted on; after that, we print an error message to error.log (stderr redirected), and proceed as usual.

I have tested the patches.

[C de-Avillez (hggdh2)]

This is probably the fix in GIT upstream for this issue: http://repo.or.cz/w/ncmpcpp.git/commit/d1b82557d266795621244c62644d4d0604cf5453

I do not know if this is the single patch needed or not.

[Artur Rona (ari-tczew)]

ncmpcpp (0.5.4-1) unstable; urgency=high

  * New upstream release.
    + Set urgency to high because it fixes a potential deletion of directories.
  * Update copyright file.
  * debian/patches/charset-use-free-to-release-memory.patch
    + Remove patch since it is fixed in new release.
  * Switch to dpkg-source 3.0 (quilt).
    + Get rid of Build-Depends on quilt.
    + Remove include of CDBS quilt file.
 -- Ubuntu Archive Auto-Sync <email address hidden> Fri, 15 Oct 2010 09:47:44 +0000

[C de-Avillez (hggdh2)]

OK, I finally had time to get back to it. I am attaching a new debdiff for Maverick, now with the upstream patch for this issue. Indeed, at least for Maverick, the patch I identified on comment 15 above is all that is needed.

Tested on Maverick. I will look at Lucid now.

[Artur Rona (ari-tczew)]

MOTU-SWAT ACK.

Some concerns:
- There is Origin tag, so Applied-Upstream is not necessary.
- I'm not sure whether this is security issue, for me rather a SRU.

Are you going to prepare a patch for lucid?

[Steve Beattie (sbeattie)]

I concur with Artur's second concern; while this is a serious issue with this package, I don't believe it's a security issue, as I don't see a means for an attacker to cause the deletion to occur, though I haven't examined what the actual code issue is specifically. It seems to me that the SRU process is more appropriate route for a fix for this issue. I'm unsubscribing ubuntu-security-sponsers on this basis; please resubscribe if there's an indetifiable route for an outside third party to cause the deletion to occur.

[Artur Rona (ari-tczew)]

OK I'll sponsor patch for maverick.We are waiting for debdiff for lucid ;)

[Martin Pitt (pitti)]

Accepted ncmpcpp into maverick-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

[C de-Avillez (hggdh2)]

Sorry for the delay. I rebased the upstream patch (there was a small difference on functionality added on a post-0.4.1 level), and checked.

[Martin Pitt (pitti)]

Accepted ncmpcpp into lucid-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

[Jean-Baptiste Lallement (jibel)]

SRU verification for Lucid:
I have reproduced the problem with ncmpcpp 0.4.1-1 in lucid and have verified that the version of ncmpcpp 0.4.1-1ubuntu0.1 in -proposed fixes the issue.

Marking as verification-done

[Jean-Baptiste Lallement (jibel)]

SRU verification for Maverick:
I have reproduced the problem with ncmpcpp 0.5.2-1 in maverick and have verified that the version of ncmpcpp 0.5.2-1ubuntu0.1 in -proposed fixes the issue.

Marking as verification-done

[Launchpad Janitor (janitor)]

This bug was fixed in the package ncmpcpp - 0.4.1-1ubuntu0.1

---------------
ncmpcpp (0.4.1-1ubuntu0.1) lucid-proposed; urgency=low

  * debian/patches/incorrect-dir-removal.patch: cherry-pick upstream fix to
    arbitrary directories removal. This patch is for Lucid only -- had to be
    rebased due to a small difference in code. Already corrected on newer
    0.5.4 onwards. (LP: #663925)
 -- C de-Avillez <email address hidden> Sat, 19 Feb 2011 14:00:57 -0600

[Launchpad Janitor (janitor)]

This bug was fixed in the package ncmpcpp - 0.5.2-1ubuntu0.1

---------------
ncmpcpp (0.5.2-1ubuntu0.1) maverick-proposed; urgency=low

   * debian/patches/incorrect-dir-removal.patch: cherry-pick upstream fix to
     arbitrary directories removal. This patch is for Lucid and Maverick
     only, already corrected on newer 0.5.4 onwards. (LP: #663925)
 -- C de-Avillez <email address hidden> Wed, 20 Oct 2010 15:25:32 -0500