Re: "Local variables" denial-of-service attack

Re: "Local variables" denial-of-service attack

[Stefan Monnier]

>    Local Variab*les:
>    byte-compile-warnings: #1=("circular" "object" . #1#)
>    End:

> Visiting such a file will result in Emacs hanging and consuming all
> CPU time. (Normally, aborting with C-g is possible. However, if the
> file is opened with emacsclient there seems to be no way to recover.)

Hmm... that's not good.  It's not the end of the world since it's "only"
a DoS, but we should try and fix it.  I see two ways:
1 - don't allow (in file-local vars and .dir-local.el) the # escapes
    that permit creation of those circular objects.
2 - be more careful about circularities in the
    safe-local-variable predicates.

Nb 1 is easier and will solve it "for all cases", so it's probably
a better choice.  Of course, it also "denies service" to those rare
individuals who used circular data in file-local variables for
non-DoS purposes.  I don't know if there are any on this planet, tho.


        Stefan

Re: "Local variables" denial-of-service attack

[Ulrich Mueller]

>>>>> On Fri, 09 Jan 2009, Stefan Monnier wrote:

>> Local Variab*les:
>> byte-compile-warnings: #1=("circular" "object" . #1#)
>> End:

> Hmm... that's not good.  It's not the end of the world since it's "only"
> a DoS, but we should try and fix it.  I see two ways:
> 1 - don't allow (in file-local vars and .dir-local.el) the # escapes
>     that permit creation of those circular objects.
> 2 - be more careful about circularities in the
>     safe-local-variable predicates.

> Nb 1 is easier and will solve it "for all cases", so it's probably
> a better choice.

But for 1 you would have to communicate to "read1" that it's reading
local variables? It's not obvious to me how one would do this in a
clean way. (I'm not very familiar with that code though).

Whereas for 2, adding a simple check like (nthcdr (safe-length foo) foo)
being nil should be sufficient in most (all?) cases.

Ulrich

Re: "Local variables" denial-of-service attack

[Stefan Monnier]

> But for 1 you would have to communicate to "read1" that it's reading
> local variables? It's not obvious to me how one would do this in a
> clean way. (I'm not very familiar with that code though).

I'd introduce a `read-circle' variable (to correspond to `print-circle'
used when printing), which I'd then let-bind around the call to `read'
used when processing file local variables.


        Stefan

Re: "Local variables" denial-of-service attack

[Ulrich Mueller]

>>>>> On Fri, 09 Jan 2009, Stefan Monnier wrote:

>> Local Variab*les:
>> byte-compile-warnings: #1=("circular" "object" . #1#)
>> End:

>> Visiting such a file will result in Emacs hanging and consuming all
>> CPU time. (Normally, aborting with C-g is possible. However, if the
>> file is opened with emacsclient there seems to be no way to recover.)

> Hmm... that's not good.  It's not the end of the world since it's "only"
> a DoS, but we should try and fix it.  I see two ways:
> 1 - don't allow (in file-local vars and .dir-local.el) the # escapes
>     that permit creation of those circular objects.
> 2 - be more careful about circularities in the
>     safe-local-variable predicates.

> Nb 1 is easier and will solve it "for all cases", so it's probably
> a better choice.

Coming back to this, the following patch works for me:

etc/NEWS entry:

** Reading of circular structures in file-local and directory-local
variable lists has been disabled.

lisp/ChangeLog entry:

2009-04-02  Ulrich Mueller  <ulm@gentoo.org>

	* files.el (hack-local-variables-prop-line)
	(hack-local-variables, dir-locals-read-from-file):
	Bind read-circle to nil, in order to disable reading of recursive
	structures in file-local and dir-local variables.

src/ChangeLog entry:

2009-04-02  Ulrich Mueller  <ulm@gentoo.org>

	* lread.c (read_circle): New variable.
	(syms_of_lread): DEFVAR_BOOL read-circle, initialize to t.
	(read1): Disable #N= and #N# syntax for recursive structures if
	read-circle is nil.


--- emacs/lisp/files.el~
+++ emacs/lisp/files.el
@@ -2896,10 +2896,11 @@
 	       ;; There used to be a downcase here,
 	       ;; but the manual didn't say so,
 	       ;; and people want to set var names that aren't all lc.
-	       (let ((key (intern (match-string 1)))
-		     (val (save-restriction
-			    (narrow-to-region (point) end)
-			    (read (current-buffer)))))
+	       (let* ((read-circle nil)
+		      (key (intern (match-string 1)))
+		      (val (save-restriction
+			     (narrow-to-region (point) end)
+			     (read (current-buffer)))))
 		 ;; It is traditional to ignore
 		 ;; case when checking for `mode' in set-auto-mode,
 		 ;; so we must do that here as well.
@@ -3044,7 +3045,9 @@
 		  (skip-chars-forward "^:\n")
 		  (if (eolp) (error "Missing colon in local variables entry"))
 		  (skip-chars-backward " \t")
-		  (let* ((str (buffer-substring beg (point)))
+		  ;; Bind `read-circle' to nil to disable recursive structures.
+		  (let* ((read-circle nil)
+			 (str (buffer-substring beg (point)))
 			 (var (read str))
 			 val)
 		    ;; Read the variable value.
@@ -3323,7 +3326,8 @@
     ;; We should probably store the modtime of FILE and then
     ;; reload it whenever it changes.
     (insert-file-contents file)
-    (let* ((dir-name (file-name-directory file))
+    (let* ((read-circle nil)
+	   (dir-name (file-name-directory file))
 	   (class-name (intern dir-name))
 	   (variables (read (current-buffer))))
       (dir-locals-set-class-variables class-name variables)
--- emacs/src/lread.c~
+++ emacs/src/lread.c
@@ -156,6 +156,9 @@
 /* List of (SYMBOL . POSITION) accumulated so far. */
 Lisp_Object Vread_symbol_positions_list;
 
+/* Nonzero means allow #N= and #N# syntax for recursive structures. */
+static int read_circle;
+
 /* List of descriptors now open for Fload.  */
 static Lisp_Object load_descriptor_list;
 
@@ -2558,7 +2561,7 @@
 	      c = READCHAR;
 	    }
 	  /* #n=object returns object, but associates it with n for #n#.  */
-	  if (c == '=')
+	  if (c == '=' && read_circle)
 	    {
 	      /* Make a placeholder for #n# to use temporarily */
 	      Lisp_Object placeholder;
@@ -2580,7 +2583,7 @@
 	      return tem;
 	    }
 	  /* #n# returns a previously read object.  */
-	  if (c == '#')
+	  if (c == '#' && read_circle)
 	    {
 	      tem = Fassq (make_number (n), read_objects);
 	      if (CONSP (tem))
@@ -4215,6 +4218,12 @@
 were read in. */);
   Vread_symbol_positions_list = Qnil;
 
+  DEFVAR_BOOL ("read-circle", &read_circle,
+	       doc: /* Non-nil means allow #N= and #N# syntax for recursive structures.
+This is normally bound by functions like `hack-local-variables' to
+disable this syntax in `read', and is not meant for users to change. */);
+  read_circle = 1;
+
   DEFVAR_LISP ("load-path", &Vload_path,
 	       doc: /* *List of directories to search for files to load.
 Each element is a string (directory name) or nil (try default directory).

Re: "Local variables" denial-of-service attack

[Ulrich Mueller]

>>>>> On Thu, 2 Apr 2009, Ulrich Mueller wrote:

>>> Local Variab*les:
>>> byte-compile-warnings: #1=("circular" "object" . #1#)
>>> End:

>>> Visiting such a file will result in Emacs hanging and consuming all
>>> CPU time.

Hi again, I had sent a patch for this, but there was no answer at all.
What is the status of this issue? Shouldn't it be fixed for 23.1?

Ulrich

Re: "Local variables" denial-of-service attack

[Chong Yidong]

Ulrich Mueller <ulm@gentoo.org> writes:

>>>>>> On Thu, 2 Apr 2009, Ulrich Mueller wrote:
>
>>>> Local Variab*les:
>>>> byte-compile-warnings: #1=("circular" "object" . #1#)
>>>> End:
>
>>>> Visiting such a file will result in Emacs hanging and consuming all
>>>> CPU time.
>
> Hi again, I had sent a patch for this, but there was no answer at all.
> What is the status of this issue? Shouldn't it be fixed for 23.1?

Could someone review this patch?  I am not extremely familiar with this
part of the code.  Stefan, what's your opinion?

Re: "Local variables" denial-of-service attack

[Chong Yidong]

Ulrich Mueller <ulm@gentoo.org> writes:

> Hi again, I had sent a patch for this, but there was no answer at all.
> What is the status of this issue? Shouldn't it be fixed for 23.1?

I've reviewed your patch and committed it, with minor alterations.