Mailing List Archive

Chris Rapier wrote:
> openssh 9.3p1
..
> In function 'explicit_bzero',
> inlined from 'kex_free_newkeys' at kex.c:743:2:

kex.c in tag V_9_3_P1 doesn't call explicit_bzero() on line 743,

> '__explicit_bzero_chk' writing 48 bytes into a region of size 8
..
> kex.h: In function 'kex_free_newkeys':
> kex.h:116:18: note: destination object 'name' of size 8
> 116 | char *name;

... in fact kex_free_newkeys() in tag V_9_3_P1 doesn't ever call
explicit_bzero() with an object called 'name'.


> Not sure if this is a real problem or not but I thought I'd pass it
> over just in case.

Could you check if you have any patch applied on top of V_9_3_P1?


Thanks

//Peter
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

On 6/6/23 2:59 PM, Peter Stuge wrote:
> Chris Rapier wrote:
>> openssh 9.3p1
> ..
>> In function 'explicit_bzero',
>> inlined from 'kex_free_newkeys' at kex.c:743:2:
>
> kex.c in tag V_9_3_P1 doesn't call explicit_bzero() on line 743,
>> '__explicit_bzero_chk' writing 48 bytes into a region of size 8
> ..
>> kex.h: In function 'kex_free_newkeys':
>> kex.h:116:18: note: destination object 'name' of size 8
>> 116 | char *name;
>
> ... in fact kex_free_newkeys() in tag V_9_3_P1 doesn't ever call
> explicit_bzero() with an object called 'name'.
>
>
>> Not sure if this is a real problem or not but I thought I'd pass it
>> over just in case.
>
> Could you check if you have any patch applied on top of V_9_3_P1?


I'm using commit cb30fbdbee869f1ce11f06aa97e1cb8717a0b645 (HEAD, tag:
V_9_3_P1, openssh-master/V_9_3) and git diff isn't reporting anything
applied.

And I just realized I grabbed that from the wrong window (which does
have patches applied). Same thing exists in the canonical code. Here is
the accurate one:


In file included from /usr/include/string.h:535,
from kex.c:34:
In function ‘explicit_bzero’,
inlined from ‘kex_free_newkeys’ at kex.c:742:2:
/usr/include/bits/string_fortified.h:72:3: warning:
‘__explicit_bzero_chk’ writing 48 bytes into a region of size 8
overflows the destination [-Wstringop-overflow=]
72 | __explicit_bzero_chk (__dest, __len, __glibc_objsize0 (__dest));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from kex.c:53:
kex.h: In function ‘kex_free_newkeys’:
kex.h:116:18: note: destination object ‘name’ of size 8
116 | char *name;
| ^~~~
/usr/include/bits/string_fortified.h:66:6: note: in a call to function
‘__explicit_bzero_chk’ declared with attribute ‘access (write_only, 1, 2)’
66 | void __explicit_bzero_chk (void *__dest, size_t __len, size_t
__destlen)
| ^~~~~~~~~~~~~~~~~~~~

Sorry about the confusion before. I always have too many terminals open.

Chris
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Chris Rapier <rapier@psc.edu> writes:

> On 6/6/23 2:59 PM, Peter Stuge wrote:
>> Chris Rapier wrote:
>>> openssh 9.3p1
>> ..
>>> In function 'explicit_bzero',
>>> inlined from 'kex_free_newkeys' at kex.c:743:2:
>> kex.c in tag V_9_3_P1 doesn't call explicit_bzero() on line 743,
>>> '__explicit_bzero_chk' writing 48 bytes into a region of size 8
>> ..
>>> kex.h: In function 'kex_free_newkeys':
>>> kex.h:116:18: note: destination object 'name' of size 8
>>> 116 | char *name;
>> ... in fact kex_free_newkeys() in tag V_9_3_P1 doesn't ever call
>> explicit_bzero() with an object called 'name'.
>>
>>> Not sure if this is a real problem or not but I thought I'd pass it
>>> over just in case.
>> Could you check if you have any patch applied on top of V_9_3_P1?
>
>
> I'm using commit cb30fbdbee869f1ce11f06aa97e1cb8717a0b645 (HEAD, tag:
> V_9_3_P1, openssh-master/V_9_3) and git diff isn't reporting anything
> applied.
>
> And I just realized I grabbed that from the wrong window (which does
> have patches applied). Same thing exists in the canonical code. Here
> is the accurate one:
>
>
> In file included from /usr/include/string.h:535,
> from kex.c:34:
> In function ‘explicit_bzero’,
> inlined from ‘kex_free_newkeys’ at kex.c:742:2:
> /usr/include/bits/string_fortified.h:72:3: warning:
> ‘__explicit_bzero_chk’ writing 48 bytes into a region of size 8
> overflows the destination [-Wstringop-overflow=]
> 72 | __explicit_bzero_chk (__dest, __len, __glibc_objsize0 (__dest));
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from kex.c:53:
> kex.h: In function ‘kex_free_newkeys’:
> kex.h:116:18: note: destination object ‘name’ of size 8
> 116 | char *name;
> | ^~~~
> /usr/include/bits/string_fortified.h:66:6: note: in a call to function
> ‘__explicit_bzero_chk’ declared with attribute ‘access (write_only, 1,
> 2)’
> 66 | void __explicit_bzero_chk (void *__dest, size_t __len, size_t
> __destlen)
> | ^~~~~~~~~~~~~~~~~~~~
>
> Sorry about the confusion before. I always have too many terminals open.

Not a comment on this particular bug, but as an FYI, sanitizers are
known to sometimes cause false-positive *compile*-time warnings (not runtime
failures, which are pretty much always legitimate).

>
> Chris
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev@mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

On Tue, 6 Jun 2023, Sam James wrote:

>Not a comment on this particular bug, but as an FYI, sanitizers are
>known to sometimes cause false-positive *compile*-time warnings

Huh, they do?

What happens here is that it thinks the pointer to newkeys->enc
is a pointer to the first element (name) inside newkeys->enc,
which is incorrect but probably correct elsewhere and I don’t
know whether it can even distinguish them where it sits.

But looking at this… newkeys->enc is an inlined struct sshenc
inside struct newkeys, so why not just bzero the entire newkeys
at once near the end instead of doing it piecemeal as if it were
a pointer?

bye,
//mirabilos
--
Infrastrukturexperte • tarent solutions GmbH
Am Dickobskreuz 10, D-53121 Bonn • http://www.tarent.de/
Telephon +49 228 54881-393 • Fax: +49 228 54881-235
HRB AG Bonn 5168 • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

****************************************************
/?\ The UTF-8 Ribbon
? ? Campaign against Mit dem tarent-Newsletter nichts mehr verpassen:
 ?  HTML eMail! Also, https://www.tarent.de/newsletter
? ? header encryption!
****************************************************
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

Thorsten Glaser <t.glaser@tarent.de> writes:

> On Tue, 6 Jun 2023, Sam James wrote:
>
>>Not a comment on this particular bug, but as an FYI, sanitizers are
>>known to sometimes cause false-positive *compile*-time warnings
>
> Huh, they do?

Yes: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107298#c4.

This is not a comment on their runtime effectiveness (and I'm
not commenting on the code in question here, just that it's
very common to get spurious compile-time warnings you don't get otherwise
when building with sanitizers).

Thorsten Glaser wrote:
> What happens here is that it thinks the pointer to newkeys->enc
> is a pointer to the first element (name) inside newkeys->enc,
> which is incorrect

Yes, so no overflow bug.

Too bad it confuses the struct with the first member inside.


> But looking at this… newkeys->enc is an inlined struct sshenc
> inside struct newkeys, so why not just bzero the entire newkeys
> at once near the end instead of doing it piecemeal as if it were
> a pointer?

Maybe to leak as little information as possible in case of error
along the way.


//Peter
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

On Tue, Jun 6, 2023, 16:31 Sam James <sam@gentoo.org> wrote:

>
> Chris Rapier <rapier@psc.edu> writes:
>
> > On 6/6/23 2:59 PM, Peter Stuge wrote:
> >> Chris Rapier wrote:
> >>> openssh 9.3p1
> >> ..
> >>> In function 'explicit_bzero',
> >>> inlined from 'kex_free_newkeys' at kex.c:743:2:
> >> kex.c in tag V_9_3_P1 doesn't call explicit_bzero() on line 743,
> >>> '__explicit_bzero_chk' writing 48 bytes into a region of size 8
> >> ..
> >>> kex.h: In function 'kex_free_newkeys':
> >>> kex.h:116:18: note: destination object 'name' of size 8
> >>> 116 | char *name;
> >> ... in fact kex_free_newkeys() in tag V_9_3_P1 doesn't ever call
> >> explicit_bzero() with an object called 'name'.
> >>
> >>> Not sure if this is a real problem or not but I thought I'd pass it
> >>> over just in case.
> >> Could you check if you have any patch applied on top of V_9_3_P1?
> >
> >
> > I'm using commit cb30fbdbee869f1ce11f06aa97e1cb8717a0b645 (HEAD, tag:
> > V_9_3_P1, openssh-master/V_9_3) and git diff isn't reporting anything
> > applied.
> >
> > And I just realized I grabbed that from the wrong window (which does
> > have patches applied). Same thing exists in the canonical code. Here
> > is the accurate one:
> >
> >
> > In file included from /usr/include/string.h:535,
> > from kex.c:34:
> > In function ‘explicit_bzero’,
> > inlined from ‘kex_free_newkeys’ at kex.c:742:2:
> > /usr/include/bits/string_fortified.h:72:3: warning:
> > ‘__explicit_bzero_chk’ writing 48 bytes into a region of size 8
> > overflows the destination [-Wstringop-overflow=]
> > 72 | __explicit_bzero_chk (__dest, __len, __glibc_objsize0
> (__dest));
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > In file included from kex.c:53:
> > kex.h: In function ‘kex_free_newkeys’:
> > kex.h:116:18: note: destination object ‘name’ of size 8
> > 116 | char *name;
> > | ^~~~
> > /usr/include/bits/string_fortified.h:66:6: note: in a call to function
> > ‘__explicit_bzero_chk’ declared with attribute ‘access (write_only, 1,
> > 2)’
> > 66 | void __explicit_bzero_chk (void *__dest, size_t __len, size_t
> > __destlen)
> > | ^~~~~~~~~~~~~~~~~~~~
> >
> > Sorry about the confusion before. I always have too many terminals open.
>
> Not a comment on this particular bug, but as an FYI, sanitizers are
> known to sometimes cause false-positive *compile*-time warnings (not
> runtime
> failures, which are pretty much always legitimate).
>
>
> This doesn't actually surprise me. I'd have expected to see actual
> problems during runtime if it really was zeroing out 40 extra bytes each
> time it called this function. On the other hand, I wanted to run it by
> people in case there really is a problem.
>
>
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev