Date: Sun, 19 May 2024 14:47:22 +0000 (UTC) From: Pedro Giffuni <pfg@freebsd.org> To: Kyle Evans <kevans@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org> Subject: Re: git: be04fec42638 - main - Import _FORTIFY_SOURCE implementation from NetBSD Message-ID: <418178403.1722964.1716130042183@mail.yahoo.com> In-Reply-To: <216ca4a4-c0b8-4d72-bc6f-95e82a6b77da@FreeBSD.org> References: <02326b5e-a1fe-4411-a869-d21f9a76130c@email.android.com> <999469960.1638478.1716080957814@mail.yahoo.com> <6276b721-6c7b-41cd-9d1b-4169e86ec5e9@FreeBSD.org> <1413980952.1357400.1716093599901@mail.yahoo.com> <216ca4a4-c0b8-4d72-bc6f-95e82a6b77da@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] Hmm... well. In all honesty I understand I am doomed to lose this battle :).FORTIFY_SOURCE is in linux and in Apple and that weights enough that it had to find it's way to FreeBSD sooner or later. Plus I am just not much involved in FreeBSD or OSs anymore so I don't feel like stopping other people from doing development. best of lucks! Pedro. ps. Just for the reference, the Google guys did develop a document on how they implemented this for clang on bionic:https://goo.gl/8HS2dW On Sunday, May 19, 2024 at 12:13:00 AM GMT-5, Kyle Evans <kevans@freebsd.org> wrote: On 5/18/24 23:39, Pedro Giffuni wrote: > FWIW .. and let me be clear I haven't worked on this in ages and I am > not planning to retake this either... > > clang just couldn't do the static fortify_source checks due to the way > llvm uses an intermediate representation; the size just couldn't be > handled in the preprocessor. Google did spend some time adding extra > attributes to clang to improve the debugging and you can see that > implemented in bionic libc but that was it. musl didn't even try. > Admittedly, I have no idea what you're talking about here; none of this implementation requires any knowledge of anything at preproc time. __builtin_object_size() does the right thing, and the typically performance critical string/memory ops use __builtin___foo_chk() that do successfully get optimized away in the common case to the underlying foo() call. This all works very well with clang, I haven't tested it under GCC but, as you've noted, would assume that it works at least as well. > fortify_source does replace some key libc functions with memory checking > alternatives and that turns out to be annoying when debugging. In a way > it breaks that principle C programmers once had, where developers are > expected to know what they are doing, and if the error is caught at > runtime by the stack protector anyways it ends up being redundant. > > One more thing about the static checks. Most of the linux distributions > out there indeed have built their software packages with GCC and > fortify_source >=2. As a consequence, when we ran an exp-run on the > ports tree (with GCC), fortify_source didn't find anything: it was > basically a waste of time. > > Another reason for not setting it by default is performance. And here I > answer Shawn's comment on why not enable stack-protector-all and > safestack and fortify_source at the same time: running unnecessary > checks over and over again wastes energy and can have some performance > hit. The later may seem negligible in modern processors, but why do them > if they bring no benefit? (No need to answer ... just left as food for > thought) > > Pedro. > > On Saturday, May 18, 2024 at 09:08:52 PM GMT-5, Kyle Evans > <kevans@freebsd.org> wrote: > > > > > On 5/18/24 20:09, Pedro Giffuni wrote: > > (sorry for top posting .. my mailer just sucks) > > Hi; > > > > I used to like the limited static checking FORTIFY_SOURCE provides and > > when I ran it over FreeBSD it did find a couple of minor issues. It only > > works for GCC though. > > > > I don't think this is particularly true anymore; I haven't found a case > yet where __builtin_object_size(3) doesn't give me the correct size > while GCC did. I'd welcome counter-examples here, though -- we have > funding to both finish the project (widen the _FORTIFY_SOURCE net to > more of libc/libsys) and add tests to demonstrate that it's both > functional and correct. It would be useful to also document > deficiencies in the tests. > > > I guess it doesn't really hurt to have FORTIFY_SOURCE around and NetBSD > > had the least intrusive implementation the last time I checked but I > > would certainly request it should never be activated by default, > > specially with clang. The GCC version has seen more development on glibc > > but I still think its a dead end. > > > > I don't see a compelling reason to avoid enabling it by default; see > above, the functionality that we need in clang appears to be just fine > (and, iirc, was also fine when I checked at the beginning of working on > this in 2021) and it provides useful > > > What I would like to see working on FreeBSD is Safestack as a > > replacement for the stack protector, which we were so very slow to adopt > > even when it was originally developed in FreeBSD. I think other projects > > based on FreeBSD (Chimera and hardenedBSD) have been using it but I > > don't know the details. > > > > No comment there, though I think Shawn Webb / HardenedBSD had been > playing around with SafeStack (and might have enabled it? I haven't > actually looked in a while now). > > > This is just all my $0.02 > > > > Pedro. > > Thanks, > > Kyle Evans > > > > > On Saturday, May 18, 2024 at 05:54:42 PM GMT-5, Kyle Evans > > <kaevans@fastmail.com <mailto:kaevans@fastmail.com>> wrote: > > > > > > > > > > On May 18, 2024 13:42, Pedro Giffuni <pfg@freebsd.org > <mailto:pfg@freebsd.org>> wrote: > > > > Oh no .. please not... > > > > We went into that in a GSoC: > > > > > https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions > <https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions>; <https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions <https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions>>; > > > > > > Ultimately it proved to be useless since stack-protector-strong. > > > > > > Respectfully, I disagree with your conclusion here: > > > > 1.) _FORTIFY_SOURCE provides more granular detection of overflow; I > > don't have to overflow all the way into the canary at the end of the > > frame to be detected, so my minor bug now can be caught before something > > causes the stack frame to be rearranged and turn it into a security > > issue later > > > > 2.) __builtin_object_size doesn't work on heap objects, but it actually > > can work on subobjects from a heap allocation (e.g., &foo->name), so the > > coverage extends beyond the stack into starting to detect other kinds of > > overflow > > > > While the security value over stack-protector-strong may be marginal (I > > won't debate this specifically), the feature still has value in general. > > > > Thanks, > > > > Kyle Evans > [-- Attachment #2 --] <html><head></head><body><div class="ydpeac7e5yahoo-style-wrap" style="font-family:Helvetica Neue, Helvetica, Arial, sans-serif;font-size:16px;"><div></div> <div dir="ltr" data-setdir="false">Hmm... well.</div><div dir="ltr" data-setdir="false"><br></div><div dir="ltr" data-setdir="false">In all honesty I understand I am doomed to lose this battle :).</div><div dir="ltr" data-setdir="false">FORTIFY_SOURCE is in linux and in Apple and that weights enough that it had to find it's way to FreeBSD sooner or later. Plus I am just not much involved in FreeBSD or OSs anymore so I don't feel like stopping other people from doing development.</div><div dir="ltr" data-setdir="false"><br></div><div dir="ltr" data-setdir="false">best of lucks!</div><div dir="ltr" data-setdir="false"><br></div><div dir="ltr" data-setdir="false">Pedro.</div><div dir="ltr" data-setdir="false"><br></div><div dir="ltr" data-setdir="false"><div><div dir="ltr" data-setdir="false" style="color: rgb(0, 0, 0); font-family: Helvetica Neue, Helvetica, Arial, sans-serif; font-size: 16px; outline: none !important;">ps. Just for the reference, the Google guys did develop a document on how they implemented this for clang on bionic:</div><div dir="ltr" data-setdir="false" style="color: rgb(0, 0, 0); font-family: Helvetica Neue, Helvetica, Arial, sans-serif; font-size: 16px; outline: none !important;"><a href="https://goo.gl/8HS2dW" style="color: rgb(25, 106, 212); text-decoration-line: underline; outline: none !important;" rel="nofollow" target="_blank">https://goo.gl/8HS2dW</a></div></div><br></div><div><br></div>; </div><div id="ydp256e56f3yahoo_quoted_7121969370" class="ydp256e56f3yahoo_quoted"> <div style="font-family:'Helvetica Neue', Helvetica, Arial, sans-serif;font-size:13px;color:#26282a;"> <div> On Sunday, May 19, 2024 at 12:13:00 AM GMT-5, Kyle Evans <kevans@freebsd.org> wrote: </div> <div><br></div> <div><br></div> <div><div dir="ltr">On 5/18/24 23:39, Pedro Giffuni wrote:<br></div><div dir="ltr">> FWIW .. and let me be clear I haven't worked on this in ages and I am <br></div><div dir="ltr">> not planning to retake this either...<br></div><div dir="ltr">> <br></div><div dir="ltr">> clang just couldn't do the static fortify_source checks due to the way <br></div><div dir="ltr">> llvm uses an intermediate representation; the size just couldn't be <br></div><div dir="ltr">> handled in the preprocessor. Google did spend some time adding extra <br></div><div dir="ltr">> attributes to clang to improve the debugging and you can see that <br></div><div dir="ltr">> implemented in bionic libc but that was it. musl didn't even try.<br></div><div dir="ltr">> <br></div><div dir="ltr"><br></div><div dir="ltr">Admittedly, I have no idea what you're talking about here; none of this <br></div><div dir="ltr">implementation requires any knowledge of anything at preproc time. <br></div><div dir="ltr">__builtin_object_size() does the right thing, and the typically <br></div><div dir="ltr">performance critical string/memory ops use __builtin___foo_chk() that do <br></div><div dir="ltr">successfully get optimized away in the common case to the underlying <br></div><div dir="ltr">foo() call. This all works very well with clang, I haven't tested it <br></div><div dir="ltr">under GCC but, as you've noted, would assume that it works at least as well.<br></div><div dir="ltr"><br></div><div dir="ltr">> fortify_source does replace some key libc functions with memory checking <br></div><div dir="ltr">> alternatives and that turns out to be annoying when debugging. In a way <br></div><div dir="ltr">> it breaks that principle C programmers once had, where developers are <br></div><div dir="ltr">> expected to know what they are doing, and if the error is caught at <br></div><div dir="ltr">> runtime by the stack protector anyways it ends up being redundant.<br></div><div dir="ltr">> > One more thing about the static checks. Most of the linux distributions<br></div><div dir="ltr">> out there indeed have built their software packages with GCC and <br></div><div dir="ltr">> fortify_source >=2. As a consequence, when we ran an exp-run on the <br></div><div dir="ltr">> ports tree (with GCC), fortify_source didn't find anything: it was <br></div><div dir="ltr">> basically a waste of time.<br></div><div dir="ltr">> <br></div><div dir="ltr">> Another reason for not setting it by default is performance. And here I <br></div><div dir="ltr">> answer Shawn's comment on why not enable stack-protector-all and <br></div><div dir="ltr">> safestack and fortify_source at the same time: running unnecessary <br></div><div dir="ltr">> checks over and over again wastes energy and can have some performance <br></div><div dir="ltr">> hit. The later may seem negligible in modern processors, but why do them <br></div><div dir="ltr">> if they bring no benefit? (No need to answer ... just left as food for <br></div><div dir="ltr">> thought)<br></div><div dir="ltr">> <br></div><div dir="ltr">> Pedro.<br></div><div dir="ltr">> <br></div><div dir="ltr">> On Saturday, May 18, 2024 at 09:08:52 PM GMT-5, Kyle Evans <br></div><div dir="ltr">> <<a href="mailto:kevans@freebsd.org" rel="nofollow" target="_blank">kevans@freebsd.org</a>> wrote:<br></div><div dir="ltr">> <br></div><div dir="ltr">> <br></div><div dir="ltr">> <br></div><div dir="ltr">> <br></div><div dir="ltr">> On 5/18/24 20:09, Pedro Giffuni wrote:<br></div><div dir="ltr">> > (sorry for top posting .. my mailer just sucks)<br></div><div dir="ltr">> > Hi;<br></div><div dir="ltr">> ><br></div><div dir="ltr">> > I used to like the limited static checking FORTIFY_SOURCE provides and<br></div><div dir="ltr">> > when I ran it over FreeBSD it did find a couple of minor issues. It only<br></div><div dir="ltr">> > works for GCC though.<br></div><div dir="ltr">> ><br></div><div dir="ltr">> <br></div><div dir="ltr">> I don't think this is particularly true anymore; I haven't found a case<br></div><div dir="ltr">> yet where __builtin_object_size(3) doesn't give me the correct size<br></div><div dir="ltr">> while GCC did. I'd welcome counter-examples here, though -- we have<br></div><div dir="ltr">> funding to both finish the project (widen the _FORTIFY_SOURCE net to<br></div><div dir="ltr">> more of libc/libsys) and add tests to demonstrate that it's both<br></div><div dir="ltr">> functional and correct. It would be useful to also document<br></div><div dir="ltr">> deficiencies in the tests.<br></div><div dir="ltr">> <br></div><div dir="ltr">> > I guess it doesn't really hurt to have FORTIFY_SOURCE around and NetBSD<br></div><div dir="ltr">> > had the least intrusive implementation the last time I checked but I<br></div><div dir="ltr">> > would certainly request it should never be activated by default,<br></div><div dir="ltr">> > specially with clang. The GCC version has seen more development on glibc<br></div><div dir="ltr">> > but I still think its a dead end.<br></div><div dir="ltr">> ><br></div><div dir="ltr">> <br></div><div dir="ltr">> I don't see a compelling reason to avoid enabling it by default; see<br></div><div dir="ltr">> above, the functionality that we need in clang appears to be just fine<br></div><div dir="ltr">> (and, iirc, was also fine when I checked at the beginning of working on<br></div><div dir="ltr">> this in 2021) and it provides useful<br></div><div dir="ltr">> <br></div><div dir="ltr">> > What I would like to see working on FreeBSD is Safestack as a<br></div><div dir="ltr">> > replacement for the stack protector, which we were so very slow to adopt<br></div><div dir="ltr">> > even when it was originally developed in FreeBSD. I think other projects<br></div><div dir="ltr">> > based on FreeBSD (Chimera and hardenedBSD) have been using it but I<br></div><div dir="ltr">> > don't know the details.<br></div><div dir="ltr">> ><br></div><div dir="ltr">> <br></div><div dir="ltr">> No comment there, though I think Shawn Webb / HardenedBSD had been<br></div><div dir="ltr">> playing around with SafeStack (and might have enabled it? I haven't<br></div><div dir="ltr">> actually looked in a while now).<br></div><div dir="ltr">> <br></div><div dir="ltr">> > This is just all my $0.02<br></div><div dir="ltr">> ><br></div><div dir="ltr">> > Pedro.<br></div><div dir="ltr">> <br></div><div dir="ltr">> Thanks,<br></div><div dir="ltr">> <br></div><div dir="ltr">> Kyle Evans<br></div><div dir="ltr">> <br></div><div dir="ltr">> ><br></div><div dir="ltr">> > On Saturday, May 18, 2024 at 05:54:42 PM GMT-5, Kyle Evans<br></div><div dir="ltr">> > <<a href="mailto:kaevans@fastmail.com" rel="nofollow" target="_blank">kaevans@fastmail.com</a> <mailto:kaevans@fastmail.com>> wrote:<br></div><div dir="ltr">> ><br></div><div dir="ltr">> ><br></div><div dir="ltr">> ><br></div><div dir="ltr">> ><br></div><div dir="ltr">> > On May 18, 2024 13:42, Pedro Giffuni <<a href="mailto:pfg@freebsd.org" rel="nofollow" target="_blank">pfg@freebsd.org</a> <br></div><div dir="ltr">> <mailto:pfg@freebsd.org>> wrote:<br></div><div dir="ltr">> ><br></div><div dir="ltr">> > Oh no .. please not...<br></div><div dir="ltr">> ><br></div><div dir="ltr">> > We went into that in a GSoC:<br></div><div dir="ltr">> ><br></div><div dir="ltr">> > <br></div><div dir="ltr">> <a href="https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions" rel="nofollow" target="_blank">https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions</a>; <br></div><div dir="ltr">> <<a href="https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions" rel="nofollow" target="_blank">https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions</a>> <<a href="https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions" rel="nofollow" target="_blank">https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions</a>; <<a href="https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions" rel="nofollow" target="_blank">https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions</a>>><br></div><div dir="ltr">> ><br></div><div dir="ltr">> ><br></div><div dir="ltr">> > Ultimately it proved to be useless since stack-protector-strong.<br></div><div dir="ltr">> ><br></div><div dir="ltr">> ><br></div><div dir="ltr">> > Respectfully, I disagree with your conclusion here:<br></div><div dir="ltr">> ><br></div><div dir="ltr">> > 1.) _FORTIFY_SOURCE provides more granular detection of overflow; I<br></div><div dir="ltr">> > don't have to overflow all the way into the canary at the end of the<br></div><div dir="ltr">> > frame to be detected, so my minor bug now can be caught before something<br></div><div dir="ltr">> > causes the stack frame to be rearranged and turn it into a security<br></div><div dir="ltr">> > issue later<br></div><div dir="ltr">> ><br></div><div dir="ltr">> > 2.) __builtin_object_size doesn't work on heap objects, but it actually<br></div><div dir="ltr">> > can work on subobjects from a heap allocation (e.g., &foo->name), so the<br></div><div dir="ltr">> > coverage extends beyond the stack into starting to detect other kinds of<br></div><div dir="ltr">> > overflow<br></div><div dir="ltr">> ><br></div><div dir="ltr">> > While the security value over stack-protector-strong may be marginal (I<br></div><div dir="ltr">> > won't debate this specifically), the feature still has value in general.<br></div><div dir="ltr">> ><br></div><div dir="ltr">> > Thanks,<br></div><div dir="ltr">> ><br></div><div dir="ltr">> > Kyle Evans<br></div><div dir="ltr">> <br></div></div> </div> </div></body></html>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?418178403.1722964.1716130042183>