Use more hardening flags:

Use more hardening flags:

-D_FORTIFY_SOURCE=2
-Wl,-z,now (aka BIND_NOW)
-Wl,-z,relro (read-only relocation tables)

BUG=55439
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=168889

[cevans, 2012-11-16 21:24:45]

Can I defer to jln@ or perhaps keescook@ ? These guys both know a lot more
than me in this area.

On Fri, Nov 16, 2012 at 12:08 PM, <phajdan.jr@chromium.org> wrote:

> Reviewers: Chris Evans,
>
> Description:
> Use more hardening flags:
>
> -D_FORTIFY_SOURCE=2
> -Wl,-z,now (aka BIND_NOW)
> -Wl,-z,relro (read-only relocation tables)
>
> BUG=55439
>
>
> Please review this at
http://codereview.chromium.**org/11411022/<http://codereview.chromium.org/114...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M build/common.gypi
>
>
> Index: build/common.gypi
> diff --git a/build/common.gypi b/build/common.gypi
> index 319e866561961edfb79bcfc31b5554**86590d5a25..**
> 7b1c73a435a5dcda52dc0054774786**121468bc18 100644
> --- a/build/common.gypi
> +++ b/build/common.gypi
> @@ -2243,6 +2243,25 @@
>      },
>    },
>    'conditions': [
> +    ['os_posix==1', {
> +      'target_defaults': {
> +        'cflags': [
> +          '-fstack-protector-all',
> +        ],
> +        'ldflags': [
> +          '-Wl,-z,now',
> +          '-Wl,-z,relro',
> +        ],
> +        'conditions': [
> +          ['chromium_code==1', {
> +            # Non-chromium code is not guaranteed to compile cleanly with
> _FORTIFY_SOURCE.
> +            'defines': [
> +              '_FORTIFY_SOURCE=2',
> +            ],
> +          }],
> +        ],
> +      },
> +    }],
>      ['os_posix==1 and OS!="mac" and OS!="ios"', {
>        'target_defaults': {
>          # Enable -Werror by default, but put it in a variable so it can
>
>
>

[Paweł Hajdan Jr., 2012-11-16 21:30:25]

Kees, could you take a look?

[Kees Cook, 2012-11-16 21:44:57]

On 2012/11/16 21:30:25, Paweł Hajdan Jr. wrote:
> Kees, could you take a look?

Are these the global build defaults? Are you sure you want
-fstack-protector-all? On Chrome OS, for example, we use the not-yet-upstream
-fstack-protector-strong.

The other options seem fine to me.

[Paweł Hajdan Jr., 2012-11-16 22:23:43]

On 2012/11/16 21:44:57, Kees Cook wrote:
> On 2012/11/16 21:30:25, Paweł Hajdan Jr. wrote:
> > Kees, could you take a look?
> 
> Are these the global build defaults?

Yes.

> Are you sure you want -fstack-protector-all?

What are potential downsides? Increased binary size or slowness?

> On Chrome OS, for example, we use the not-yet-upstream
> -fstack-protector-strong.

For now I'd prefer to only use flags that are supported by Ubuntu's LTS gcc.

[Kees Cook, 2012-11-16 23:39:27]

On 2012/11/16 22:23:43, Paweł Hajdan Jr. wrote:
> On 2012/11/16 21:44:57, Kees Cook wrote:
> > On 2012/11/16 21:30:25, Paweł Hajdan Jr. wrote:
> > > Kees, could you take a look?
> > 
> > Are these the global build defaults?
> 
> Yes.
> 
> > Are you sure you want -fstack-protector-all?
> 
> What are potential downsides? Increased binary size or slowness?

Both, yes. Using "-fstack-protector --param=ssp-buffer-size=4" tends to be
sufficient, but -all is more paranoid. I'm mostly worried that if the default is
-fstack-protector-all and a builder adds -fstack-protector-strong, it'll
continue to be -all.

> > On Chrome OS, for example, we use the not-yet-upstream
> > -fstack-protector-strong.
> 
> For now I'd prefer to only use flags that are supported by Ubuntu's LTS gcc.

Absolutely. As mentioned above, I'm worried about argument precedence.

[Paweł Hajdan Jr., 2012-11-19 21:24:32]

PTAL; I changed the flag to -fstack-protector, and impact on Release size binary
is negligible (less than 0.1%)

On 2012/11/16 23:39:27, Kees Cook wrote:
> Both, yes. Using "-fstack-protector --param=ssp-buffer-size=4" tends to be
> sufficient, but -all is more paranoid. I'm mostly worried that if the default
is
> -fstack-protector-all and a builder adds -fstack-protector-strong, it'll
> continue to be -all.

That makes sense to me. I've changed that to -fstack-protector
--param=ssp-buffer-size=4 . WDYT?

[Kees Cook, 2012-11-19 22:03:14]

On 2012/11/19 21:24:32, Paweł Hajdan Jr. wrote:
> PTAL; I changed the flag to -fstack-protector, and impact on Release size
binary
> is negligible (less than 0.1%)
> 
> On 2012/11/16 23:39:27, Kees Cook wrote:
> > Both, yes. Using "-fstack-protector --param=ssp-buffer-size=4" tends to be
> > sufficient, but -all is more paranoid. I'm mostly worried that if the
default
> is
> > -fstack-protector-all and a builder adds -fstack-protector-strong, it'll
> > continue to be -all.
> 
> That makes sense to me. I've changed that to -fstack-protector
> --param=ssp-buffer-size=4 . WDYT?

LGTM

[Kees Cook, 2012-11-19 22:03:39]

lgtm

[Kees Cook, 2012-11-19 22:05:16]

On 2012/11/19 22:03:39, Kees Cook wrote:
> lgtm

Sorry for the repeat.

It may also be worth adding "-Wformat-security" and "-Werror=format-security" in
a separate CL, as that will abort the build when there are potential format
string problems.

[commit-bot: I haz the power, 2012-11-19 22:31:25]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.