Bug 214881 - jail with path=/ and sysctl.disablefullpath=1 leads to NULL dereference
Summary: jail with path=/ and sysctl.disablefullpath=1 leads to NULL dereference
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 11.0-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-jail mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-11-27 14:56 UTC by aler
Modified: 2018-12-23 04:01 UTC (History)
3 users (show)

See Also:


Attachments
Check for path == NULL. (645 bytes, patch)
2016-11-28 13:25 UTC, Konstantin Belousov
no flags Details | Diff
patch to fix bugs (2.24 KB, patch)
2016-12-02 03:37 UTC, aler
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description aler 2016-11-27 14:56:27 UTC
How to reproduce:
 sysctl debug.disablefullpath=1
 jail / x 127.0.0.1 csh

Source of the problem:
/sys/kern/kern_jail.c, kern_jail_set(), lines near 930-1000 depending on source version (9.3, 10.x, 11.x, HEAD), "path" option handling branch

This sets path=NULL when it is "" or "/" and disablefullpath=1
936 	                error = vn_path_to_global_path(td, root, g_path, MAXPATHLEN);
937 	                if (error == 0)
938 	                        path = g_path;
939 	                else if (error == ENODEV) {
940 	                        /* proceed if sysctl debug.disablefullpath == 1 */
941 	                        fullpath_disabled = 1;
942 	                        if (len < 2 || (len == 2 && path[0] == '/'))
943 	                                path = NULL;

This dereferencing it:
954 	                if (fullpath_disabled) {
955 	                        /* Leave room for a real-root full pathname. */
956 	                        if (len + (path[0] == '/' && strcmp(mypr->pr_path, "/")
957 	                            ? strlen(mypr->pr_path) : 0) > MAXPATHLEN) {
958 	                                error = ENAMETOOLONG;
959 	                                vrele(root);
960 	                                goto done_free;
961 	                        }
962 	                }


Most likely it should release all things that it locked/allocated for path-handling after vfs_getopt(opts, "path", (void **)&path, &len) and jump out of this if() after setting path to NULL, but i'm not sure how exactly.
May be, comparsion of path with "/" is not in place and should be done after successful vn_path_to_global_path() too.
The whole "path" option handling branch code looks a bit weird to me.
Comment 1 Konstantin Belousov freebsd_committer 2016-11-28 13:24:47 UTC
(In reply to aler from comment #0)
I suspect that this is just a thinko, the case path == NULL is handled later.  OTOH, it is not clear to me that path == NULL should result in the '0' addend to len in the check.

If you do consider the right action to be cleanup, then the failing len check already provides an example what to do.  Anyway, I am attaching patch with simple additional check for path == NULL.
Comment 2 Konstantin Belousov freebsd_committer 2016-11-28 13:25:54 UTC
Created attachment 177482 [details]
Check for path == NULL.
Comment 3 aler 2016-12-02 03:34:30 UTC
Things apprear to be even worse. This code will overwrite root and leak old reference if path==NULL and root!=NULL.
1218                 if (path == NULL) {
1219                         path = "/";
1220                         root = mypr->pr_root;
1221                         vref(root);
1222                 }
And this exactly happens in case of disablefullpath=1 and path="/".
path==NULL means "nothing done for path"

Adding patch to do proper fix for all this.
Also i can note that pr->pr_path will be anyway unreliable in case of disablefullpath=1 and relative path given as argument (it doesn't event try to be). But i don't think it is important for rarely-used (if even used, don't know) debugging feature.
Comment 4 aler 2016-12-02 03:37:41 UTC
Created attachment 177588 [details]
patch to fix bugs
Comment 5 Kyle Evans freebsd_committer 2018-04-22 00:41:57 UTC
This appears to have gone by the wayside, and the originally pinpointed code still exists. Is this still an issue, or has it been mitigated in some other non-obvious way?
Comment 6 aler 2018-12-23 04:01:37 UTC
(In reply to Kyle Evans from comment #5)

Just tested on 11.2-RELEASE - it still panics.