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.
Created attachment 945 [details] quick fix Simple fix that decrements the buffer length after the snprintf. A real fix would not be so hacky. :)
Are you saying that gssbuf.value shouldn't be \0 terminated?
(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.
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.
(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.
Created attachment 1109 [details] Comitted patch might as well close this bug and simplify this code using xasprintf() now that we have it
patch applied, thanks for the report.
Change all RESOLVED bug to CLOSED with the exception of the ones fixed post-4.4.