XPost: linux.debian.security
From: [email protected]

On Tue, Aug 10, 2004 at 10:12:06AM -0700, Josh Martin wrote:

-- no debconf information
Although this should never actually happen, if you set your environment variable HOME to an extremely large string a buffer overflow will occur upon connecting to a server using telnet.

Urgh.. This really calls for an upload to t-p-u.

I was not able to overwrite 'eip'
but I have included a patch that fixes this problem.

Could you overwrite esp/ebp? Anyway, I'm CCing the security team for assistance on the impact. I don't think it's release-critical since a
tainted HOME already implies there's a flaw somewhere.

--- commands.orig.cc 2004-08-10 09:50:44.000000000 -0700
+++ commands.cc 2004-08-10 09:51:07.000000000 -0700
@@ -2148,7 +2148,7 @@
if (rcname == 0) {
rcname = getenv("HOME");
if (rcname)
- strcpy(rcbuf, rcname);
+ strncpy(rcbuf, rcname, 127);
else
rcbuf[0] = '\0';
strcat(rcbuf, "/.telnetrc");

I don't like it. This keeps the 127-byte hardcoded limit. What would you think about:

--- netkit-telnet-0.17/telnet/commands.cc~ 2004-05-19 01:56:10.000000000 +0200
+++ netkit-telnet-0.17/telnet/commands.cc 2004-08-11 21:32:02.000000000 +0200
@@ -2139,22 +2139,14 @@
}

void cmdrc(const char *m1, const char *m2, const char *port) {
- static char *rcname = 0;
- static char rcbuf[128];
+ static char *rcname;

if (skiprc) return;

readrc(m1, m2, port, "/etc/telnetrc");
- if (rcname == 0) {
- rcname = getenv("HOME");
- if (rcname)
- strcpy(rcbuf, rcname);
- else
- rcbuf[0] = '\0';
- strcat(rcbuf, "/.telnetrc");
- rcname = rcbuf;
- }
+ asprintf (&rcname, "%s/.telnetrc", getenv ("HOME"));
readrc(m1, m2, port, rcname);
+ free (rcname);
}

#if defined(IP_OPTIONS) && defined(HAS_IPPROTO_IP)


Let me know if I screwed on something, we need to be extra careful with standard packages during the freeze..

--
Robert Millan

(Debra and Ian) (Gnu's Not (UNiplexed Information and Computing System))/\ (kernel of *(Berkeley Softw

XPost: linux.debian.security
From: [email protected]

* Josh Martin <[email protected]> [040810 10:08]:

Although this should never actually happen, if you set your environment variable HOME to an extremely large string a buffer overflow will occur upon connecting to a server using telnet. I was not able to overwrite 'eip'
but I have included a patch that fixes this problem.

[some context for the patch]
void cmdrc(const char *m1, const char *m2, const char *port) {
static char *rcname = 0;
static char rcbuf[128];

if (skiprc) return;

readrc(m1, m2, port, "/etc/telnetrc");

--- commands.orig.cc 2004-08-10 09:50:44.000000000 -0700
+++ commands.cc 2004-08-10 09:51:07.000000000 -0700
@@ -2148,7 +2148,7 @@
if (rcname == 0) {
rcname = getenv("HOME");
if (rcname)
- strcpy(rcbuf, rcname);
+ strncpy(rcbuf, rcname, 127);
else
rcbuf[0] = '\0';
strcat(rcbuf, "/.telnetrc");

I may be utterly confused, but that patch does look quite strange.
It may make it near to impossible to introduce code, but only reduces
the problem: strncpy will not '\0'-terminate the string, so that the
following "/.telnetrc" will be written to some random position.
and even if there was some termination, 127 chars plus 10 chars
for "/.telnetrc" is still more than the reserved 128. (thus when
having $HOME 116 to 126 chars one could even control where the
/.telnetrc letters get to).


Hochachtungsvoll,
Bernhard R. Link


--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]

--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)

From: [email protected]

On Wed, Aug 11, 2004 at 02:09:37PM -0700, Wade Richards wrote:

Hi,

I hope you don't mind me nit-picking...

Feedback is appreciated, of course.

On Wed, Aug 11, 2004 at 09:38:32PM +0200, Robert Millan wrote:

--- netkit-telnet-0.17/telnet/commands.cc~ 2004-05-19 01:56:10.000000000 +0200
+++ netkit-telnet-0.17/telnet/commands.cc 2004-08-11 21:32:02.000000000 +0200
@@ -2139,22 +2139,14 @@
}

void cmdrc(const char *m1, const char *m2, const char *port) {
- static char *rcname = 0;
- static char rcbuf[128];
+ static char *rcname;

Why still static? Without knowing the rest of the program flow, I need
to worry about whether or not this is a multi-threaded bug. And with
the new logic, there is no advantage to making it static.

The program is not multi-threaded, but making it static is pointless anyway.

Also, I'd keep the initialization to NULL. It helps to start with a
known state.

Yep.

if (skiprc) return;

readrc(m1, m2, port, "/etc/telnetrc");
- if (rcname == 0) {
- rcname = getenv("HOME");
- if (rcname)
- strcpy(rcbuf, rcname);
- else
- rcbuf[0] = '\0';
- strcat(rcbuf, "/.telnetrc");
- rcname = rcbuf;
- }
+ asprintf (&rcname, "%s/.telnetrc", getenv ("HOME"));

What if asprintf fails? I think it returns 0, but you are ignoring the return value. So if you "HOME" is large enough, the malloc will fail,

You mean, like, ENOMEM? Ok, I'll add something.

Anything else?

--
Robert Millan

(Debra and Ian) (Gnu's Not (UNiplexed Information and Computing System))/\ (kernel of *(Berkeley Software Distribution))


--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]

--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)

XPost: linux.debian.security
From: [email protected]

On Thu, Aug 12, 2004 at 10:04:52AM +0200, Bernhard R. Link wrote:

I may be utterly confused, but that patch does look quite strange.
It may make it near to impossible to introduce code, but only reduces
the problem: strncpy will not '\0'-terminate the string, so that the following "/.telnetrc" will be written to some random position.
and even if there was some termination, 127 chars plus 10 chars
for "/.telnetrc" is still more than the reserved 128. (thus when
having $HOME 116 to 126 chars one could even control where the
/.telnetrc letters get to).

That patch is wrong. Please direct your comments at the patch for dynamic allocation I just sent instead.

--
Robert Millan

(Debra and Ian) (Gnu's Not (UNiplexed Information and Computing System))/\ (kernel of *(Berkeley Software Distribution))


--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]

--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)

From: [email protected]

On Thu, Aug 12, 2004 at 11:14:37AM +0200, Robert Millan wrote:

+ asprintf (&rcname, "%s/.telnetrc", getenv ("HOME"));

What if asprintf fails? I think it returns 0, but you are ignoring the return value. So if you "HOME" is large enough, the malloc will fail,

You mean, like, ENOMEM? Ok, I'll add something.

Actually, I really should have read "man 3 asprintf" before replying.
If allocation fails, it will return -1, and the pointer is undefined.
So, if asprintf returns -1, you cannot trust the pointer.

--- Wade

--
/"\ . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
\ / ASCII Ribbon Campaign | Wade Richards --- [email protected]
X - NO HTML/RTF in e-mail | Hagh qoHpu' neH QaghDI' ghunwI''a'pu'
/ \ - NO Word docs in e-mail |


--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]

--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)