[prev in list] [next in list] [prev in thread] [next in thread] 

List:       elinks-dev
Subject:    Re: [elinks-dev] festival command injection when PIPE_BUF < 1037
From:       Kalle Olavi Niemitalo <kon () iki ! fi>
Date:       2007-03-09 19:50:42
Message-ID: 87lki6i4bx.fsf () Astalo ! kon ! iki ! fi
[Download RAW message or body]

[Attachment #2 (multipart/signed)]

Content-Transfer-Encoding: quoted-printable

Witek, I see you have fixed this bug in commit
1f09c740a8207b38a475ec1375fc859b90b3a625.  Please consider
applying the patch at the end of this message to clarify the
assumptions being made.

I'm still worrying about these problems in the speech code:

- Does "festival -i" enable readline-like functionality that
  could be used to erase the quote character before the parser
  sees it, and thus inject arbitrary commands again?  When ELinks
  reads struct screen_char in order to generate a string that it
  will send to the terminal, it filters out control characters;
  doing the same here might be prudent.  Of course, if festival
  automatically disables such functionality when its stdin is not
  a terminal, then such filtering is not needed, but this should
  be documented.

- If struct screen_char contains UCS-4, write_to_festival uses
  only the low 8 bits.  I think this clashes with the intention
  to make --enable-utf-8 the default in ELinks 0.12.0.  The input
  charsets of the speech synthesizer executables are very badly
  documented but the espeak library apparently can autodetect
  between UTF-8 and the language-specific ISO-8859-*, and for
  festival and flite the appropriate charset depends on the voice
  selected in configuration files.  So ELinks should have a
  configuration option for the speech charset, and recode to
  that.  Because the charset conversion can change the number of
  bytes in the string, the MAX_LINE_LENGTH restriction may have
  to be implemented in a different way.

After those issues have been resolved, I think the speech code
can be merged, and then the following less important problems
should also be fixed or at least added to bugzilla:

- The document.speech.system option is documented as "Default
  speech system".  This is wrong because ELinks never uses a
  non-"default" speech system.

- If the speech synthesizer process dies for any reason, there
  should be a way to make ELinks spawn a new one.  Perhaps by
  detecting EPIPE?

Perhaps there should also be a way to let the user specify the
file name of the speech synthesizer executable.  I don't know
though how to best prevent kiosk users from specifying arbitrary
executables in an anonymous-mode ELinks.


festival: Assert that writes are not short.  Add comments.

---
commit f6a8bcdd75e68f9153c7115edce7f3714f1a7225
tree f27156249ad0674b37bffbf2c889010032908237
parent 1f09c740a8207b38a475ec1375fc859b90b3a625
author Kalle Olavi Niemitalo <kon@iki.fi> Thu, 08 Mar 2007 00:33:50 +0200
committer Kalle Olavi Niemitalo <Kalle@Astalo.kon.iki.fi> Thu, 08 Mar 2007 00:33:50 +0200

 src/viewer/text/festival.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/src/viewer/text/festival.c b/src/viewer/text/festival.c
index d67bba4..a4f83cd 100644
--- a/src/viewer/text/festival.c
+++ b/src/viewer/text/festival.c
@@ -50,10 +50,16 @@ read_from_festival(struct fest *fest)
 		     NULL, NULL, fest);
 }
 
+/* These numbers must match the documentation of the
+ * "document.speech.system" option.  */
 #define FESTIVAL_SYSTEM	0
 #define FLITE_SYSTEM	1
 #define ESPEAK_SYSTEM	2
 
+/* How many character cells to speak from each line.  Because
+ * write_to_festival currently cannot recover from short writes, this
+ * must be set so low that the generated string cannot become more
+ * than PIPE_BUF bytes long.  SUSv2 says PIPE_BUF >= 512.  */
 #define MAX_LINE_LENGTH	240
 
 static void
@@ -86,6 +92,8 @@ write_to_festival(struct fest *fest)
 
 	data = doc->data[fest->line].chars;
 	if (festival.speech_system == FESTIVAL_SYSTEM) {
+		/* The string generated here will be at most
+		 * 10+MAX_LINE_LENGTH*2+2+1 bytes long.  */ 
 		add_to_string(&buf, "(SayText \"");
 		/* UTF-8 not supported yet. Does festival support UTF-8? */
 		for (i = 0; i < len; i++) {
@@ -105,8 +113,29 @@ write_to_festival(struct fest *fest)
 	}
 	add_char_to_string(&buf, '\n');
 
+	/* Cannot allow a short write here.  It would probably cause
+	 * an unpaired quote character to be sent to festival,
+	 * enabling a command injection attack.  If buf.length <=
+	 * PIPE_BUF, then the write will occur either completely or
+	 * not at all.  Compare to _POSIX_PIPE_BUF instead, to ensure
+	 * portability to systems with an unusually low PIPE_BUF.
+	 * If this assertion fails, that means MAX_LINE_LENGTH was
+	 * set too high.  */ 
+	assert(buf.length <= _POSIX_PIPE_BUF);
+	if_assert_failed { buf.length = 0; }
 	w = safe_write(fest->out, buf.source, buf.length);
 	if (w >= 0) {
+		assert(w == buf.length);
+		if_assert_failed {
+			/* The input parser of the festival process is
+			 * now in an unknown state.  */
+			clear_handlers(fest->in);
+			close(fest->in);
+			fest->in = -1;
+			clear_handlers(fest->out);
+			close(fest->out);
+			fest->out = -1;
+		}
 		if (fest->line >= doc_view->vs->y + doc_view->box.height) {
 			move_page_down(doc_view->session, doc_view);
 			refresh_view(doc_view->session, doc_view, 0);

[Attachment #5 (application/pgp-signature)]

_______________________________________________
elinks-dev mailing list
elinks-dev@linuxfromscratch.org
http://linuxfromscratch.org/mailman/listinfo/elinks-dev


[prev in list] [next in list] [prev in thread] [next in thread]