From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 4369 invoked from network); 13 Dec 2021 14:24:10 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 13 Dec 2021 14:24:10 -0000 Received: (qmail 19881 invoked by uid 550); 13 Dec 2021 14:24:07 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 19846 invoked from network); 13 Dec 2021 14:24:06 -0000 Date: Mon, 13 Dec 2021 09:23:53 -0500 From: Rich Felker To: jvoisin Cc: musl@lists.openwall.com Message-ID: <20211213142353.GG7074@brightrain.aerifal.cx> References: <20211212183440.43118-1-julien.voisin@dustri.org> <20211212193502.GF7074@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] Zero the leading stack canary byte On Mon, Dec 13, 2021 at 01:24:38PM +0100, jvoisin wrote: > > As written this patch assumes a 64-bit uintptr_t, which isn't ok. > > Indeed 56 bits should be fine on a 64-bit arch, but dropping from 32 > > to 24 on a 32-bit arch severely weakens the protection. So it probably > > needs to be conditional on 64-bit. > Will do. > > > Also, zeroing the first byte means we can no longer catch buffer > > overflows of the form "off-by-one string length". This seems > > unfortunate. Putting the 0 byte at the end would solve that at the > > expense of allowing the canary value to be leaked via missing > > termination bugs, and overall I would lean towards catching actual > > buffer overflow bugs vs stopping canary leaks. > As discussed on IRC, what about zero'ing the second byte instead? This > would allow to catch overflows, as well as preventing canary > leaks/overwrite via string-manipulating functions. That seems like a good idea too. Note that it can be done without masking logic and large constants just with: ((char *)&__stack_chk_guard)[1] = 0; And beginning or end could have been done just by reducing the memcpy length by 1. Rich