setting a password in loader.conf on 10.2-RELEASE+ machines, including 10.3-BETA1 causes loader to crash the boot with the following error message: Loading /boot/defaults/loader.conf can't load 'kernel' no valid kernel found
Created attachment 167348 [details] Works for me! uses getenv on 'kernelname' to see if the kernel is loaded (copied from other places in the code), if it is, calls load_kernel and load_modules (attempted with load_kernel_and_modules, but that was throwing a stack underflow, and I don't understand the code well enough to just throw things onto the stack, other parts of the code was also calling load_kernel and load_modules separately), and then continue and execute autoboot as normal, restoring the functionality from 10.1 and earlier. Testing under the following conditions: 1) no password set -- doesn't delay boot, or interfere with anthing as expected 2) password set -- autboot as designed, asks for password if interrupted. 3) password set and kernel loaded -- does not attempt to reload kenrel or modules, continutes with autoboot 4) zfs_load="yes" set in loader.conf loads kernel and zfs module, continues with autoboot. Untested:modules loaded, but no kernel. Is this even legal/supported?
Added dteske@ who is knowledgeable about this stuff.
Been over a year on this. Any progress made, by chance? Still seems to be a problem on 11.1-RELEASE, afaict.
Can we please get this in for 11.2? If not, could we please get an explanation for why not? Seems ludicrous to maintain a 2 line patch for multiple years here
Since this is a security function, I'd prefer a more complete explanation about why this works before pushing it in.
Backtrace for the problem: ... -> /boot/loader Loading /boot/defaults/loader.conf --> /boot/loader.rc: line 12: check-password ---> /boot/check-password.4th: line 164: autoboot can't load 'kernel' ... Here's a link to line 164 of check-password.4th: https://svnweb.freebsd.org/base/head/stand/forth/check-password.4th?view=annotate#l164 Annotation shows SVN r244158 (by me) for line 164. Here's a link to SVN r244158, (committed 12/12/12 at 12P EST, for some trivia), which introduced the call to autoboot in check-password function: https://svnweb.freebsd.org/base?view=revision&revision=244158 Line 164 (which has remained unchanged since its introduction) isn't the problem, but rather a commit that came after SVN r244158 introduced the problem, as we'll see below. Stepping up the stack in the above backtrace, to line 12 of /boot/loader.rc, we can gain some insights by comparing the state of that code circa revision 244158 to head. Link to /boot/loader.rc with sticky-bit set on revision 244158: https://svnweb.freebsd.org/base/head/sys/boot/i386/loader/loader.rc?annotate=151874&pathrev=244158 Link to /boot/loader.rc head revision: https://svnweb.freebsd.org/base/head/stand/i386/loader/loader.rc?view=annotate /boot/loader.rc used to call start before check-password, and now it calls initialize instead of start. The change from "start" to "initialize" in /boot/loader.rc was done in SVN r257650 (almost a year later, on 11/4/2013): https://svnweb.freebsd.org/base?view=revision&revision=257650 This commit (SVN r257650) neglected to address check-password.4th at the time that kernel loading was deferred (wherein "start" was changed to "initialize" in /boot/loader.rc). Now, to bring it home, the core difference between "start" and "initialize" is seen in /boot/loader.4th: https://svnweb.freebsd.org/base/head/stand/forth/loader.4th?view=annotate Lines 142 to 179 are where start and initialize are declared. initialize does everything that start does except load the kernel and the modules. So the patch provided is not far from what is needed. However, in the patch provided, you don't need the following: s" kernelname" getenv? false = But instead you should use: any_conf_read? As an aside, it might be tempting to use the flag left on the stack from initialize, but that would require changing the check-password function to only conditionally consume a stack item. Conditional consumption is never a good idea, and so you would then reason that check-password would always take a stack flag. This would be fine if check-password weren't documented in a man-page so now we're talking about changing the documented Forth API. So I do believe leaving the flag from initialize (which is from "any_conf_read?" as-is needed above) alone and instead re-running "any_conf_read?" to produce a fresh stack flag for the "if load_kernel load_modules then" routine.
SVN r257650 was MFC'd all the way back to stable/9, so in planning for the fix I plan to similarly merge the fix back to stable/9. The scope of the security implications can be described as ... --- Topic: deferred kernel loading breaks loader password Affects: All supported version of FreeBSD running i386 and amd64. Corrected: (when the patch goes in) I. Background Prior to SVN r257650 all platforms loaded the kernel and modules using "start" in /boot/loader.rc. After SVN r257650 the i386 and amd64 platforms started using "initialize" in place of "start" to defer loading of the kernel allowing the user to select which kernel was loaded. When the loader password is enabled as documented in loader.conf(5), the menu is not loaded and instead the machine goes into the autoboot routine. II. Problem Description The autoboot routine fails when the kernel has not yet been loaded. This yields a loader prompt where the user has full control of the boot process. III. Impact This is counter to the desired effect by having had set a password in loader.conf(5) -- you want to prevent interruption of the boot process. The other impact is that the machine did not boot as expected. IV. Workaround To boot the machine, you can type "boot" and press ENTER. To restore the ability to prevent the user from interrupting the boot process without first entering the password stored in loader.conf(5), the system administrator had to patch /boot/check-password.4th manually. V. Solution The pending patch (which is not entirely dissimilar to the patch submitted to this PR) restores the ability to boot with a loader password and prevent user interruption of the boot process without said password. --- I expect a full security advisory will have to be made for this patch once it is made so that people with existing systems can have instructions on how to do that. However, I am not on secteam, and the above information is just a step in the direction of helping secteam understand the scope of the [pending] patch with respect to security.
With respect to the pending patch, I am defining the test surface required before finalizing the patch (which needs packaging once finailzation). So far I have successfully tested my candidate patch on 11.1 amd64, but I next need to test non-x86 where /boot/loader.rc still has the "start" preamble instead of the problematic "initialize" preamble.
Everything has been tested and the (one-line) patch is finalized. Just waiting on secteam
Yes, I saw the difference between start and initialize, but I believe that was intentional, as to defer kernel loading to let the menu load fist and give users (especially operating on for 'out of box' experience) a good positive feedback; especially if you had to fall back to an older version (in which case you just lost all of that time) So I deliberately put it with autoboot vs making initialize and start operate the same
err, not IN autoboot, next to autoboot
(In reply to david from comment #10) start vs initialize not the problem. SVN r257650 was incomplete -- it was missing a patch to check-password.4th.
SVN r257650 was never tested with a password set.
Agreed; just that r257650 is the reason that I believe 'start' and 'initialize' differ in that action, and making them the same will undo that? You mention a proposed patch? I didn't see anything to base/head/stand/... Could you point me at the proposed patch? I think a diff will be worth 1000 words to me.
(In reply to david from comment #14) initialize was introduced by dcs@ 19 years ago and remains largely unchanged: https://svnweb.freebsd.org/base?view=revision&revision=47198 r257650 did not change start/initialize Why you don't see the commit in base/head/stand/... yet: The EN (Errata Notice) has to be created/filed by secteam@ before the commit is made because commits are watched closely and we try to prevent premature disclosure in situations that impact security. The one-line patch is: --- stand/forth/check-password.4th.orig 2018-06-24 08:12:19.642888000 -0700 +++ stand/forth/check-password.4th 2018-06-25 22:47:58.245592000 -0700 @@ -129,7 +129,7 @@ variable readlen \ input len again \ Enter was not pressed; repeat ; -only forth definitions also password-processing +only forth definitions also password-processing also support-functions : check-password ( -- ) @@ -161,6 +161,7 @@ only forth definitions also password-pro \ We should prevent the user from visiting the menu or dropping to the \ interactive loader(8) prompt, but still allow the machine to boot... + any_conf_read? if load_kernel load_modules then 0 autoboot \ Only reached if autoboot fails for any reason (including if/when
I guess technically it's a "two-line patch" ;D (wasn't counting the vocabulary include)
Ohhhh... ok, I see; that basically is my patch with the difference of "any_conf_read?" Ok, I am all good now.. well, mostly.. What is 'any_conf_read?', semantically, what does that mean?
any_conf_read? is a variable declared in support.4th. It defaults to false (0) and indicates if a conf file was successfully read. We don't load a kernel unless we read at least one config file. It is set to true (-1) by process_conf_errors in support.4th which is called by include_conf_files and include_nextboot_file. The primary config file is /boot/defaults/loader.conf which defines loader_conf_files by default containing /boot/device.hints /boot/loader.conf /boot/loader.conf.local (in that order). It also contains the default kernel name to load. Your patch was to test if $kernelname is set in the environment, which only gets set when a kernel has been loaded. While this seems logically OK, it would be better to leverage consistency by using the same logic that is present in start/initialize which is to check the any_conf_read? boolean instead.
Got it. Yeah, I struggled a lot with how to tell which state I was in at that point and 'do the right thing' for a few different cases; so not surprising to me that I didn't get all of the nuance there; I will go re-read things here to get a better understanding (even though I hope to not touch this part of the system again!)
A commit references this bug: Author: dteske Date: Sun Oct 21 00:15:51 UTC 2018 New revision: 339509 URL: https://svnweb.freebsd.org/changeset/base/339509 Log: Restore the ability to prevent the user from interrupting the boot process without first entering the password stored in loader.conf(5). PR: kern/207069 Reported by: david@dcrosstech.com MFC after: 3 days Sponsored by: Smule, Inc. Changes: head/stand/forth/check-password.4th
A commit references this bug: Author: dteske Date: Wed Oct 24 23:13:53 UTC 2018 New revision: 339696 URL: https://svnweb.freebsd.org/changeset/base/339696 Log: MFC r339509: Fix loader.conf(5) "password" feature Restore the ability to prevent the user from interrupting the boot process without first entering the password stored in loader.conf(5). PR: kern/207069 Reported by: david@dcrosstech.com Approved by: re (rgrimes) Sponsored by: Smule, Inc. Changes: _U stable/12/ stable/12/stand/forth/check-password.4th
A commit references this bug: Author: dteske Date: Wed Oct 24 23:17:18 UTC 2018 New revision: 339697 URL: https://svnweb.freebsd.org/changeset/base/339697 Log: MFC r339509: Fix loader.conf(5) "password" feature Restore the ability to prevent the user from interrupting the boot process without first entering the password stored in loader.conf(5). PR: kern/207069 Reported by: david@dcrosstech.com Sponsored by: Smule, Inc. Changes: _U stable/11/ stable/11/stand/forth/check-password.4th
A commit references this bug: Author: dteske Date: Wed Oct 24 23:21:12 UTC 2018 New revision: 339698 URL: https://svnweb.freebsd.org/changeset/base/339698 Log: MFC r339509: Fix loader.conf(5) "password" feature Restore the ability to prevent the user from interrupting the boot process without first entering the password stored in loader.conf(5). PR: kern/207069 Reported by: david@dcrosstech.com Sponsored by: Smule, Inc. Changes: _U stable/10/ stable/10/sys/boot/forth/check-password.4th
A commit references this bug: Author: dteske Date: Wed Oct 24 23:31:34 UTC 2018 New revision: 339699 URL: https://svnweb.freebsd.org/changeset/base/339699 Log: MFC r339509: Fix loader.conf(5) "password" feature Restore the ability to prevent the user from interrupting the boot process without first entering the password stored in loader.conf(5). PR: kern/207069 Reported by: david@dcrosstech.com Sponsored by: Smule, Inc. Changes: _U stable/9/ stable/9/sys/boot/forth/check-password.4th
Updated EN candidate text (requiring "???" to be replaced with actual data): --- Topic: deferred kernel loading breaks loader password Category: core Module: loader Announced: (pending) Affects: All supported version of FreeBSD running i386 and amd64. Corrected: 2018-10-24 23:13:53 UTC (stable/12, 12.0-???) 2018-10-24 23:17:17 UTC (stable/11, 11.???) 2018-10-24 23:21:12 UTC (stable/10, 10.???) 2018-10-24 23:31:33 UTC (stable/9, 9.???) For general information regarding FreeBSD Errata Notices and Security Advisories, including descriptions of the fields above, security branches, and the following sections, please visit <URL:https://security.FreeBSD.org/>. I. Background Prior to SVN r257650 all platforms loaded the kernel and modules using "start" in /boot/loader.rc. After SVN r257650 the i386 and amd64 platforms started using "initialize" in place of "start" to defer loading of the kernel allowing the user to select which kernel was loaded. When the loader password is enabled as documented in loader.conf(5), the menu is not loaded and instead the machine goes into the autoboot routine. II. Problem Description The autoboot routine fails when the kernel has not yet been loaded. This yields a loader prompt where the user has full control of the boot process. III. Impact This is counter to the desired effect by having had set a password in loader.conf(5) -- you want to prevent interruption of the boot process. The other impact is that the machine did not boot as expected. IV. Workaround To boot the machine, you can type "boot" and press ENTER. To restore the ability to prevent the user from interrupting the boot process without first entering the password stored in loader.conf(5), the system administrator had to patch /boot/check-password.4th manually. V. Solution Perform one of the following: 1) Upgrade your system to a supported FreeBSD stable or release / security branch (releng) dated after the correction date. Afterward, reboot the system. 2) To update your system via a binary patch: Systems running a RELEASE version of FreeBSD on the i386 or amd64 platforms can be updated via the freebsd-update(8) utility: # freebsd-update fetch # freebsd-update install Afterward, reboot the system. 3) To update your system via a source code patch: The following patches have been verified to apply to the applicable FreeBSD release branches. a) Download the relevant patch from the location below, and verify the detached PGP signature using your PGP utility. # fetch https://security.FreeBSD.org/patches/EN-18:???/???.patch # fetch https://security.FreeBSD.org/patches/EN-18:???/???.patch.asc # gpg --verify ???.patch.asc b) Apply the patch. Execute the following commands as root: # cd /usr/src # patch < /path/to/patch c) Recompile your kernel as described in <URL:https://www.FreeBSD.org/handbook/kernelconfig.html> and reboot the system. VI. Correction details The following list contains the correction revision numbers for each affected branch. Branch/path Revision - ------------------------------------------------------------------------- stable/9/ r339699 stable/10/ r339698 stable/11/ r339697 stable/12/ r339696 - ------------------------------------------------------------------------- To see which files were modified by a particular revision, run the following command, replacing NNNNNN with the revision number, on a machine with Subversion installed: # svn diff -cNNNNNN --summarize svn://svn.freebsd.org/base Or visit the following URL, replacing NNNNNN with the revision number: <URL:https://svnweb.freebsd.org/base?view=revision&revision=NNNNNN> VII. References The latest revision of this advisory is available at <URL:https://security.FreeBSD.org/advisories/FreeBSD-EN-18:???.???.asc> The pending patch (which is not entirely dissimilar to the patch submitted to this PR) restores the ability to boot with a loader password and prevent user interruption of the boot process without said password.
(In reply to Devin Teske from comment #25) Ignore the tail-end -- this text should have been removed before hitting save on the comment ... [ignore] The pending patch (which is not entirely dissimilar to the patch submitted to this PR) restores the ability to boot with a loader password and prevent user interruption of the boot process without said password. [/ignore]
*** Bug 221289 has been marked as a duplicate of this bug. ***