Bug 214881
| Summary: | jail with path=/ and sysctl.disablefullpath=1 leads to NULL dereference | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | aler | ||||||
| Component: | kern | Assignee: | freebsd-jail (Nobody) <jail> | ||||||
| Status: | Closed FIXED | ||||||||
| Severity: | Affects Only Me | CC: | jamie, kevans, kib | ||||||
| Priority: | --- | Keywords: | patch | ||||||
| Version: | 11.0-RELEASE | ||||||||
| Hardware: | Any | ||||||||
| OS: | Any | ||||||||
| Attachments: |
|
||||||||
(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. Created attachment 177482 [details]
Check for path == NULL.
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.
Created attachment 177588 [details]
patch to fix bugs
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? (In reply to Kyle Evans from comment #5) Just tested on 11.2-RELEASE - it still panics. Fixed for 12.2 release, even if it's too late for 11.X. It's no longer a problem in CURRENT, as debug.disablefullpath no longer exists. A commit references this bug: Author: jamie Date: Sat Aug 29 22:09:36 UTC 2020 New revision: 364969 URL: https://svnweb.freebsd.org/changeset/base/364969 Log: Fix a null dereference when debug.disablefullpath=1 and jail created with path=/. PR: 214881 Submitted by: aler (at) playground.ru Reported by: aler (at) playground.ru Changes: stable/12/sys/kern/kern_jail.c |
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.