client: Fix rogue execute protection

To protect against a rogue server reading your files, the client
honors C_EXECUTE only when it matches recent player input.

This has a somewhat troubled history, detailed in the previous commit.

The remaining major issue comes from commit 8b7d0b9 (v4.3.11): any
suffix of a recent line of input is accepted as C_EXECUTE text.
Before, only text that looked like an argument of an execute command
or a redirection was accepted.

Fix by again requiring the text to be preceded by something that looks
like an execute command.  But do it more carefully: don't break
execute with a prompted for argument, and prevent abuse of
redirections for execute.

Signed-off-by: Markus Armbruster <armbru@pond.sub.org>

diff --git a/src/client/ringbuf.c b/src/client/ringbuf.c
index 1fb6bff6fa0aa984b67ed226afd045d1126c2f03..82e772dc30ca15aef2fef628ef33aa50c17767a4 100644 (file)
--- a/src/client/ringbuf.c
+++ b/src/client/ringbuf.c
@@ -157,16 +157,17 @@ ring_discard(struct ring *r, int n)
 
 /*
  * Search the ring buffer for zero-terminated string S.
- * If found, return a non-negative value referring to the beginning of
- * S in the buffer when passed to ring_peek().  Else return -1.
+ * Start at the @(n+1)-th byte to be gotten.
+ * If found, return the number of bytes in the buffer before S.
+ * Else return -1.
  */
 int
-ring_search(struct ring *r, char *s)
+ring_search(struct ring *r, char *s, int n)
 {
     size_t len = strlen(s);
     size_t i, j;
 
-    for (i = r->cons; i + len <= r->prod; i++) {
+    for (i = r->cons + n; i + len <= r->prod; i++) {
        for (j = 0; s[j] && s[j] == (char)r->buf[(i + j) % RING_SIZE]; j++)
            ;
        if (!s[j])

diff --git a/src/client/ringbuf.h b/src/client/ringbuf.h
index d261635661f338e5d9064354513815f1d3601835..4d44a2b939cdc647f3f99fb84f46931211b6b40c 100644 (file)
--- a/src/client/ringbuf.h
+++ b/src/client/ringbuf.h
@@ -27,7 +27,7 @@
  *  ringbuf.h: Simple ring buffer
  *
  *  Known contributors to this file:
- *     Markus Armbruster, 2007
+ *     Markus Armbruster, 2007-2015
  */
 
 #ifndef RINGBUF_H
@@ -59,7 +59,7 @@ extern int ring_getc(struct ring *);
 extern int ring_putc(struct ring *, unsigned char);
 extern int ring_putm(struct ring *, void *, size_t);
 extern void ring_discard(struct ring *, int);
-extern int ring_search(struct ring *, char *);
+extern int ring_search(struct ring *, char *, int);
 extern int ring_from_file(struct ring *, int fd);
 extern int ring_to_file(struct ring *, int fd);
 

diff --git a/src/client/secure.c b/src/client/secure.c
index 401eaefd74261f94eaed5d3eb15a55f51132f187..330260f7f2d7fa2fce6a0841f6f74615a8e087a2 100644 (file)
--- a/src/client/secure.c
+++ b/src/client/secure.c
@@ -33,6 +33,8 @@
 #include <config.h>
 
 #include <assert.h>
+#include <ctype.h>
+#include <stdio.h>
 #include <string.h>
 #include "ringbuf.h"
 #include "secure.h"
@@ -52,7 +54,7 @@ save_input(char *inp)
     assert(len && inp[len - 1] == '\n');
 
     while (ring_putm(&recent_input, inp, len) < 0) {
-       eol = ring_search(&recent_input, "\n");
+       eol = ring_search(&recent_input, "\n", 0);
        assert(eol >= 0);
        ring_discard(&recent_input, eol + 1);
     }
@@ -68,5 +70,42 @@ seen_input(char *tail)
     size_t len = strlen(tail);
 
     assert(len && tail[len - 1] == '\n');
-    return ring_search(&recent_input, tail) >= 0;
+    return ring_search(&recent_input, tail, 0) >= 0;
+}
+
+/*
+ * Can you still remember input that looks like an execute @arg?
+ * @arg must end with a newline.
+ */
+int
+seen_exec_input(char *arg)
+{
+    size_t len = strlen(arg);
+    int n, i, j, ch;
+    unsigned char buf[RING_SIZE + 1];
+
+    assert(len && arg[len - 1] == '\n');
+
+    n = 1;
+    for (;;) {
+       /* find next line ending with arg */
+       n = ring_search(&recent_input, arg, n + 1);
+       if (n <= 0)
+           return 0;
+
+       /* extract command (same or previous line) */
+       i = n - 1;
+       if (ring_peek(&recent_input, i) == '\n')
+           i--;
+       j = sizeof(buf);
+       buf[--j] = 0;
+       for (; i >= 0 && (ch = ring_peek(&recent_input, i)) != '\n'; i--)
+           buf[--j] = ch;
+
+       /* compare command */
+       for (; isspace(buf[j]); j++) ;
+       for (i = j; buf[i] && !isspace(buf[i]); i++) ;
+       if (i - j >= 2 && !strncmp("execute", (char *)buf + j, i - j))
+           return 1;
+    }
 }

diff --git a/src/client/secure.h b/src/client/secure.h
index 301a684667cbe1cc1433ca8c5f2240d931e0a1ff..8e4454f9636eee10c59e53057c454dec6d9cb40b 100644 (file)
--- a/src/client/secure.h
+++ b/src/client/secure.h
@@ -37,5 +37,6 @@
 
 extern void save_input(char *);
 extern int seen_input(char *);
+extern int seen_exec_input(char *);
 
 #endif

diff --git a/src/client/servcmd.c b/src/client/servcmd.c
index 65d029c7b06a868aaa6740dc2184cc2071bff76a..1bb05cbbe84eb14f85c4366841bd3f753813b3d5 100644 (file)
--- a/src/client/servcmd.c
+++ b/src/client/servcmd.c
@@ -154,9 +154,24 @@ fname(char *s)
 }
 
 static int
-redir_authorized(char *arg, char *attempt, int expected)
+common_authorized(char *arg, char *attempt)
 {
-    if (!expected) {
+    if (restricted) {
+       fprintf(stderr, "Can't %s in restricted mode\n", attempt);
+       return 0;
+    }
+
+    if (executing) {
+       fprintf(stderr, "Can't %s in a batch file\n", attempt);
+       return 0;
+    }
+    return 1;
+}
+
+static int
+redir_authorized(char *arg, char *attempt)
+{
+    if (redir_fp) {
        fprintf(stderr, "Warning: dropped conflicting %s %s",
                attempt, arg);
        return 0;
@@ -168,16 +183,19 @@ redir_authorized(char *arg, char *attempt, int expected)
        return 0;
     }
 
-    if (restricted) {
-       fprintf(stderr, "Can't %s in restricted mode\n", attempt);
-       return 0;
-    }
+    return common_authorized(arg, attempt);
+}
 
-    if (executing) {
-       fprintf(stderr, "Can't %s in a batch file\n", attempt);
+static int
+exec_authorized(char *arg)
+{
+    if (!seen_exec_input(arg)) {
+       fprintf(stderr,
+               "Warning: server attempted to execute batch file %s", arg);
        return 0;
     }
-    return 1;
+
+    return common_authorized(arg, "execute batch file");
 }
 
 static void
@@ -186,7 +204,7 @@ doredir(char *p)
     int mode;
     int fd;
 
-    if (!redir_authorized(p, "redirect to file", !redir_fp))
+    if (!redir_authorized(p, "redirect to file"))
        return;
     if (*p++ != '>') {
        fprintf(stderr, "Warning: dropped weird redirection %s", p);
@@ -221,7 +239,7 @@ doredir(char *p)
 static void
 dopipe(char *p)
 {
-    if (!redir_authorized(p, "pipe to shell command", !redir_fp))
+    if (!redir_authorized(p, "pipe to shell command"))
        return;
     if (*p++ != '|') {
        fprintf(stderr, "Warning: dropped weird pipe %s", p);
@@ -250,7 +268,7 @@ doexecute(char *p)
 {
     int fd;
 
-    if (!redir_authorized(p, "execute batch file", 1))
+    if (!exec_authorized(p))
        return -1;
 
     p = fname(p);