Re: PR pkg/60563: exposed sshd w/o password-login causes sshd-auth looping

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

This is essentially a duplicate of PR bin/45200: Scp hangs after
sending (https://gnats.NetBSD.org/45200), which we worked around in
the base openssh by using a different function which happens not to
have an ssp wrapper, which is not great.  But we have since fixed the
underlying problem in NetBSD>=11, though unfortunately a pullup
wouldn't help for packages built against 10.0 as we do.

> Date: Sat, 03 Oct 2026 20:51:36 +0200 (CEST)
> From: Havard Eidnes <he%uninett.no@localhost>
> 
> Now... __SSP_FORTIFY_LEVEL may play into this.  Not sure what it
> is by default, but possibly and probably not 0, so read() doesn't
> get defined via this entry in <unistd.h>:
> [...]
> but ... is that messing things up for the openssh code in this
> case?
> 
> There is also the issue that the address taken may not point to
> the "real" function but to an entry in the procedure linkage
> table(?)  But why is the value different as seen from the two
> different functions?  I beleive I've not understood why the
> pattern used by openssh fails.

The procedure linkage table is not relevant: the toolchain ensures
that even when there's PLT indirections involved, the address you get
for a library symbol will be the same for all objects in the process,
which is constraint that is annoying for some optimization purposes
(e.g., see the comments about it on pp. 19-20 of
https://web.archive.org/web/20250328091912/https://akkadia.org/drepper/dsohowto.pdf).

But you are right that fortify/ssp plays a role -- specifically,
defining _FORTIFY_SOURCE in the build, as pkgsrc does with
PKGSRC_USE_FORTIFY, which is on by default.

>From NetBSD 6 through NetBSD 10, we defined the fortify/ssp wrappers
roughly like so:

/* make C function `__ssp_real_read' use ELF symbol `read' */
ssize_t __ssp_real_read(int, void *, size_t) __asm("read");

/* make C function `read' do ssp checks before calling ELF symbol */
static inline __attribute__((__always_inline__, __gnu_inline_))
ssize_t read(int fd, void *buf, size_t len)
{
	if (__builtin_object_size(buf, 0) != (size_t)-1 &&
	    len > __builtin_object_size(__buf, 0))
		__chk_fail();
	return __ssp_real_read(fd, buf, len);
}

Unfortunately, this gets copied in each .c file, so if you take the
address of the `read' function, there is a _different definition_ in
each .o file.  That leads to the problem you saw.


What changed in NetBSD 11 is that we switched from `static inline' to
`extern inline':

	If you specify both inline and extern in the function
	definition, then the definition is used only for inlining.  In
	no case is the function compiled on its own, not even if you
	refer to its address explicitly.  Such an address becomes an
	external reference, as if you had only declared the function,
	and had not defined it.

	This combination of inline and extern has almost the effect of
	a macro.  The way to use it is to put a function definition in
	a header file with these keywords, and put another copy of the
	definition (lacking inline and extern) in a library file.  The
	definition in the header file causes most calls to the
	function to be inlined.  If any uses of the function remain,
	they refer to the single copy in the library.

https://gcc.gnu.org/onlinedocs/gcc-10.5.0/gcc/Inline.html

The ssp wrapper macros also, by the way, introduce _another_ inline
declaration of `read' with __asm(__ssp_protected_read), causing the
_inline definition_ to have the ELF symbol `__ssp_protected_read' if
it is ever materialized.  And, when you take the address of a _static_
inline function, it _is_ materialized as a separate function, distinct
from the library function.  So if you objdump -dlr atomicio.o on
NetBSD<=10, you'll see:

0000000000000000 <__ssp_protected_read>:
read():
/usr/include/ssp/unistd.h:39
   0:   e9 00 00 00 00          jmpq   5 <atomiciov6.part.0>
                        1: R_X86_64_PLT32       read-0x4
...
000000000000022d <atomicio6>:
...
/home/riastradh/pkgsrc/current/work/security/openssh/work/openssh-10.5p1/atomicio.c:55
 25c:   48 8d 05 9d fd ff ff    lea    -0x263(%rip),%rax        # 0 <__ssp_protected_read>
 263:   49 39 c5                cmp    %rax,%r13

And you'll find the same _content_ of __ssp_protected_read with a
different identity (so it will have a different address) in kex.o (and
different alignment in objdump output because kex.o has four digits of
instruction offsets vs atomicio.o's three):

0000000000000000 <__ssp_protected_read>:
read():
/usr/include/ssp/unistd.h:39
       0:       e9 00 00 00 00          jmpq   5 <kex_protocol_error>
                        1: R_X86_64_PLT32       read-0x4
...
000000000000309b <kex_exchange_identification>:
...
    3235:       48 8d 3d c4 cd ff ff    lea    -0x323c(%rip),%rdi        # 0 <__ssp_protected_read>
    323c:       e8 00 00 00 00          callq  3241 <kex_exchange_identification+0x1a6>                 
                        323d: R_X86_64_PLT32    atomicio-0x4

In the final sshd-auth binary, you'll see both definitions (actually,
many of them) under the same name but at different addresses:

0000000000072040 <__ssp_protected_read>:
read():
/usr/include/ssp/unistd.h:39
   72040:       e9 5b 99 f9 ff          jmpq   b9a0 <read@plt>
...
000000000007226d <atomicio6>:
...
   7229c:       48 8d 05 9d fd ff ff    lea    -0x263(%rip),%rax        # 72040 <__ssp_protected_read>
   722a3:       49 39 c5                cmp    %rax,%r13
...
0000000000082bd0 <__ssp_protected_read>:
read():
/usr/include/ssp/unistd.h:39
   82bd0:       e9 cb 8d f8 ff          jmpq   b9a0 <read@plt>
...
0000000000085c6b <kex_exchange_identification>:
...
   85e05:       48 8d 3d c4 cd ff ff    lea    -0x323c(%rip),%rdi        # 82bd0 <__ssp_protected_read>
   85e0c:       e8 99 c5 fe ff          callq  723aa <atomicio>

(Note that they all jump to read@plt -- and if you were to take the
address of the read function _without_ the ssp indirections, that's
the address you would get.)

I'm not clear why we still have this extra declaration now that we've
switched to `extern inline' (which should guarantee it is never
materialized as a separate function, even if we refer to its address),
and I think we should delete it.

The way gcc reacts to such an `extern inline' declaration with an ELF
symbol __asm("__ssp_protected_read") changed between 10 and 12 --
gcc10 rejects it; gcc12 accepts it.

In any case, while we could backport the ssp.h `extern inline' change
to netbsd-10 (and remove the __asm("__ssp_protected_read") part), it
wouldn't help for packages built against 10.0.  So we have to keep
workarounds in the packages until we drop support for all NetBSD<11.