Bug 1679 - chroot and shell check ambiguity
Summary: chroot and shell check ambiguity
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: 5.3p1
Hardware: Other All
: P2 normal
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks: V_5_4
  Show dependency tree / graph
 
Reported: 2009-12-02 16:03 AEDT by Alex Beregszaszi
Modified: 2010-03-26 10:50 AEDT (History)
1 user (show)

See Also:

Attachments
Initial patch (1.62 KB, application/octet-stream)
2009-12-02 16:03 AEDT, Alex Beregszaszi 		no flags Details
Initial patch (1.63 KB, patch)
2009-12-04 12:21 AEDT, Alex Beregszaszi 		no flags Details | Diff
/home/djm/auth-check-chroot-shell.diff (1.96 KB, patch)
2010-01-13 23:25 AEDT, Damien Miller 		no flags Details | Diff
/home/djm/auth-check-chroot-shell.diff (1.83 KB, patch)
2010-01-13 23:28 AEDT, Damien Miller 		dtucker: ok+
Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Beregszaszi 2009-12-02 16:03:39 AEDT 
Created attachment 1737 [details]
Initial patch

OpenSSH will fail in a scenario where the server is configured with chroot and a shell used by a user is not available outside, just inside the chroot.

The reason behind this is that ssh checks whether the given shell is a file and is executable, but this check doesn't takes the chroot path into account.

I also attach an initial patch diffed against CVS (checked out few minutes ago), which I am not happy with but it can stand here as a record. I copied the chroot part from session.c
Comment 1 Alex Beregszaszi 2009-12-04 12:21:29 AEDT 
Created attachment 1738 [details]
Initial patch

Attached a wrong patch first.
Comment 2 Damien Miller 2010-01-13 23:25:59 AEDT 
Created attachment 1777 [details]
/home/djm/auth-check-chroot-shell.diff

tweaked patch
Comment 3 Damien Miller 2010-01-13 23:28:13 AEDT 
Created attachment 1778 [details]
/home/djm/auth-check-chroot-shell.diff

oops, patch without tabs vs. space issues
Comment 4 Darren Tucker 2010-01-13 23:35:44 AEDT 
Comment on attachment 1778 [details]
/home/djm/auth-check-chroot-shell.diff

>+	    strcasecmp(options.chroot_directory, "none") != 0) {

Do we do case-insensitive matches on "none"?
Comment 5 Damien Miller 2010-01-14 10:50:29 AEDT 
(In reply to comment #4)
> (From update of attachment 1778 [details])
> >+	    strcasecmp(options.chroot_directory, "none") != 0) {
> 
> Do we do case-insensitive matches on "none"?

We are inconsistent already, I'll send a diff to use case-insensitive comparisons everywhere.
Comment 6 Damien Miller 2010-01-14 11:02:05 AEDT 
(In reply to comment #4)
> (From update of attachment 1778 [details])
> >+	    strcasecmp(options.chroot_directory, "none") != 0) {
> 
> Do we do case-insensitive matches on "none"?

This is consistent with the other match against "none" for chrootdirectory, but in general we are not consistent in matching :(

Anyway, patch applied - this will be in OpenSSH 5.4 - thanks for the report and patch!
Comment 7 Darren Tucker 2010-03-26 10:50:45 AEDT 
With the release of 5.4p1, this bug is now considered closed.