repl: revise security checks.

Summary: we now check the entire path of .txr_history and .txr_profile
files for security issues; we enforce that these files must not be
readable to other users, not just not writable. And there is a bugfix:
we do not load the history if it has a permission problem, instead of
loading it anyway and just issuing a diagnostic.

* repl.c (report_security_problem): Rename to report_file_perm_problem.
Drop the umask check, because we are going to be checking for files
that are not readable for others, which would require a stricter umask
than the usual 022.
(report_path_perm_problem): New static function.
(load_rcfile): Take the needed function symbols as arguments, because
the only caller is repl and it has them; it can pass them down.
Check the path using path-components-safe function, and bail with
an error message if it is bad.  Then check the file using
path-strictly-private-to-me-p, rather than path-private-to-me-p
as previously. This requires the file not to be readable to others too.
(repl): path_private_to_me_p variable renamed to ppriv_s for brevity
and holds a different symbol: path-strictly-private-to-me-p,
the function which checks that other users cannot read the file, not
just write. Also capture the path-components-safe symbol as
psafe_s. ppriv_s and psafe_s are passed down to load_rcfile so it
can do checks. Like in the case of the rcfile, we now check the
history file using both functions, validating the path not just
the file's own permissions. Bugfix: we now check the history file's
path before loading the history file, and avoid loading it if the
check fails. We use the path-exists-p function now to check that
the history and rc files exist. That leaves a small flaw: an
attacker could be in control of the paths to these files and
manipulate these paths such that these files appear not to exist;
we will then not report on such a situation.

* txr.1: Documented.

2 files changed, 52 insertions, 33 deletions

diff --git a/parser.c b/parser.c
index 1ed3ca78..e8c51f7d 100644
--- a/parser.c
+++ b/parser.c
@@ -939,42 +939,39 @@ val txr_parse(val source_in, val error_stream,
   return pi->syntax_tree;
 }
 
-static void report_security_problem(val name)
+static void report_file_perm_problem(val name)
 {
-  val self = lit("listener");
-  static int umask_warned;
+  format(std_output,
+         lit("** security problem: ~a is readable by others\n"),
+         name, nao);
+}
 
+static void report_path_perm_problem(val name)
+{
   format(std_output,
-         lit("** possible security problem: ~a is writable to others\n"),
+         lit("** security problem: a component of ~a is writable to others\n"),
          name, nao);
-#if HAVE_SYS_STAT
-  if (!umask_warned++)
-  {
-    val um = umask_wrap(colon_k);
-    if ((c_num(um, self) & 022) != 022) {
-      format(std_output,
-             lit("** possible reason: your umask has an insecure value: ~,03o\n"),
-             um, nao);
-    }
-  }
-#endif
 }
 
-static void load_rcfile(val name)
+static void load_rcfile(val name, val psafe_s, val ppriv_s)
 {
   val self = lit("listener");
   val resolved_name = name;
   val lisp_p = t;
   val stream = nil;
-  val path_private_to_me_p =  intern(lit("path-private-to-me-p"), user_package);
+
+  if (!funcall1(psafe_s, name)) {
+    report_path_perm_problem(name);
+    return;
+  }
 
   uw_catch_begin (catch_error, sy, va);
 
   open_txr_file(name, &lisp_p, &resolved_name, &stream, nil, self);
 
   if (stream) {
-    if (!funcall1(path_private_to_me_p, stream)) {
-      report_security_problem(name);
+    if (!funcall1(ppriv_s, stream)) {
+      report_file_perm_problem(name);
     } else {
       val saved_dyn_env = set_dyn_env(make_env(nil, nil, dyn_env));
       env_vbind(dyn_env, load_path_s, resolved_name);
@@ -1563,7 +1560,9 @@ val repl(val bindings, val in_stream, val out_stream, val env)
   val counter_sym = intern(lit("*n"), user_package);
   val var_counter_sym = intern(lit("*v"), user_package);
   val result_hash_sym = intern(lit("*r"), user_package);
-  val path_private_to_me_p =  intern(lit("path-private-to-me-p"), user_package);
+  val pexist_s =  intern(lit("path-exists-p"), user_package);
+  val ppriv_s =  intern(lit("path-strictly-private-to-me-p"), user_package);
+  val psafe_s = intern(lit("path-components-safe"), user_package);
   val result_hash = make_hash(hash_weak_none, nil);
   val done = nil;
   val counter = one;
@@ -1612,17 +1611,19 @@ val repl(val bindings, val in_stream, val out_stream, val env)
   lino_set_enter_cb(ls, is_balanced_line, 0);
   lino_set_tempfile_suffix(ls, ".tl");
 
-  if (rcfile)
-    load_rcfile(rcfile);
+  if (rcfile && funcall1(pexist_s, rcfile))
+    load_rcfile(rcfile, psafe_s, ppriv_s);
 
   lino_hist_set_max_len(ls, c_num(cdr(hist_len_var), self));
 
-  if (histfile_w) {
-    if (lino_hist_load(ls, histfile_w) == 0 &&
-        !funcall1(path_private_to_me_p, histfile))
-    {
-      report_security_problem(histfile);
+  if (histfile_w && funcall1(pexist_s, rcfile)) {
+    if (!funcall1(psafe_s, home)) {
+      report_path_perm_problem(home);
+    } else if (!funcall1(ppriv_s, histfile)) {
+      report_file_perm_problem(histfile);
     }
+
+    lino_hist_load(ls, histfile_w);
   }
 
 #if CONFIG_FULL_REPL
diff --git a/txr.1 b/txr.1
index c1a832e9..475a814d 100644
--- a/txr.1
+++ b/txr.1
@@ -88156,12 +88156,16 @@ environment variable in POSIX environments or the
 environment variable on MS Windows. If that variable doesn't exist, no further
 attempt is made to locate this file.
 
-If the file exists, it is subject to a security check.
-The function
-.code path-private-to-me-p
-is applied to the file. If it returns
-.code nil
-then an error message is displayed and the file is not loaded.
+If the file exists, it is subject to security checks. First, the
+.code path-components-safe
+is applied to its path name. The function validates that no component
+of the path name is a directory that is writable to another user, or
+a symbolic link that could be rewritten by another user.
+If that check passes, the file is then checked with the function
+.code path-strictly-private-to-me-p
+which requires that other users have no read or write permission.
+If the checks fail, then an error message is displayed and the file is not
+loaded.
 
 If the file passes the security check, it is expected to be readable and
 to contain
@@ -88230,6 +88234,20 @@ only adds to the history file new input since the most recent
 .code :save
 command.
 
+When the history file is loaded, security checks take place, in exactly
+the same way that the
+.str .txr_profile
+file is validated. First the path of the history file is checked using
+the function
+.codn path-components-safe ,
+which determines that no component of the path name can be subverted
+by another user, other than the superuser.  If that check passes, then
+the file is checked using
+.code path-strictly-private-to-me-p
+which requires that other users have no read or write permission.
+If the checks fail, then an error message is displayed and the history
+file is not loaded.
+
 .SS* Parenthesis Matching
 
 A feature of the listener is visual parenthesis matching in the form of a