Chris Rapier <rapier at 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 at mindrot.org > https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev-------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 377 bytes Desc: not available URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20230606/bb0fe13e/attachment.asc>
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 warningsHuh, 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! ****************************************************
On Tue, Jun 6, 2023, 16:31 Sam James <sam at gentoo.org> wrote:> > Chris Rapier <rapier at 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. > >