[MIR] libnfs

[Jeremy Bícha]

Availability
============
Built for all supported architectures. In sync with Debian.

Rationale
=========
gvfs 1.24 added an NFS backend 3 years ago. It was enabled in Debian a year ago, but we can't enable it in Ubuntu until libnfs is promoted to main.

The Ubuntu bug for this feature to be enabled in gvfs is LP: #1637988

qemu-block-extra could also enable libnfs support.

Security
========
No known CVEs.

https://security-tracker.debian.org/tracker/source-package/libnfs
https://launchpad.net/ubuntu/+source/libnfs/+cve

Quality assurance
=================
- Desktop Packages team is subscribed.
- dh_auto_test runs but the tests shipped by upstream cant'b be run at build time since they modify the system.
- Autopkgtests are added in 3.0.0-1, running upstream's test suite which includes running tests with valgrind, too.

https://bugs.launchpad.net/ubuntu/+source/libnfs
https://bugs.debian.org/cgi-bin/pkgreport.cgi?src=libnfs
https://github.com/sahlberg/libnfs/issues

https://autopkgtest.ubuntu.com/packages/libn/libnfs (failing, never passed:
see https://bugs.debian.org/921578 )

https://ci.debian.net/packages/libn/libnfs/ (tests aren't run in Debian because they request machine isolation which isn't implemented there yet)

Dependencies
============
No universe binary dependencies

Standards compliance
====================
3.9.8, debhelper compat 9, dh7 simple rules

Maintenance
===========
Actively maintained:
https://github.com/sahlberg/libnfs

Not team maintained in Debian.
https://tracker.debian.org/pkg/libnfs

[Matthias Klose (doko)]

chatting with the Debian libnfs maintainer, the recommendation is to update to libnfs 2.x first. So maybe delay that for 18.10.

[Balint Reczey (rbalint)]

I updated the package to 2.0 but it is held up in experimental/NEW due to the SO bump. It needs a small transition in universe to have it in Bionic but I think it would be fairly smooth - no big symbol changes.

[Balint Reczey (rbalint)]

The package ended up in unstable thus apparently FTP Masters agreed with my opinion that license-wise the package is ok.

It can be synced to Ubuntu and we can run the small transition before Bionic's release.

I'm a bit concerned about potential memory handling issues, since libnfs had some in the past according to the commit logs. There are no known CVE-s, but past issues were not reported either and I think it would be prudent to give the package a security review before accepting it to main and enabling libnfs in gvfs.

[Matthias Klose (doko)]

2.0 now in the release pocket. The packging looks ok, so assigning to the security team for a review.

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[KeithG (grider-4)]

Postponing this for 18.10 would be a problem, IMO. Please include it in 18.04.x (LTS). This missing package not only does not allow a Nautilus browse, it also breaks deja dup nfs functionality as well.

[Adam Kosseck (tyderian)]

A lack of NFS support is a big issue for Enterprise deployments.

[Leonidas S. Barbosa (leosilvab)]

I reviewed libnfs (3.0.0-1) from disco

- Build dependencies:
 - debhelper, dh-autoreconf, libopt-dev

- Few issues on github
- NO CVE history
- no pre or postinst scritps
- no systemd unit files
- no system dbus services
- no setuid files
- Some binaries:
   /usr/bin/nfs-cp:
        Position Independent Executable: yes
        Stack protected: yes
        Fortify Source functions: yes
        Read-only relocations: yes
        Immediate binding: yes
   /usr/bin/nfs-cat:
        Position Independent Executable: yes
        Stack protected: yes
        Fortify Source functions: yes
        Read-only relocations: yes
        Immediate binding: yes

  /usr/bin/nfs-ls:
        Position Independent Executable: yes
        Stack protected: yes
        Fortify Source functions: yes
        Read-only relocations: yes
        Immediate binding: yes
- no sudo fragments on the code just in test/functions.sh line 11 and
15
- no udev rules
- It has dozens of tests, but I didn't see any of them be called during
build
- no cron jobs
- clean build log
- doesn't spawn other process. The only spawn happens in test files
(some .sh
  scripts)
- Memory mgmt looked like OK (in a first review), but cppcheck shows
some mem leaks, in a previous analyzes show it can be treat as irrelevant. Further considerations are welcomed.
   - [lib/nfs_v3.c:3106]: (error) Memory leak: cb_data
     [lib/nfs_v3.c:3115]: (error) Memory leak: cb_data
     [lib/nfs_v3.c:3473]: (error) Memory leak: cb_data
     [lib/nfs_v3.c:3482]: (error) Memory leak: cb_data
- File IO: some reads/open files, what seems to be internally to the
lib and lot of them in examples file.
- logging looked fine
- no envars
- does not use encryption
- does not use webkit
- does not use javascript

Said that, I'm ok in that package be promoted to main. Please feel free to re-review this points I made.

[Didier Roche-Tolomelli (didrocks)]

Updating the status to fix committed thus, after doko's and Leonidas' feedbacks.

[Sebastien Bacher (seb128)]

Override component to main
libnfs12 3.0.0-2 in disco amd64: universe/libs/optional/100% -> main
libnfs12 3.0.0-2 in disco arm64: universe/libs/optional/100% -> main
libnfs12 3.0.0-2 in disco armhf: universe/libs/optional/100% -> main
libnfs12 3.0.0-2 in disco i386: universe/libs/optional/100% -> main
libnfs12 3.0.0-2 in disco ppc64el: universe/libs/optional/100% -> main
libnfs12 3.0.0-2 in disco s390x: universe/libs/optional/100% -> main
Override [y|N]? y
6 publications overridden.

[Adam Kosseck (tyderian)]

Can this fix please be backported to Bionic

[Mathieu Trudel-Lapierre (cyphermox)]

I'm not sure I understand the request. This is a paperwork bug that was Fix Released, as libnfs was moved to the right location; it does not really affect anything for supported releases.

If you need some changes made to packages, please file a separate new bug with your specific request so that it can be acted upon.

[Eric Desrochers (slashd)]

@cyphermox,

It was "Fix Released" for disco only[1], can we complete it for stable releases ?

[1] - rmadison
 libnfs | 1.2.0-3 | precise/universe | source
 libnfs | 1.3.0-2ubuntu1 | trusty/universe | source
 libnfs | 1.9.8-1 | xenial/universe | source
 libnfs | 2.0.0-1~exp1 | bionic/universe | source
 libnfs | 2.0.0-1~exp1 | cosmic/universe | source
 => libnfs | 3.0.0-2 | disco | source

[Sebastien Bacher (seb128)]

I'm removing the lines for other series and using bug #1637988 for the SRU instead

[Leonidas S. Barbosa (leosilvab)]

Hello!

It was asked us Sec Team to review this pkg for cosmic and bionic (both version 2.0.0).
From the security team point of view it would be better if the version in these release was upgrade to version 3.0.0 as it is the same of disco and it was already MIRed/reviewed. So if it is possible it would be the best scenario, if not we can follow with the review for this version, but between 2.0.0 it probably has not only sec issues bug old nasty bugs that will need to be address.

Another point is, libnfs is already version 4.0.0 in upstream.
Attached the commits between the two version: 152 :)

[Jeremy Bícha (jbicha)]

Leonidas, upgrading from 2.0.0 to 3.0.0 bumps the soname and would require updates of kodi, mpd, and vlc. Normally, we don't do library transitions in SRUs. (libnfs11 → libnfs12)

[Łukasz Zemczak (sil2100)]

Looking at this with my SRU hat on, I'd prefer if we could avoid backporting a completely new upstream version to bionic especially if there is a soname bump involved. It's not impossible of course, we do that for certain projects when there's need for it, but I'd suppose we would require a 'hard' rationale for that. Not sure if we have a strong one like that here.

Security team - what changes/bugfixes do you think would be needed before the package is good for main in those stable series? Such information would also be very useful for us in the SRU team to assess the situation. We could then decide if the gvfs nfs support feature is a no-go, the libnfs security-bugfix changes need to be cherry-picked or maybe the rationale for 3.0.0 should be revisit.

Thank you.

[Matthias Klose (doko)]

On 19.03.19 16:20, Łukasz Zemczak wrote:
> Looking at this with my SRU hat on, I'd prefer if we could avoid
> backporting a completely new upstream version to bionic especially if
> there is a soname bump involved. It's not impossible of course, we do
> that for certain projects when there's need for it, but I'd suppose we
> would require a 'hard' rationale for that. Not sure if we have a strong
> one like that here.
>
> Security team - what changes/bugfixes do you think would be needed
> before the package is good for main in those stable series? Such
> information would also be very useful for us in the SRU team to assess
> the situation. We could then decide if the gvfs nfs support feature is a
> no-go, the libnfs security-bugfix changes need to be cherry-picked or
> maybe the rationale for 3.0.0 should be revisit.

the MIR team asked for the new upstream for promotion. Now promoting the old
version for older releases wouldn't be much appreciated.

[Balint Reczey (rbalint)]

@sil200 Just properly reviewing what needs to be backported seems to be more work than fixing the media players in older releases:

https://github.com/sahlberg/libnfs/compare/libnfs-2.0.0...sahlberg:libnfs-3.0.0

Most likely simple rebuilds of the rdeps would be enough.

[Balint Reczey (rbalint)]

@sil2100 ^

[Adam Conrad (adconrad)]

@doko To be fair, the MIR team asked for 2.0, which is what's in bionic and cosmic, not 3.0, which the security team is hoping for. That said, I'm not against the backport-and-rebuild-rdeps solution, if that looks to be the best way forward.

[Matthias Klose (doko)]

sorry, didn't the next soname bump.

[Leonidas S. Barbosa (leosilvab)]

Bellow is the review for 2.0.0 - cosmic and bionic. Pls feel free to add your considerations :)

build dependencies:
 - debhelper, dh-autoreconf, libnfs11

- No CVE history
- Security bugs found between 2.0.0...3.0.0:
   commit 486b74f64717dfb8bef774fc795636fa4faf4071

     Avoid underflow in readahead when offset < NFS_BLKSIZE

   commit 0c5732eb2605d2046e62b24cdc6439b7b94d06fc
   Author: Ronnie Sahlberg <email address hidden>
   Date: Sun Jul 2 07:48:56 2017 +1000

    Fix SEGV in rename error paths and add tests

    Signed-off-by: Ronnie Sahlberg <email address hidden>

- Other fixes I would consider, but I think you want to take a look also in the log between 2.0.0 ... 3.0.0 to add or be sure, are:

commit e8a200483f54f29eb3cd3311335c35df9fd755a4
Author: Shreyas Siravara <email address hidden>
Date: Mon Apr 23 12:29:59 2018 -0700

    Use MSG_NOSIGNAL when calling send() to avoid SIGPIPE

commit ea94d4e3a6d6947e2f239b015723bb4884f63b74
Author: Ronnie Sahlberg <email address hidden>
Date: Sat Jul 1 10:16:36 2017 +1000

    nfs_symlink: Fix it so we can create symlinks in the current directory

    Fix a bug in hte symlink code that required that linkname contained at lea...

[Eric Desrochers (slashd)]

I started to look how much effort the above fixes requirement will implies.

Most fixes involved modifying 'lib/nfs3.c' and 'lib/nfs_v3.c' not yet existing in current Bionic/Cosmic version (2.0.0) due to some 'code moving' and 'code addition'.

I start to doubt that it will be a good idea to backport fixes at this point as they seems to have been done on top on some quite major/important changes.

With that being said, I think we should re-consider and upgrade 3.0.0 + rebuild-rdeps.
Thoughts ?

# git log --diff-filter=A -- lib/nfs3.c
commit 0e0bb3fb34f5530e70b565998dc20d6aa11c11b0
Author: Ronnie Sahlberg <email address hidden>
Date: Wed Jun 28 06:31:44 2017 +1000

    NFSv4: Move all v3 specific code into its own nfsv3.c file

    Split out the nfsv3 code from libnfs.c into its own file.

    Signed-off-by: Ronnie Sahlberg <email address hidden>

# git describe --contains 0e0bb3fb34f5530e70b565998dc20d6aa11c11b0
libnfs-3.0.0~103

---

# git log --diff-filter=A -- lib/nfs_v3.c
commit 56194deeeebee9f5ff8545aa3882580f1c84780a
Author: Ronnie Sahlberg <email address hidden>
Date: Thu Jul 6 07:15:51 2017 +1000

    NFS4: Add support to nfs_mount()

    Signed-off-by: Ronnie Sahlberg <email address hidden>

# git describe --contains 56194deeeebee9f5ff8545aa3882580f1c84780a
libnfs-3.0.0~81

[Eric Desrochers (slashd)]

This MIR request has been abandoned a support point of view for C/B.