linux-kernel - Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130903211253.GA22253@gnuservers.com.ar>
Date:	Tue, 3 Sep 2013 23:12:53 +0200
From:	Maximiliano Curia <maxy@...servers.com.ar>
To:	Peter Hurley <peter@...leysoftware.com>
Cc:	Margarita Manterola <margamanterola@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>,
	Linux kernel <linux-kernel@...r.kernel.org>
Subject: Re: Large pastes into readline enabled programs causes breakage from
 v2.6.31 onwards

¡Hola Peter!

El 2013-08-19 a las 08:25 -0400, Peter Hurley escribió:
> My primary concern is canonical readers not become stuck with a full
> read buffer, even with bogus input data (IOW, that an error condition will
> not prevent a reader from making forward progress). I believe that won't
> happen with this change, but what I really need in this case is a detailed
> analysis from you of why that won't happen. That analysis should be in
> the patch changelog. (Feel free to send me private mail if you need help
> preparing a patch.)

I'm not sure what level of analysis you are looking for. The driver will block
when there are no readers but as soon as there is a read call it unblocks.
I've added this information to the patch description that I'm including below.

> And the patch above has a bug that allows a negative 'left' to be
> assigned to tty->receive_room which will be interpreted as a very large
> positive value.

Ok, fixed with an else clause. It could also use an extra &&, but it looks a
bit confusing.

> This approach still has several drawbacks.

> 1) Since additional state is reset when the termios is changed by
> readline(), the canonical line buffer state will be bogus.
> This renders the termios change by readline() pointless; the
> caller will not be able to retrieve expected input properly.

> 2) Since the input data is interpreted with the current termios when
> data is received, any embedded control characters will not be
> interpreted properly; again, the caller will not be able to retrieve
> expected input properly.

Indeed this is correct, however this is not an issue of this patch but of the
current interaction between the kernel and readline. In order to fix this, the
reading buffer should always be in raw and only when responding to a read call
for canonical mode should it be interpreted. This is a very big change, and
I'm not sure if anybody will be interested in implementing it.

> >What do you think? Is the proposed solution, or something along those lines,
> >acceptable?

> I'm wondering if this problem might be best addressed on the paste side
> instead of the read side. Although this wouldn't be a magic bullet, it
> would be easier to control when more paste data is added.

I don't see how this could work, could you elaborate?

This is the patch proposal, for comments:

From 81afd3b666cbf94bb9923ebf87fb2017a7cd645e Mon Sep 17 00:00:00 2001
From: Maximiliano Curia <maxy@...servers.com.ar>
Date: Tue, 3 Sep 2013 22:48:34 +0200
Subject: [PATCH] Only let characters through when there are active readers.

If there is an active reader, previous behavior is in place. When there is no
active reader, input is blocked until the next read call unblocks it.

This fixes a long standing issue with readline when pasting more than 4096
bytes.
---
 drivers/tty/n_tty.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 4bf0fc0..cdc3b19 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -147,9 +147,16 @@ static int set_room(struct tty_struct *tty)
 	 * pending newlines, let characters through without limit, so
 	 * that erase characters will be handled.  Other excess
 	 * characters will be beeped.
+	 * If there is no reader waiting for the input, block instead of
+	 * letting the characters through.
 	 */
 	if (left <= 0)
-		left = ldata->icanon && !ldata->canon_data;
+		if (waitqueue_active(&tty->read_wait)) {
+			left = ldata->icanon && !ldata->canon_data;
+		} else {
+			left = 0;
+		}
+
 	old_left = tty->receive_room;
 	tty->receive_room = left;
 
-- 
1.8.4.rc3


-- 
"Always code as if the person who ends up maintaining your code is a violent
psychopath who knows where you live."
-- John Woods
Saludos /\/\ /\ >< `/

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)