Bug 1066

Summary:

off-by-one error with GSSAPI names

Product:

Portable OpenSSH

Reporter:

David Leonard <David.Leonard>

Component:

ssh

Assignee:

Assigned to nobody <unassigned-bugs>

Status:

CLOSED FIXED

Severity:

normal

Priority:

Version:

4.1p1

Hardware:

All

OS:

All

Attachments:

Description	Flags
quick fix	none
Comitted patch	none

Description David Leonard 2005-08-08 23:27:27 AEST

ssh_gssapi_import_name() passes a string through a GSSAPI buffer that is one
byte too long. This seems to occasionally cause problems, like spurious garbage
characters appearing at the end of realms.

Comment 1 David Leonard 2005-08-08 23:33:02 AEST

Created attachment 945 [details]
quick fix

Simple fix that decrements the buffer length after the snprintf.
A real fix would not be so hacky. :)

Comment 2 Damien Miller 2005-08-27 01:16:45 AEST

Are you saying that gssbuf.value shouldn't be \0 terminated?

Comment 3 David Leonard 2005-08-29 14:51:43 AEST

(In reply to comment #2)
> Are you saying that gssbuf.value shouldn't be \0 terminated?

yes. It's not explicit, but RFC 1509 s2.1.3.2 means gss buffers use a length,
not a sentinel.

Comment 4 Simon Wilkinson 2005-08-30 19:24:53 AEST

Yes - all GSSAPI buffers use an explicit length, rather than using NULL as a
marker. This is a bug which should be fixed for correctness's sake.

David's fix is probably as good as any (and leaves the code more readable than
using a series of memcpy's to avoid the NULL in the first place). Perhaps the
modification to gssbuf.length should have a comment - to explain that its
stripping the trailing NULL.

However, in the case of import_name, all of the GSSAPI implementations I have
source for then take the buffer, malloc a string 1 character longer, and stick a
NULL back on the end. I can't see anyway in which already having the string NULL
terminated would cause the problems that David's describing.

Comment 5 David Leonard 2005-08-30 20:58:49 AEST

(In reply to comment #4)
> However, in the case of import_name, all of the GSSAPI implementations I have
> source for then take the buffer, malloc a string 1 character longer, and stick a
> NULL back on the end. I can't see anyway in which already having the string NULL
> terminated would cause the problems that David's describing.

You are right; the symptom I am seeing actually came back after this fix, and I
am still trying to track it down. (It was an extra '0' or '@' after the realm
name, but realm referrals are happening.. it is possibly somewhere beneath the
gssapi interface.)

I am using a gss impl derived from heimdal, and you're right; it does exactly
the malloc you spoke of. so my analysis was wrong. this is likely a benign bug.

Comment 6 Damien Miller 2006-04-03 17:08:36 AEST

Created attachment 1109 [details]
Comitted patch

might as well close this bug and simplify this code using xasprintf() now that we have it

Comment 7 Damien Miller 2006-04-03 17:11:15 AEST

patch applied, thanks for the report.

Comment 8 Darren Tucker 2006-10-07 11:41:26 AEST

Change all RESOLVED bug to CLOSED with the exception of the ones fixed post-4.4.