Bug 235121 - shells/rssh: rssh crashes on invocation due to new basename(3) POSIX behaviour
Summary: shells/rssh: rssh crashes on invocation due to new basename(3) POSIX behaviour
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Kai Knoblich
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-22 07:40 UTC by topical
Modified: 2019-03-11 16:09 UTC (History)
3 users (show)

See Also:
jharris: maintainer-feedback+


Attachments
basename(3): avoid unnecessary writing to passed string (316 bytes, patch)
2019-01-22 07:41 UTC, topical
no flags Details | Diff
rssh: pass string copy to basename(3) so it can safely modify it (709 bytes, patch)
2019-01-22 07:43 UTC, topical
no flags Details | Diff
comply POSIX standard when using basename, part 1 (766 bytes, patch)
2019-02-05 11:46 UTC, topical
no flags Details | Diff
comply POSIX standard when using basename, part 2 (869 bytes, patch)
2019-02-05 11:46 UTC, topical
no flags Details | Diff
comply POSIX standard when using basename, part 3 (370 bytes, patch)
2019-02-05 11:47 UTC, topical
no flags Details | Diff
comply POSIX standard when using basename, part 4 (688 bytes, patch)
2019-02-05 11:47 UTC, topical
no flags Details | Diff
shar patch - supersedes all others (3.06 KB, patch)
2019-02-16 20:36 UTC, jharris
jharris: maintainer-approval+
Details | Diff
rssh_with_basename_and_security_fixes.patch (11.16 KB, patch)
2019-03-03 21:29 UTC, Kai Knoblich
kai: maintainer-approval? (jharris)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description topical 2019-01-22 07:40:01 UTC
In FreeBSD 12.0, basename(3) was changed to be POSIX compliant. This implies that it can possibly write to the passed string.

rssh passes a const string, so it always crashes on invocation.

There are 2 solutions:

  * modify basename(3) so it only modifies the string if it has to
  * modify rssh, making a copy of the string first and pass it instead

The first solution may help other ports as well but I know that it will take some time until this change will be public. Meanwhile we could fix at least this single port by making it more POSIX compliant.
Comment 1 topical 2019-01-22 07:41:19 UTC
Created attachment 201321 [details]
basename(3): avoid unnecessary writing to passed string
Comment 2 topical 2019-01-22 07:43:00 UTC
Created attachment 201322 [details]
rssh: pass string copy to basename(3) so it can safely modify it
Comment 3 jharris 2019-02-03 05:25:21 UTC
Comment on attachment 201322 [details]
rssh: pass string copy to basename(3) so it can safely modify it

This patch looks good.  Approved.
Comment 4 jharris 2019-02-03 05:28:05 UTC
Comment on attachment 201321 [details]
basename(3): avoid unnecessary writing to passed string

There is no basename.c in the extracted work/rssh-2.3.4 directory, and we can't patch basename.c for the kernel, etc., so this won't work.  Not approved.
Comment 5 jharris 2019-02-03 05:35:58 UTC
topical, I think you'll want to add your same fix from util.c before all invocations of basename(3):

%grep -n basename *.c
log.c:96:       ident = strdup(basename((char*)name));
rssh_chroot_helper.c:162:       progname = strdup(log_make_ident(basename(argv[0])));
util.c:166:     char    *prog;          /* basename of cmd */
util.c:189:             prog = basename(cmd);


So also in log.c and rssh_chroot_helper.c, right?

Thanks!
Comment 6 Koichiro Iwao freebsd_committer 2019-02-05 01:55:21 UTC
Tell me when final fix is done and which is the final decision. Then I'll commit it to ports tree.
Comment 7 topical 2019-02-05 11:46:27 UTC
Created attachment 201752 [details]
comply POSIX standard when using basename, part 1
Comment 8 topical 2019-02-05 11:46:49 UTC
Created attachment 201753 [details]
comply POSIX standard when using basename, part 2
Comment 9 topical 2019-02-05 11:47:13 UTC
Created attachment 201754 [details]
comply POSIX standard when using basename, part 3
Comment 10 topical 2019-02-05 11:47:31 UTC
Created attachment 201755 [details]
comply POSIX standard when using basename, part 4
Comment 11 topical 2019-02-05 11:49:17 UTC
jharris, you are right - there are multiple invocations of basename. I tried to fix them all and created a patch set.
Comment 12 jharris 2019-02-16 20:36:12 UTC
Created attachment 202073 [details]
shar patch - supersedes all others

This final sharfile of 3 patches follows the original coding style in util.c and _attempts_ to be as clear as possible.  It supersedes all earlier patches on this PR.

topical, thanks for pointing out the problem with the port on FreeBSD 12 and your initial patches.

Tested on: 12.0-RELEASE-p3 GENERIC  amd64
Comment 13 Kai Knoblich freebsd_committer 2019-03-01 15:12:54 UTC
(In reply to jharris from comment #12)

Just a short question: The shar file seems to contain a bogus file (named "pa" with no content in it)? 

I left that file intentionally out and with the three other patches (patch-log.c, patch-rssh__chroot__helper.c and patch-util.c) it compiles so far fine on all FreeBSD releases (amd64 + i386).

If you can confirm that there's no patch is missing then everything is set to commit these files.
Comment 14 jharris 2019-03-01 17:31:46 UTC
(In reply to Kai Knoblich from comment #13)

Yes, ‘pa’ was the original patch file (attached to this bug) and should be removed.

Thanks!
Comment 15 Kai Knoblich freebsd_committer 2019-03-03 21:29:57 UTC
Created attachment 202528 [details]
rssh_with_basename_and_security_fixes.patch

Attached is an updated version for rssh. I did some testing on 11.2-, 12.0-RELEASE and 13.0-CURRENT@r344684. 

On 13.0-CURRENT shells/rssh didn't work as expected because there was a reference to unallocated memory (as far I can tell with my rudimentary C knowledge). The reason was in util.c:

> prog = basename(tmp2);
> free (tmp2);
> [...]

I have fixed that and placed the "free (tmp2)" after the if-contruct when the variable "prog" has been used for the string comparison.

The updated patch incorporates also four patches taken from Debian:

- 0003-Fix-buffer-allocation-buffer-for-fail-message.patch -> Included in patch-util.c
- 0007-Verify-rsync-command-options.patch (updated version with tighter syntax checking) -> Included in optional-patch_util.c
- 0009-Verify-scp-command-options.patch -> Included in patch-util.c
- 0010-Check-command-line-after-chroot.patch - Included in patch-rssh__chroot__helper.c

Patch "0009-Verify-scp-command-options.patch" addresses an actual CVE issue [1].

Then there's fact that upstream has abandoned shells/rssh since some years ago [2] and the software were solely maintained by an individual of Debian. But he will pull the plug for it, so rssh won't be in the next Debian stable release.

Thus I think it would be the best to deprecate the port in the FreeBSD ports tree.

What do you think? Are there any suggestions/objections?
--
[1] https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-1000018
[2] https://sourceforge.net/p/rssh/mailman/message/36525645/
[3] https://sourceforge.net/p/rssh/mailman/message/36530715/
Comment 16 topical 2019-03-04 20:11:46 UTC
Thank you for adding even more patches.

At least I use this packet intensively: my switches allow backing up their configuration by scp. So, avoid using scp altogether isn't really an option, but perhaps I can find another packet that does the same job.
Comment 17 Kai Knoblich freebsd_committer 2019-03-06 19:47:10 UTC
I'm going to commit the whole set of patches in some minutes because it's a security concern due the recently reported CVEs.

But I'm not adding a deprecation note, yet. I'll let the final decision to the maintainer.

@jharris: Would it be okay to set a deprecation date on shells/rssh? If so, what date should be set?
Comment 18 commit-hook freebsd_committer 2019-03-06 19:57:41 UTC
A commit references this bug:

Author: kai
Date: Wed Mar  6 19:56:57 UTC 2019
New revision: 494835
URL: https://svnweb.freebsd.org/changeset/ports/494835

Log:
  security/vuxml: Document shells/rssh < 2.3.4_2 vulnerabilities

  PR:		235121
  Approved by:	tcberner (mentor)
  Differential Revision:	https://reviews.freebsd.org/D19473

Changes:
  head/security/vuxml/vuln.xml
Comment 19 commit-hook freebsd_committer 2019-03-06 20:46:22 UTC
A commit references this bug:

Author: kai
Date: Wed Mar  6 20:45:28 UTC 2019
New revision: 494837
URL: https://svnweb.freebsd.org/changeset/ports/494837

Log:
  shells/rssh: Apply fixes for basename(3) handling and some security issues

  basename(3) has been changed to be POSIX compliant in r308264. This implies
  that it can possibly write to the passed string. shells/rssh passes a const
  string, so it always crashes on invocation with FreeBSD 12 and later. The
  new patches remedy this issue. [1] [2]

  During further tests and research came to light that there were also
  recently discovered security issues with the parsing of rsync/scp command
  line arguments and insufficient sanitization of environment variables when
  using rysnc.

  The corresponding fixes have been incorporated to the new patches and the
  already existing patch for the RSYNC option has been tightened for the
  argument parsing. Please note that with this patch the scp option "-3" can
  no longer be used. [3]

  Furthermore, another patch was applied to make this port a bit more secure.
  That patch handles a buffer allocation issue for an error message. [4]

  PR:		235121
  Submitted by:	topical@gmx.net (first version) [1], Jason Harris (maintainer) [2]
  Approved by:	tcberner (mentor)
  Obtained from:	Debian [3] [4]
  MFH:		2019Q1
  Security:	d193aa9f-3f8c-11e9-9a24-6805ca0b38e8
  Differential Revision:	https://reviews.freebsd.org/D19474

Changes:
  head/shells/rssh/Makefile
  head/shells/rssh/files/optional-patch-util.c
  head/shells/rssh/files/patch-log.c
  head/shells/rssh/files/patch-rssh__chroot__helper.c
  head/shells/rssh/files/patch-util.c
Comment 20 jharris 2019-03-07 01:11:03 UTC
(In reply to Kai Knoblich from comment #17)

Sure, it can be deprecated on whatever schedule is standard for these circumstances.

Thanks!
Comment 21 commit-hook freebsd_committer 2019-03-07 15:00:38 UTC
A commit references this bug:

Author: kai
Date: Thu Mar  7 14:59:37 UTC 2019
New revision: 494951
URL: https://svnweb.freebsd.org/changeset/ports/494951

Log:
  MFH: r494837

  shells/rssh: Apply fixes for basename(3) handling and some security issues

  basename(3) has been changed to be POSIX compliant in r308264. This implies
  that it can possibly write to the passed string. shells/rssh passes a const
  string, so it always crashes on invocation with FreeBSD 12 and later. The
  new patches remedy this issue. [1] [2]

  During further tests and research came to light that there were also
  recently discovered security issues with the parsing of rsync/scp command
  line arguments and insufficient sanitization of environment variables when
  using rysnc.

  The corresponding fixes have been incorporated to the new patches and the
  already existing patch for the RSYNC option has been tightened for the
  argument parsing. Please note that with this patch the scp option "-3" can
  no longer be used. [3]

  Furthermore, another patch was applied to make this port a bit more secure.
  That patch handles a buffer allocation issue for an error message. [4]

  PR:		235121
  Submitted by:	topical@gmx.net (first version) [1], Jason Harris (maintainer) [2]
  Approved by:	tcberner (mentor)
  Obtained from:	Debian [3] [4]
  Security:	d193aa9f-3f8c-11e9-9a24-6805ca0b38e8
  Differential Revision:	https://reviews.freebsd.org/D19474

  Approved by:	ports-secteam (riggs), mentors implicit

Changes:
_U  branches/2019Q1/
  branches/2019Q1/shells/rssh/Makefile
  branches/2019Q1/shells/rssh/files/optional-patch-util.c
  branches/2019Q1/shells/rssh/files/patch-log.c
  branches/2019Q1/shells/rssh/files/patch-rssh__chroot__helper.c
  branches/2019Q1/shells/rssh/files/patch-util.c
Comment 22 commit-hook freebsd_committer 2019-03-11 15:59:50 UTC
A commit references this bug:

Author: kai
Date: Mon Mar 11 15:59:03 UTC 2019
New revision: 495357
URL: https://svnweb.freebsd.org/changeset/ports/495357

Log:
  shells/rssh: Mark as deprecated and set to expire

  Upstream has officially abandoned the port some time ago and the software
  was maintained then for a while by the Debian project.

  But even Debian will now pull the plug and rssh won't be available in their
  next stable release. Considering these facts it makes sense to let this port
  expire towards the end of Q2.

  PR:		235121
  Approved by:	miwi (mentor), maintainer
  Differential Revision:	https://reviews.freebsd.org/D19503

Changes:
  head/shells/rssh/Makefile
Comment 23 Kai Knoblich freebsd_committer 2019-03-11 16:09:34 UTC
All changes have been committed, thank you all for the patches and feedback!